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

Set last-applied-label during create or replace operation atomically #972

Merged
merged 4 commits into from
Jan 16, 2025

Conversation

timothysmith0609
Copy link
Contributor

@timothysmith0609 timothysmith0609 commented Jan 15, 2025

Replaces #971

For resources that are BOTH

  • Created via create and/or replace
  • Prunable

krane currently first issues the create/replace command, then subsequently calls apply. This is done to ensure ongoing cleanup of resources like DB-Migrate pods that would otherwise just hang around.

Unfortunately, this subsequent apply breaks if a mutating admission controller modified an otherwise immutable field. The document presented by krane now conflicts with the API server and the deploy fails.

Thankfully, kubectl supports adding the last-applied-configuration annotation (which is the signal used by k8s to marks resources as prunable). Using the exact same logic we were using to add individual resources to the applyables list, we can just conditionally set the --save-config flag on kubectl create/replace

EDIT: I had to bend over backwards a bit further than I first thought to get this working. The intent with this behaviour is to clean up created/replaced resources from the LAST deployment that has occurred. After fighting trying to get no-op server-side applies to work (the documentation does not match the behaviour), I've been forced to conditionally set last-applied-configuration annotations on created resources in the Deploy all Resources phase. This entails adding a conditional parameter for whether or not to annotate individual resources, since this code path is used by both the Predeploy Priority Resources and Deploy all Resources steps

This does introduce a subtle failure mode. If the apply_all succeeds but annotate_individuals fails, the individual resources will never be tagged and thus not subject to pruning on the subsequent deploy.

@timothysmith0609 timothysmith0609 self-assigned this Jan 15, 2025
@timothysmith0609 timothysmith0609 marked this pull request as ready for review January 15, 2025 19:00
Copy link
Contributor

@jpfourny jpfourny left a comment

Choose a reason for hiding this comment

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

Nice!

@timothysmith0609
Copy link
Contributor Author

This does introduce a subtle failure mode. If the apply_all succeeds but annotate_individuals fails, the individual resources will never be tagged and thus not subject to pruning on the subsequent deploy.

This issue actually already exists, though in a slightly narrower scope, today. If the apply fails, subsequent krane deploy of a different set of documents would also cause 'orphaned' (I'm using the term very loosely) resources.

@timothysmith0609 timothysmith0609 merged commit 57fc6ef into main Jan 16, 2025
97 checks passed
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.

3 participants