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

Batched dry-run? No thank you! #946

Merged
merged 6 commits into from
Jan 30, 2024

Conversation

timothysmith0609
Copy link
Contributor

What are you trying to accomplish with this PR?
During validation we attempt to send dry-run=server apply all resources, but there's some flaws in this approach:

  • Even if the batched apply fails, we just fall back to per-resource validations, anyway, so anything caught by server-side apply does not actually raise an error, anyway
  • We were not separating out applyable vs non-applyable resources (e.g. resources with generateName field). This would cause the dry-run to fail because the incoming resource can not be applied. This is easily fixed by simply only taking applyable resources, but the fact this bug has existed for so long and no one has fixed it speaks to the lack of value batch dry running is adding
  • More broadly, batched dry-run is at odds with how Krane breaks up a deploy into multiple stages. It does this because some resources are, in general, dependencies of other resources in the same apply-set. E.g. deploying a ConfigMap/Secret/ServiceAccount that is mounted by a Pod. Server-side dry run is not smart enough to resolve these dependencies when submitting a batch of resources, so this too will cause a failure. To me, this puts the multi-stage deploys of krane fundamentally at odds with the ability to get a reliably-passing batch dry-run and is the largest argument against continued support for batched dry-runs.

@jpfourny
Copy link
Contributor

❤️ for splitting test matrix by test suite.

@timothysmith0609 timothysmith0609 merged commit ed0b0f4 into main Jan 30, 2024
64 of 65 checks passed
@timothysmith0609 timothysmith0609 deleted the tsmith/batch-dry-run-not-in-my-krane branch January 30, 2024 20:18
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.

2 participants