Skip to content
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

feat(controller): use CRD status subresource #789

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

jessesuen
Copy link
Member

@jessesuen jessesuen commented Oct 19, 2020

Resolves #771
Resolves #440

This change updates Argo Rollouts to enable the use of the CRD status subresource. The reason for doing so, is to allow the rollout controller to record the numeric metadata.generation into status.observedGeneration. This allows us to have a reliable indicator of a Rollout who's spec has (or has not yet) been observed by the controller (for example if the argo-rollouts controller was down). This is needed in situations when tooling needs to wait/block on a Rollout to become healthy, after making a spec change (e.g. Argo CD PostSync hook).

A huge consequence of this change, is that Rollouts can no longer update spec/metadata at the same time as status, since the latter requires a separate call to the rollout's /status endpoint. Most changes are in test expectations that currently expect updates on the resource and not resource/status.

The main code change is in rollout/sync.go, which splits the update of a rollout into two API calls, and promote.go which also needs to make two API calls during promotion.

This change also:

  • removes the deprecated status.canary.stableRS field.
  • reorganizes installation manifests so that there is only one role file to update

TODO:

  • make sure this works with rollouts with an invalid spec. (e.g. can we still persist rollouts that have invalid specs)

@jessesuen jessesuen force-pushed the status-subresource branch 5 times, most recently from b5ab88e to df9f844 Compare November 8, 2020 11:54
@codecov-io
Copy link

Codecov Report

Merging #789 (df9f844) into master (ac74a9b) will decrease coverage by 0.07%.
The diff coverage is 76.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #789      +/-   ##
==========================================
- Coverage   82.09%   82.01%   -0.08%     
==========================================
  Files          98       98              
  Lines        8523     8536      +13     
==========================================
+ Hits         6997     7001       +4     
- Misses       1085     1093       +8     
- Partials      441      442       +1     
Impacted Files Coverage Δ
rollout/canary.go 78.90% <ø> (-0.33%) ⬇️
rollout/sync.go 70.51% <53.33%> (-1.57%) ⬇️
pkg/kubectl-argo-rollouts/cmd/promote/promote.go 95.45% <88.00%> (-4.55%) ⬇️
pkg/kubectl-argo-rollouts/info/rollout_info.go 78.94% <90.90%> (+1.92%) ⬆️
pkg/kubectl-argo-rollouts/cmd/abort/abort.go 100.00% <100.00%> (ø)
pkg/kubectl-argo-rollouts/cmd/retry/retry.go 100.00% <100.00%> (ø)
rollout/controller.go 71.50% <100.00%> (-0.97%) ⬇️
utils/conditions/conditions.go 78.72% <100.00%> (-0.66%) ⬇️

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 ac74a9b...df9f844. Read the comment docs.

Signed-off-by: Jesse Suen <jesse_suen@intuit.com>
Comment on lines +140 to +142
if !isGenerationObserved(ro) {
return "Progressing", "waiting for rollout spec update to be observed"
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the check that allows us to detect if rollout controller observed the new spec.

@jessesuen jessesuen marked this pull request as ready for review November 8, 2020 12:28
@jessesuen jessesuen changed the title feat(controller): use status subresource feat(controller): use CRD status subresource Nov 9, 2020
Copy link
Contributor

@khhirani khhirani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jessesuen jessesuen merged commit f4358f8 into argoproj:master Nov 9, 2020
@jessesuen jessesuen deleted the status-subresource branch November 9, 2020 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Argo CD can fire PostSync hooks prematurely (before Rollout is Healthy) Remove .Status.Canary.StableRS
3 participants