feat: instanceID support for argo server. Closes #2004 #2365
Conversation
whynowy
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2365 +/- ##
=========================================
Coverage ? 13.26%
=========================================
Files ? 71
Lines ? 25230
Branches ? 0
=========================================
Hits ? 3346
Misses ? 21450
Partials ? 434
Continue to review full report at Codecov.
|
whynowy
changed the title
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &cronworkflowpkg.CronWorkflowDeletedResponse{}, nil | ||
| } | ||
|
|
||
| func (c *cronWorkflowServiceServer) withInstanceID(opt metav1.ListOptions) metav1.ListOptions { | ||
| if c.mode == KubeClientMode { |
There was a problem hiding this comment.
should also work for kube client - should not need the guard condition
There was a problem hiding this comment.
This is needed.
The expected behaviors for workflow list on GRPC server and kube-client are different.
-
On
GRPC server, we expect it to list the workflows either noinstanceID(instanceID == "") orinstanceIDis the same (intanceID != ""). -
When it's
kube-client, we wantargo listto list all the workflows without any instanceID relatedlabelSelector, without this we can not achieve that.
There was a problem hiding this comment.
I'm not sure about this. As much as possible, both clients should work in the same manner.
This would mean that the GRPC server should be the same as the kube-client.
Can you merge the labels together?
I think you're doing great work btw.
There was a problem hiding this comment.
Thanks.
argo server client works the same as GRPC server, but I don't think kube-client does.
A simple question, what is expected behavior for kube-client if there are multiple workflow controllers with different instanceid installed in one cluster? As a kube-client, you can not be the same as any of the argo servers, instead of being limited to a subset of the objects (intanceid lableselector), by default you should have the ability to see whatever you want to see.
There was a problem hiding this comment.
No, I don't think we want that. The kube-client is legacy code really, so we should not make extra effort to maintain any esoteric features. If you need to make a choice here, can you do whatever is simpler. I can even accept breaking changes to the kube-client.
There was a problem hiding this comment.
If this is the case, let's pick up the legacy client code, if later on we decide not to support it any more, we can easily dump it.
|
@alexec - could you review it again? |
Closes #2004
Make Argo server behaves the same as workflow controller:
Argo server only manages the workflows/CronWorkflows with the same
instanceID(labelxxxx/controller-instanceid), or wfs/cronWfs withoutinstanceID, depends on ifinstanceIDis configured in the ConfigMap.argocli withoutARGO_SERVERENV goes through kube api,argo listorargo cron listcan see all the results.argo submitorargo cron createneeds to give--instanceid=xxxxto create wf/cronWf targeting a specific controller.argocli withARGO_SERVERENV uses argo server, results ofargo listandargo cron listmight be different, depends oninstanceIDconfiguration in the ConfigMap. As a result,argo submitorargo cron createby default will include theinstanceIDin the labels if it's configured in the ConfigMap.Checklist:
"fix(controller): Updates such and such. Fixes #1234".