fix: Add pod name format annotation. Fixes #6962 and #6989 #6982

JPZ13 · 2021-10-19T23:22:19Z

Signed-off-by: J.P. Zivalich jp@pipekit.io

This PR:

Adds the pod name version as an annotation to a workflow
Displays ui logs for a given workflow based on the annotation
Adds a function to get a template name from a template ref

codecov · 2021-10-19T23:28:32Z

Codecov Report

Merging #6982 (f4c7cee) into master (64fce4a) will decrease coverage by 0.03%.
The diff coverage is 48.00%.

❗ Current head f4c7cee differs from pull request most recent head 5d72b97. Consider uploading reports for the commit 5d72b97 to get more accurate results

@@            Coverage Diff             @@
##           master    #6982      +/-   ##
==========================================
- Coverage   48.55%   48.52%   -0.04%     
==========================================
  Files         265      265              
  Lines       19329    19367      +38     
==========================================
+ Hits         9386     9398      +12     
- Misses       8892     8914      +22     
- Partials     1051     1055       +4

Impacted Files	Coverage Δ
workflow/common/common.go	`80.00% <ø> (ø)`
workflow/util/pod_name.go	`48.00% <31.57%> (+1.33%)`	⬆️
workflow/controller/operator.go	`71.62% <100.00%> (+0.08%)`	⬆️
util/tls/tls.go	`52.17% <0.00%> (-12.12%)`	⬇️
workflow/metrics/server.go	`13.23% <0.00%> (-6.07%)`	⬇️
config/config.go	`28.57% <0.00%> (-3.01%)`	⬇️
cmd/argo/commands/get.go	`58.30% <0.00%> (-0.59%)`	⬇️
workflow/controller/workflowpod.go	`73.89% <0.00%> (-0.52%)`	⬇️
workflow/controller/controller.go	`22.53% <0.00%> (-0.07%)`	⬇️
workflow/metrics/metrics.go	`76.11% <0.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64fce4a...5d72b97. Read the comment docs.

JPZ13 · 2021-10-20T00:21:40Z

@tczhao I validated that the default v2 logs are showing up properly. I have some other obligations to attend to this evening and can't validate the v1 path until tomorrow. In the meantime, are you free to review?

tczhao · 2021-10-20T00:34:28Z

Thanks @JPZ13, will take a look in my afternoon

tczhao · 2021-10-20T04:14:24Z

workflow/controller/operator.go

+
+// setWfPodNamesAnnotation sets an annotation on a workflow with the pod naming
+// convention version
+func setWfPodNamesAnnotation(wf *wfv1.Workflow) {


maybe need a test for this function, the rest looks good

tczhao · 2021-10-20T04:21:18Z

workflow/controller/operator.go

@@ -142,6 +142,8 @@ func newWorkflowOperationCtx(wf *wfv1.Workflow, wfc *WorkflowController) *wfOper
 	// You can use DeepCopy() to make a deep copy of original object and modify this copy
 	// Or create a copy manually for better performance
 	wfCopy := wf.DeepCopyObject().(*wfv1.Workflow)
+	setWfPodNamesAnnotation(wfCopy)


I'm thinking perhaps if we should annotate at pod level instead?
so that in rolling upgrade (podname v1->v2), we won't end up having a workflow that has first n pods using v1 but get annotated as v2

Makes sense. @tczhao can you help me with these two questions to clarify my mental model?:

Does newWorkflowOperationCtx get invoked in the case of a rolling upgrade? If so, when and where?

Are pod annotations persisted when the workflow is archived? If so, where can I get them from?

I think newWorkflowOperationCtx get invoked at least every time when there's a workflow update e.g. pod finished

good point, pod annotation doesn't persist in workflow crd

I wonder if we can only add annotation for new workflow
or just accept this as a caveat

We want to support rolling upgrades. In that case, workflows are either:

Complete, not annotated - v1.

Already running, not annotated - therefore v1.

New (status.pending="") - annotate with pod name version from config.

Already running, annotated - use annotation.

I think that means we can be confident of which format we are using.

There is a specific block of code which sets up new workflows:

https://github.com/argoproj/argo-workflows/blob/master/workflow/controller/operator.go#L264

That's the place to add the annotation.

tczhao · 2021-10-21T04:34:11Z

Hi @JPZ13 ,
I thought it's more relevant here since this PR is about pod name v1/v2 mappings

I think we need to modify this

argo-workflows/ui/src/app/shared/services/workflows-service.ts

Lines 192 to 207 in c5b1533

    
           public getContainerLogs(workflow: Workflow, nodeId: string, container: string, grep: string, archived: boolean): Observable<LogEntry> { 
        
               const getLogsFromArtifact = () => this.getContainerLogsFromArtifact(workflow, nodeId, container, grep, archived); 
        
               // If our workflow is archived, don't even bother inspecting the cluster for logs since it's likely 
        
               // that the Workflow and associated pods have been deleted 
        
               if (archived) { 
        
                   return getLogsFromArtifact(); 
        
               } 
        
               // return archived log if main container is finished and has artifact 
        
               return Observable.fromPromise(this.isWorkflowNodePendingOrRunning(workflow, nodeId)).switchMap(isPendingOrRunning => { 
        
                   if (!isPendingOrRunning && this.hasArtifactLogs(workflow, nodeId, container) && container === 'main') { 
        
                       return getLogsFromArtifact(); 
        
                   } 
        
                   return this.getContainerLogsFromCluster(workflow, nodeId, container, grep).catch(getLogsFromArtifact); 
        
               }); 
        
           }

to have the live log uses podname and the archive log uses nodeId

backend:
(live: find log using pod name in k8s)
(archive: find archived log location using workflow[nodeId])

this should solve #6947

JPZ13 · 2021-10-22T19:08:50Z

Thanks for the heads up on that @tczhao! I'm hopefully gonna get this PR wrapped up this weekend. Can I get your eyes on it once complete?

tczhao · 2021-10-26T22:50:05Z

LGTM

alexec · 2021-10-25T16:54:08Z

ui/package.json

@@ -12,6 +12,7 @@
        "test": "jest"
    },
    "dependencies": {
+        "@types/lodash": "^4.14.175",


this library has security vulns, can we avoid it?

alexec · 2021-10-25T16:55:03Z

ui/src/app/shared/pod-name.ts

+            return workflowName;
+        }
+
+        let prefix = `${workflowName}-${templateName}`;


minor - inline

alexec · 2021-10-25T16:55:17Z

ui/src/app/shared/pod-name.ts

+        return node.templateName;
+    }
+
+    const name = get(node, 'templateRef.template');


minor - inline

alexec · 2021-10-25T16:58:17Z

workflow/controller/operator.go

@@ -142,6 +142,8 @@ func newWorkflowOperationCtx(wf *wfv1.Workflow, wfc *WorkflowController) *wfOper
 	// You can use DeepCopy() to make a deep copy of original object and modify this copy
 	// Or create a copy manually for better performance
 	wfCopy := wf.DeepCopyObject().(*wfv1.Workflow)
+	setWfPodNamesAnnotation(wfCopy)


We want to support rolling upgrades. In that case, workflows are either:

Complete, not annotated - v1.

Already running, not annotated - therefore v1.

New (status.pending="") - annotate with pod name version from config.

Already running, annotated - use annotation.

I think that means we can be confident of which format we are using.

There is a specific block of code which sets up new workflows:

https://github.com/argoproj/argo-workflows/blob/master/workflow/controller/operator.go#L264

That's the place to add the annotation.

Signed-off-by: J.P. Zivalich <jp@pipekit.io> Add annotation reading to ui Signed-off-by: J.P. Zivalich <jp@pipekit.io> Refactor get annotations Signed-off-by: J.P. Zivalich <jp@pipekit.io> Fix lint error Signed-off-by: J.P. Zivalich <jp@pipekit.io> Fix template names for template refs Signed-off-by: J.P. Zivalich <jp@pipekit.io> Fix linting errors Signed-off-by: J.P. Zivalich <jp@pipekit.io> Fix live logs vs archived logs Signed-off-by: J.P. Zivalich <jp@pipekit.io> Fix failing unit test Signed-off-by: J.P. Zivalich <jp@pipekit.io> Add test Signed-off-by: J.P. Zivalich <jp@pipekit.io> Change wf marking location Signed-off-by: J.P. Zivalich <jp@pipekit.io> Update ui Signed-off-by: J.P. Zivalich <jp@pipekit.io> Remove lodash Signed-off-by: J.P. Zivalich <jp@pipekit.io>

JPZ13 · 2021-10-28T16:57:38Z

Updated for your changes @alexec

workflow/controller/operator.go

Signed-off-by: J.P. Zivalich <jp@pipekit.io>

workflow/controller/operator_test.go

alexec · 2021-10-28T22:01:00Z

workflow/util/pod_name.go

+const (
+	// PodNameVersion1 is the v1 name that uses node ids for pod names
+	PodNameVersion1 PodNameVersion = "v1"
+	// PodNameVersion2 is the v2 name that uses workflow name combined with


um, nodeID + templateName?

minor PodNameV1

alexec

see minor comments

Signed-off-by: J.P. Zivalich <jp@pipekit.io>

workflow/controller/operator.go

ui/src/app/shared/pod-name.ts

JPZ13 force-pushed the archive-logs branch from 26d30c7 to 4b53396 Compare October 20, 2021 00:09

tczhao reviewed Oct 20, 2021

View reviewed changes

JPZ13 mentioned this pull request Oct 20, 2021

UI logs for multi-step workflows with templateRef still missing in v3.2.1 #6989

Closed

alexec linked an issue Oct 20, 2021 that may be closed by this pull request

UI logs for multi-step workflows with templateRef still missing in v3.2.1 #6989

Closed

JPZ13 changed the title ~~fix: Add pod name format annotation. Fixes #6962~~ fix: Add pod name format annotation. Fixes #6962 and #6989 Oct 20, 2021

JPZ13 force-pushed the archive-logs branch from a85aa2b to df91642 Compare October 22, 2021 19:50

JPZ13 marked this pull request as ready for review October 23, 2021 23:46

whynowy assigned alexec Oct 25, 2021

alexec reviewed Oct 26, 2021

View reviewed changes

JPZ13 force-pushed the archive-logs branch from 8f87166 to 07f41a9 Compare October 28, 2021 00:43

alexec reviewed Oct 28, 2021

View reviewed changes

workflow/controller/operator.go Outdated Show resolved Hide resolved

Remove object meta

f4c7cee

Signed-off-by: J.P. Zivalich <jp@pipekit.io>

alexec reviewed Oct 28, 2021

View reviewed changes

workflow/controller/operator_test.go Show resolved Hide resolved

alexec reviewed Oct 28, 2021

View reviewed changes

alexec requested changes Oct 28, 2021

View reviewed changes

Make minor changes for alex

5d72b97

Signed-off-by: J.P. Zivalich <jp@pipekit.io>

alexec approved these changes Oct 29, 2021

View reviewed changes

alexec merged commit 1e87159 into argoproj:master Oct 29, 2021

alexec mentioned this pull request Nov 5, 2021

v3.2.4 cherry-pick #7171

Closed

25 tasks

JPZ13 mentioned this pull request Nov 16, 2021

Pod logs no longer displayed in UI with POD_NAMES v2 since v3.2.2 #7221

Closed

sarabala1979 mentioned this pull request Dec 15, 2021

v3.2.5 Release #7414

Closed

73 tasks

sarabala1979 pushed a commit that referenced this pull request Dec 15, 2021

fix: Add pod name format annotation. Fixes #6962 and #6989 (#6982)

f5f6899

alexec mentioned this pull request Jan 20, 2022

v3.2.6 Argo UI failed to render pod log #7595

Closed

shadiramadan mentioned this pull request Feb 2, 2022

Passing k8s secret to a workflow as input #5506

Open

alexec mentioned this pull request Feb 4, 2022

UI: e.TemplateRef is undefined when using inline template #7749

Closed

JPZ13 mentioned this pull request Feb 17, 2022

REQUEST: New membership for JPZ13 argoproj/argoproj#113

Closed

5 tasks

sarabala1979 mentioned this pull request Mar 1, 2022

v3.2.9 Cherry-pick #8043

Closed

crenshaw-dev mentioned this pull request Nov 2, 2023

REQUEST: Promotion to reviewer for JPZ13 argoproj/argoproj#186

Open

5 tasks

agilgur5 reviewed Apr 22, 2024

View reviewed changes

workflow/controller/operator.go Show resolved Hide resolved

ui/src/app/shared/pod-name.ts Show resolved Hide resolved

This was referenced Apr 22, 2024

No logs accessible in the UI on workflows that went from Pending to Running #10178

Open

refactor: simplify getPodName and make consistent with back-end #12964

Merged

fix(controller): set pod name version annotation when no lock #12965

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add pod name format annotation. Fixes #6962 and #6989 #6982

fix: Add pod name format annotation. Fixes #6962 and #6989 #6982

JPZ13 commented Oct 19, 2021 •

edited

codecov bot commented Oct 19, 2021 •

edited

JPZ13 commented Oct 20, 2021

tczhao commented Oct 20, 2021

tczhao Oct 20, 2021 •

edited

tczhao Oct 20, 2021

JPZ13 Oct 20, 2021

tczhao Oct 20, 2021

alexec Oct 25, 2021

tczhao commented Oct 21, 2021

JPZ13 commented Oct 22, 2021

tczhao commented Oct 26, 2021

alexec Oct 25, 2021

JPZ13 Oct 27, 2021

alexec Oct 25, 2021

alexec Oct 25, 2021

alexec Oct 25, 2021

JPZ13 commented Oct 28, 2021

alexec Oct 28, 2021

alexec Oct 28, 2021

alexec left a comment

fix: Add pod name format annotation. Fixes #6962 and #6989 #6982

fix: Add pod name format annotation. Fixes #6962 and #6989 #6982

Conversation

JPZ13 commented Oct 19, 2021 • edited

codecov bot commented Oct 19, 2021 • edited

Codecov Report

JPZ13 commented Oct 20, 2021

tczhao commented Oct 20, 2021

tczhao Oct 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tczhao commented Oct 21, 2021

JPZ13 commented Oct 22, 2021

tczhao commented Oct 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JPZ13 commented Oct 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec left a comment

Choose a reason for hiding this comment

JPZ13 commented Oct 19, 2021 •

edited

codecov bot commented Oct 19, 2021 •

edited

tczhao Oct 20, 2021 •

edited