Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
📖 Label Sync Between MachineDeployment and underlying Kubernetes Nodes #6255
📖 Label Sync Between MachineDeployment and underlying Kubernetes Nodes #6255
Changes from all commits
5f96f5a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to propagate all labels inline. Otherwise we will end up in a weird state where changes to some labels trigger a MD/MS rollout while others are in-place updated (should be simpler to implement as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, that said this is a breaking behavioral change which needs proper release documentation and a new minor release; in addition, changes to the logic of MachineDeployment might be extensive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah which is why I favored a separate field ... but yeah that decision has been made now :)
Just to confirm, we're fine with breaking behavior changes of our API fields as long as we do them in new minor releases? (my impression was that this requires a new apiVersion)
P.S. re: MD changes. The real fun part is probably to stop considering labels for the hash we're using while not triggering a MD rollout when the new CAPI version with this behavior is rolled out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API fields wouldn't need to change, but the propagation behavior of those fields would change, which is something that needs to be documented.
The same problem applies if we add a new field to the Machine template, we'd need a way to make sure that the generated hash is backward compatible. If that's easier though, let's just add a new field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, good to know that we don't consider this a breaking change :)
I think if it's a separate field we just never have to include it in the hash and we don't have to figure out how to remove something from the hash calculation without rollouts. Probably?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between the two solutions proposed above, the second one seems the most sane from a user perspective. If we also think that in the fullness of time we'd want to expand the in place node fields, this might be a good start.
That said, the biggest issue I see with the above is that we're relying on a hashing mechanism that's flaky and fragile. Have we considered breaking the hashing first before introducing new changes that might affect future rollouts and extensibility points?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I don't think we went that far regarding thinking about how to implement this proposal. But I think we should try to get to a consensus relatively soon, we're going back and forth on the field discussion since a while.
Maybe the following is a path forward?
Here it was basically a 50:50 decision (📖 Label Sync Between MachineDeployment and underlying Kubernetes Nodes #6255 (comment)), but sounds like now we prefer a separate field
I think the feature already would have a big benefit after 2., even without in-place upgrades (as it at least allows to propagate labels to nodes at all, which isn't possible right now).
Then we can continue with 3. and doing POC's and figuring out what the ideal way is to implement in-place upgrades.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn :/ Forgot that adding the field will already lead to a rollout on CAPI upgrade if we don't change the hashing.
So we definitely have to figure out how to do it for 2. already.
But If the general consensus is that we prefer the new field anyway (and we have the hash problem in both cases) we can at least move the proposal forward and then have to experiment on the implementation how to actually do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbueringer Thanks for pointing out the issue!
Introducing a new field is fine by me! I don't believe we have any objections, besides maybe @fabriziopandini :)
Should we also consider replacing the Deepcopy with something that includes specific fields (i.e. excludes our new field).
Also, if I'm not mistaken, wouldn't introducing a new field always cause a different hash to be generated, even if that new field is nil. So, when people upgrade, there will always be a rollout? Is this behavior considered okay for a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as long as we are using a changed struct the hash will change. I think we have a few options to solve this:
There might be other alternatives as well. But for me personally it would be fine to experiment during implementation. I think by making it a separate field we have a few advantages, e.g.:
Up until now we tried to avoid that at all costs (we even have an e2e test which validates it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have 3 scenarios (or something similar)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming key:effect
(same for other occurrences)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please search/replace across the whole doc (also "propagation")