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

compositor: minimal invasive name generation without dry-run #4858

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Oct 20, 2023

Description of your changes

Fixes #4712.

The old code used dry-run to make the apiserver generate a name for unnamed resources. But dry-run has side-effects, namely it validates. With that name generation could fail in the past and generate error messages containing an ever changing name, leading to continuous event generation.

This PR replaces the dry-run with an equivalent name generation that does not validate:

  1. use the Kube apiserver simple name generator, the same that is used by the apiserver itself
  2. verify with client.Get that the name is not taken. Try another name if it is, up to 10 times.

There are 8 million names (length 5, 27 different characters). The chance that a random name is taken with 10k objects is pretty low. With 10 tries, the chance is practically zero.

Note: this is a minimal invasive change. As before, a name can be taken when the composed resource is eventually created. Again, the chance is super low, even lower than before because only newly created objects can conflict. So it is far less than 10k objects that have the chance to conflict.

Peer PR to #4780, which removes dry-run in the claim controller with a similar attempt, but less minimal invasive, but also 100% correct in the sense that it handles all kind of conflicts gracefully.

I have:

  • Read and followed Crossplane's contribution process.
  • Added or updated unit tests.
  • Added or updated e2e tests, if necessary. (check with reviewers/maintainers if you're unsure whether E2E tests are necessary for the change).
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, if necessary.
  • Opened a PR updating the docs, if necessary.

Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@upbound.io>
@pedjak
Copy link
Contributor

pedjak commented Oct 20, 2023

LGTM

Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@negz negz merged commit 931e78d into crossplane:master Oct 20, 2023
16 checks passed
@avalanche123 avalanche123 mentioned this pull request Dec 12, 2023
3 tasks
@phisco
Copy link
Contributor

phisco commented Feb 9, 2024

Turns out we relied on the mentioned dry-run side-effect, both Compose functions assumed resources to be valid because of that dry-run. See #5292 and linked PR.

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.

Growing list of ComposeResources events when the schema of composed resource is not valid
4 participants