-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ui): Use proper podname for containersets. Fixes #13038 #13039
base: main
Are you sure you want to change the base?
Conversation
ui/src/app/shared/pod-name.ts
Outdated
@@ -19,7 +19,8 @@ export function getPodName(workflow: Workflow, node: NodeStatus): string { | |||
} | |||
|
|||
const workflowName = workflow.metadata.name; | |||
if (workflowName === node.name) { | |||
const podNodeName = node.type == 'Container' ? node.name.replace(/\.[^/.]+$/, '') : node.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm this doesn't seem to match the back-end. these two functions should be equivalent per the comment on line 14 in this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, reverted the change and moved it outside the function call. Maybe there are still better ways to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if you could locate where in the back-end this gets set, that way we can keep them equivalent, whether in this function or elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the pod is created based on the pod node name.
argo-workflows/workflow/controller/container_set_template.go
Lines 20 to 24 in f492ea3
_, err = woc.createWorkflowPod(ctx, nodeName, tmpl.ContainerSet.GetContainers(), tmpl, &createWorkflowPodOpts{ | |
includeScriptOutput: includeScriptOutput, | |
onExitPod: opts.onExitTemplate, | |
executionDeadline: opts.executionDeadline, | |
}) |
Then the containerset nodes are created
argo-workflows/workflow/controller/container_set_template.go
Lines 31 to 46 in f492ea3
for _, c := range tmpl.ContainerSet.GetContainers() { | |
ctxNodeName := fmt.Sprintf("%s.%s", nodeName, c.Name) | |
_, err := woc.wf.GetNodeByName(ctxNodeName) | |
if err != nil { | |
_ = woc.initializeNode(ctxNodeName, wfv1.NodeTypeContainer, templateScope, orgTmpl, node.ID, wfv1.NodePending, opts.nodeFlag) | |
} | |
} | |
for _, c := range tmpl.ContainerSet.GetGraph() { | |
ctrNodeName := fmt.Sprintf("%s.%s", nodeName, c.Name) | |
if len(c.Dependencies) == 0 { | |
woc.addChildNode(nodeName, ctrNodeName) | |
} | |
for _, v := range c.Dependencies { | |
woc.addChildNode(fmt.Sprintf("%s.%s", nodeName, v), ctrNodeName) | |
} | |
} |
Where the container node names are just their pod node name concatenated with the container name.
ctxNodeName := fmt.Sprintf("%s.%s", nodeName, c.Name)
So removing this postfix should map from containerset node name to pod node name, which can be used to calculate the pod name using the existing function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it looks like ContainerSet node -> Pod node name never happens directly in the back-end, which explains why they wouldn't be equivalent in this case.
In that case, I'd prefer your earlier variant, since I intentionally consolidated getPodName
in #12964.
Let's just leave a comment on why this block differs from the back-end then for explicitness
cd6e0fd
to
495f907
Compare
8447e69
to
5bd55eb
Compare
Signed-off-by: instauro <instauro@proton.me>
5bd55eb
to
262313e
Compare
Fixes #13038
Motivation
Containerset nodes currently do not retrieve their logs in the UI. This is due to the podname calculation failing, which makes the request to the backend incorrect. This fix will hopefully become obsolete when #12528 is fixed, but until then this should do the trick.
Modifications
Uses a better podname when the node type is
Container
.Verification
Tried a few different containerset templates to verify the fix, and a few none-containetset templates to verify that there is no regression.