fix(controller): Fix getPodByNode, TestGetPodByNode. Fixes #6458 by micahbeeman-wf · Pull Request #6897 · argoproj/argo-workflows

micahbeeman-wf · 2021-10-08T16:15:55Z

Signed-off-by: Micah Beeman micah.beeman@workiva.com

getPodByNode was broken by the changes in #6712
Fixed by using the PodName util to obtain the pod name to pass to getPod
Added sleep after pods are created to fix flakiness in TestGetPodByNode

Fixes #6458

micahbeeman-wf · 2021-10-08T16:23:07Z

@alexec Can you approve me for running workflows so I can fix this test?

micahbeeman-wf · 2021-10-08T16:37:08Z

--- FAIL: TestGetPodByNode (0.10s)
    operator_test.go:3553: 
        	Error Trace:	operator_test.go:3553
        	Error:      	Expected value not to be nil.
        	Test:       	TestGetPodByNode

micahbeeman-wf · 2021-10-11T21:29:27Z

Went back to the commit which added this test (cc701a1) and it passes consistently there. This test now fails consistently so something has regressed since this went in.

micahbeeman-wf · 2021-10-12T19:29:48Z

The test fails because it expects the cache to contain a pod called argo/dag-events-1004301846 but the cache actually contains argo/dag-events-whalesay-1004301846.

micahbeeman-wf · 2021-10-12T20:04:17Z

@alexec do you know of a recent change that would cause the template name (whalesay) to be included in the pod name?

alexec · 2021-10-12T20:41:41Z

@alexec do you know of a recent change that would cause the template name (whalesay) to be included in the pod name?

Yes. That changed recently. Test will need to be updated.

micahbeeman-wf · 2021-10-12T20:51:46Z

@alexec do you know of a recent change that would cause the template name (whalesay) to be included in the pod name?

Yes. That changed recently. Test will need to be updated.

What was the change?

alexec · 2021-10-12T21:04:06Z

Um. See Git history?

micahbeeman-wf · 2021-10-14T17:25:04Z

Looks like it was changed in #6712

micahbeeman-wf · 2021-10-14T17:50:30Z

Now that the test is running successfully it appears that it is flaky, failing about 3% of the time.

Signed-off-by: Micah Beeman <micah.beeman@workiva.com>

micahbeeman-wf · 2021-10-14T18:39:10Z

@alexec Do you have to approve each run of the tests?

Signed-off-by: Micah Beeman <micah.beeman@workiva.com>

micahbeeman-wf · 2021-10-14T20:36:15Z

@alexec I updated getPodByNode to support the changes made in #6712 and fixed the test flakiness, so this is ready to review. Can you allow my workflow to run?

kennytrytek

Looks good to me, assuming the tests pass. ⛄

codecov · 2021-10-15T14:25:50Z

Codecov Report

Merging #6897 (08c30e7) into master (c5b1533) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6897      +/-   ##
==========================================
+ Coverage   48.44%   48.49%   +0.05%     
==========================================
  Files         265      265              
  Lines       19259    19260       +1     
==========================================
+ Hits         9330     9341      +11     
+ Misses       8880     8871       -9     
+ Partials     1049     1048       -1

Impacted Files	Coverage Δ
workflow/controller/operator.go	`71.22% <100.00%> (+0.10%)`	⬆️
workflow/controller/controller.go	`22.59% <0.00%> (+0.28%)`	⬆️
workflow/controller/workflowpod.go	`74.41% <0.00%> (+0.51%)`	⬆️
cmd/argoexec/commands/emissary.go	`51.79% <0.00%> (+1.43%)`	⬆️

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 c5b1533...08c30e7. Read the comment docs.

micahbeeman-wf · 2021-10-15T14:44:46Z

@alexec I am not sure why the E2E test failed, seems unrelated to my changes?

alexec · 2021-10-15T14:54:37Z

That test is flakey.

alexec · 2021-10-15T14:55:18Z

workflow/controller/operator.go

 		return nil, fmt.Errorf("Expected node type %s, got %s", wfv1.NodeTypePod, node.Type)
 	}
-	return woc.controller.getPod(woc.wf.GetNamespace(), node.ID)
+	podName := wfutil.PodName(woc.wf.Name, node.Name, node.TemplateName, node.ID)


this is a bug fix, needs to be back-ported to v3.2

Signed-off-by: Micah Beeman <micah.beeman@workiva.com>

(argoproj#6897) Signed-off-by: Micah Beeman <micah.beeman@workiva.com> Signed-off-by: kriti-sc <kathuriakriti1@gmail.com>

micahbeeman-wf changed the title ~~Unskip TestGetPodByNode to test its flakiness~~ Update TestGetPodByNode, fixes #6458 Oct 14, 2021

micahbeeman-wf changed the title ~~Update TestGetPodByNode, fixes #6458~~ fix(controller): Update TestGetPodByNode, fixes #6458 Oct 14, 2021

micahbeeman-wf changed the title ~~fix(controller): Update TestGetPodByNode, fixes #6458~~ fix(controller): Update TestGetPodByNode. Fixes #6458 Oct 14, 2021

micahbeeman-wf added 3 commits October 14, 2021 13:33

Unskip TestGetPodByNode to test its flakiness

c7f9c0d

Signed-off-by: Micah Beeman <micah.beeman@workiva.com>

Set POD_NAMES to v1 to use nodeID for pod name

b8db8b0

Signed-off-by: Micah Beeman <micah.beeman@workiva.com>

Fix flakiness

d630bc0

Signed-off-by: Micah Beeman <micah.beeman@workiva.com>

micahbeeman-wf force-pushed the fix_TestGetPodByNode branch from 82e7702 to d630bc0 Compare October 14, 2021 18:34

Fix getPodByNode by using the correct pod name

08c30e7

Signed-off-by: Micah Beeman <micah.beeman@workiva.com>

micahbeeman-wf changed the title ~~fix(controller): Update TestGetPodByNode. Fixes #6458~~ fix(controller): Fix getPodByNode, TestGetPodByNode. Fixes #6458 Oct 14, 2021

micahbeeman-wf marked this pull request as ready for review October 14, 2021 20:02

kennytrytek approved these changes Oct 14, 2021

View reviewed changes

chrisgustavsen-wf approved these changes Oct 14, 2021

View reviewed changes

alexec approved these changes Oct 15, 2021

View reviewed changes

alexec reviewed Oct 15, 2021

View reviewed changes

alexec merged commit 41515d6 into argoproj:master Oct 15, 2021

This was referenced Oct 15, 2021

v3.2.1 cherry-pick #6951

Closed

v3.1.14 cherry-pick #6975

Closed

sarabala1979 pushed a commit that referenced this pull request Oct 19, 2021

fix(controller): Fix getPodByNode, TestGetPodByNode. Fixes #6458 (#6897)

d04aabf

Signed-off-by: Micah Beeman <micah.beeman@workiva.com>

alexec mentioned this pull request Nov 5, 2021

v3.2.4 cherry-pick #7171

Closed

25 tasks

sarabala1979 mentioned this pull request Dec 15, 2021

v3.2.5 Release #7414

Closed

73 tasks

sarabala1979 mentioned this pull request Mar 1, 2022

v3.2.9 Cherry-pick #8043

Closed

Conversation

micahbeeman-wf commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

micahbeeman-wf commented Oct 8, 2021

Uh oh!

micahbeeman-wf commented Oct 8, 2021

Uh oh!

micahbeeman-wf commented Oct 11, 2021

Uh oh!

micahbeeman-wf commented Oct 12, 2021

Uh oh!

micahbeeman-wf commented Oct 12, 2021

Uh oh!

alexec commented Oct 12, 2021

Uh oh!

micahbeeman-wf commented Oct 12, 2021

Uh oh!

alexec commented Oct 12, 2021

Uh oh!

micahbeeman-wf commented Oct 14, 2021

Uh oh!

micahbeeman-wf commented Oct 14, 2021

Uh oh!

micahbeeman-wf commented Oct 14, 2021

Uh oh!

micahbeeman-wf commented Oct 14, 2021

Uh oh!

kennytrytek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 15, 2021

Codecov Report

Uh oh!

micahbeeman-wf commented Oct 15, 2021

Uh oh!

alexec commented Oct 15, 2021

Uh oh!

alexec Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

micahbeeman-wf commented Oct 8, 2021 •

edited

Loading

kennytrytek left a comment •

edited

Loading