feat: add workload-ref/generation to rollout #1198

huikang · 2021-05-19T04:19:38Z

Add a "rollout.argoproj.io/workload-generation" annotation to
the rollout metadata, which equals to the generation of reference
workload
workload-generation is updated when the referenced workload is
updated.
status.workloadObservedGeneration records the observed generation
of the rollout
e2e test is updated

Signed-off-by: Hui Kang [email protected]

close #1189

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed my commits with DCO
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

codecov · 2021-05-19T04:42:52Z

Codecov Report

Merging #1198 (d803ca0) into master (460262e) will increase coverage by 0.01%.
The diff coverage is 85.93%.

@@            Coverage Diff             @@
##           master    #1198      +/-   ##
==========================================
+ Coverage   81.37%   81.38%   +0.01%     
==========================================
  Files         106      106              
  Lines        9721     9776      +55     
==========================================
+ Hits         7910     7956      +46     
- Misses       1273     1279       +6     
- Partials      538      541       +3

Impacted Files	Coverage Δ
rollout/temlateref.go	`83.67% <68.96%> (-4.52%)`	⬇️
rollout/sync.go	`76.42% <100.00%> (+0.32%)`	⬆️
utils/annotations/annotations.go	`97.02% <100.00%> (+0.68%)`	⬆️
utils/rollout/rolloututil.go	`88.23% <100.00%> (+1.79%)`	⬆️
rollout/controller.go	`78.35% <0.00%> (ø)`

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 460262e...d803ca0. Read the comment docs.

sonarqubecloud · 2021-05-20T16:08:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
17.0% Duplication

huikang · 2021-05-20T17:11:20Z

Hi, @jessesuen , this PR is ready for review. (I was trying to increase the test coverage, but still fall short of the required target. Please provide any advice if you have. Thanks.)

jessesuen · 2021-05-24T18:21:48Z

Thanks for working on this. Will be taking a look at this in the coming days.

jessesuen · 2021-05-26T00:23:08Z

@alexmt thinks there might be a better way to address the issue. I will ask him to put an alternative proposal in the issue.

huikang · 2021-05-26T01:58:13Z

@alexmt thinks there might be a better way to address the issue. I will ask him to put an alternative proposal in the issue.

Sounds good. Will follow up the thread.

jessesuen · 2021-06-10T09:11:08Z

@alexmt thinks there might be a better way to address the issue. I will ask him to put an alternative proposal in the issue.
Sounds good. Will follow up the thread.

I checked in with @alexmt about this again. He gave it some more thought but didn't have a better proposal. So we will go with the original improvement as discussed in the issue.

jessesuen

Change looks very good. Some minor points

jessesuen · 2021-07-01T00:46:55Z

rollout/temlateref.go

 		if key, err := cache.MetaNamespaceKeyFunc(ro); err == nil {
 			r.rolloutWorkQueue.Add(key)
 		}


There's actually no need to enqueue the rollout to r.rolloutWorkQueue anymore with this new approach because the act of annotating the rollout automatically gets the rollout into the workqueue. I think we should remove these three lines because we may be operating on the rollout prematurely (where the annotation is not reflected in the rollout), which could cause ConflictErrors during reconciliation.

Can we remove these lines?

if key, err := cache.MetaNamespaceKeyFunc(ro); err == nil { r.rolloutWorkQueue.Add(key) }

You are correct. Thanks for the explanation.

HI, @jessesuen , I tested remove the above 3 lines. However, the unit test failed at

argo-rollouts/rollout/temlateref_test.go

Lines 366 to 367 in 460262e

require.False(t, done)

assert.Equal(t, "default/my-rollout", item)

The Get() does not return any object. I think this is because the test code uses argoprojectclientset := fake.Clientset{}. Any advice?

I think test needs to be modified since we no longer update workqueue directly. So I think this part of test can be removed as well.

Maybe in place of this, we verify the rollout was annotated.

True. Will change the test to verify annotation is added.

In fact, can we remove the rolloutWorkQueue field entirely from NewInformerBasedWorkloadRefResolver?

Hi, @jessesuen , I updated the test to verifying the annotations are added for the rollout. However, it looks like the test clientset can't turn interface{} to rollout. Could you help to test a look?

https://github.com/argoproj/argo-rollouts/pull/1198/files#diff-52c4f59609dee33fdeab90f1c17716000fca7922b7524c4e718e114cc1e41afdR243

test/e2e/canary_test.go

- Add a "rollout.argoproj.io/workload-generation" annotation to the rollout metadata, which equals to the generation of reference workload - workload-generation is updated when the referenced workload is updated. - status.workloadObservedGeneration records the observed generation of the rollout - workloadref e2e test is update Signed-off-by: Hui Kang <[email protected]>

- fix proto number after rebase Signed-off-by: Hui Kang <[email protected]>

rollout/sync.go

huikang · 2021-07-02T05:16:36Z

rollout/temlateref.go

 	for _, ro := range rollouts {
-		if key, err := cache.MetaNamespaceKeyFunc(ro); err == nil {
-			r.rolloutWorkQueue.Add(key)
+		un, ok := ro.(*unstructured.Unstructured)


@jessesuen , in the e2e test, the ro can be mappted to *unstructured.Unstructured without any issue (so the e2e test passed). However, in the unit test code, the rollout can't be resolved. Do you have any idea about this? Thanks.

FYI, to reproduce: go test -timeout 999s -run ^TestRequeueReferencedRollouts$ ./rollout -v

…ange - doc update: migration.md - Remove the workqueue of informerBasedTemplateResolver Signed-off-by: Hui Kang <[email protected]>

jessesuen · 2021-07-02T21:44:31Z

utils/rollout/rolloututil.go

+func isWorkloadGenerationObserved(ro *v1alpha1.Rollout) bool {
+	if _, ok := annotations.GetWorkloadGenerationAnnotation(ro); !ok {
+		return true
+	}
+	workloadGeneration, _ := annotations.GetWorkloadGenerationAnnotation(ro)
+	observedWorkloadGen, err := strconv.Atoi(ro.Status.WorkloadObservedGeneration)
+	if err != nil {
+		return true
+	}
+
+	return int32(observedWorkloadGen) == workloadGeneration


The code scanner is catching a valid concern with the use of strconv.Atoi. We should use strconv.ParseInt(ro.Status.WorkloadObservedGeneration, 10, 32) instead

I just noticed that we may not handle the case properly where we still may have a workloadGenerationAnnotation on the rollout (leftover from when it switched from workload to non-workload ref) even if rollout switched to using inline template.

I think this function needs to check if ro.Spec.WorkloadRef != nil before proceeding. At the same time, I think we should remove the workload generation annotation from the rollout, since I don't think that gets cleaned up at the moment.

Thanks for the feedback. The PR is updated and an e2e test case is added to verify the workload generation annotation is removed after it switches to inline template.

- e2e test added Signed-off-by: Hui Kang <[email protected]>

Signed-off-by: Hui Kang <[email protected]>

sonarqubecloud · 2021-07-05T23:42:09Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

huikang marked this pull request as draft May 19, 2021 04:19

huikang force-pushed the fix-workloadRef-generation branch from 0e35cf2 to 34a6cb1 Compare May 19, 2021 04:34

huikang force-pushed the fix-workloadRef-generation branch 2 times, most recently from a0b9d1d to 2ebd1ae Compare May 19, 2021 15:40

huikang marked this pull request as ready for review May 19, 2021 15:40

huikang force-pushed the fix-workloadRef-generation branch 6 times, most recently from 5ad3f4c to df74692 Compare May 20, 2021 16:07

jessesuen reviewed Jul 1, 2021

View reviewed changes

Hui Kang added 2 commits June 30, 2021 23:21

Add client side check for observed workload generation

96473c8

- fix proto number after rebase Signed-off-by: Hui Kang <[email protected]>

huikang force-pushed the fix-workloadRef-generation branch 2 times, most recently from 8d7fcfd to 9d51b8e Compare July 1, 2021 03:50

jessesuen reviewed Jul 1, 2021

View reviewed changes

rollout/sync.go Show resolved Hide resolved

huikang force-pushed the fix-workloadRef-generation branch 3 times, most recently from d3a5abf to bb5ad39 Compare July 2, 2021 05:11

huikang commented Jul 2, 2021

View reviewed changes

huikang force-pushed the fix-workloadRef-generation branch 2 times, most recently from 8bd3ab6 to f2d24f4 Compare July 2, 2021 15:35

huikang force-pushed the fix-workloadRef-generation branch from f2d24f4 to e7ceecc Compare July 2, 2021 15:45

Change requeue rollout to updating its annotations on workload ref ch…

ab451dc

…ange - doc update: migration.md - Remove the workqueue of informerBasedTemplateResolver Signed-off-by: Hui Kang <[email protected]>

huikang force-pushed the fix-workloadRef-generation branch from e7ceecc to ab451dc Compare July 2, 2021 17:41

jessesuen reviewed Jul 2, 2021

View reviewed changes

huikang force-pushed the fix-workloadRef-generation branch 3 times, most recently from 10e2d89 to 990c469 Compare July 3, 2021 03:37

Remove workloadRef annotation after switch to inline template

9a202a6

- e2e test added Signed-off-by: Hui Kang <[email protected]>

huikang force-pushed the fix-workloadRef-generation branch 5 times, most recently from 48149d5 to adb1264 Compare July 5, 2021 17:53

update test code

d803ca0

Signed-off-by: Hui Kang <[email protected]>

huikang force-pushed the fix-workloadRef-generation branch from adb1264 to d803ca0 Compare July 5, 2021 23:41

jessesuen approved these changes Jul 7, 2021

View reviewed changes

jessesuen merged commit 2339ddc into argoproj:master Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add workload-ref/generation to rollout #1198

feat: add workload-ref/generation to rollout #1198

huikang commented May 19, 2021 •

edited

Loading

codecov bot commented May 19, 2021 •

edited

Loading

sonarqubecloud bot commented May 20, 2021

huikang commented May 20, 2021

jessesuen commented May 24, 2021

jessesuen commented May 26, 2021

huikang commented May 26, 2021

jessesuen commented Jun 10, 2021

jessesuen left a comment

jessesuen Jul 1, 2021

huikang Jul 1, 2021

huikang Jul 1, 2021 •

edited

Loading

jessesuen Jul 1, 2021

huikang Jul 1, 2021

jessesuen Jul 1, 2021

huikang Jul 2, 2021

huikang Jul 2, 2021 •

edited

Loading

huikang Jul 2, 2021

jessesuen Jul 2, 2021

huikang Jul 3, 2021

sonarqubecloud bot commented Jul 5, 2021

	require.False(t, done)
	assert.Equal(t, "default/my-rollout", item)

feat: add workload-ref/generation to rollout #1198

feat: add workload-ref/generation to rollout #1198

Conversation

huikang commented May 19, 2021 • edited Loading

codecov bot commented May 19, 2021 • edited Loading

Codecov Report

sonarqubecloud bot commented May 20, 2021

huikang commented May 20, 2021

jessesuen commented May 24, 2021

jessesuen commented May 26, 2021

huikang commented May 26, 2021

jessesuen commented Jun 10, 2021

jessesuen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huikang Jul 1, 2021 • edited Loading

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

huikang Jul 2, 2021 • edited Loading

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

sonarqubecloud bot commented Jul 5, 2021

huikang commented May 19, 2021 •

edited

Loading

codecov bot commented May 19, 2021 •

edited

Loading

huikang Jul 1, 2021 •

edited

Loading

huikang Jul 2, 2021 •

edited

Loading