Add an opt-in pod-template-generation label for TopologySpreadConstraints.matchLabelKeys - #2485
Add an opt-in pod-template-generation label for TopologySpreadConstraints.matchLabelKeys#2485Donouyi wants to merge 13 commits into
Conversation
johscheuer
left a comment
johscheuer
left a comment
There was a problem hiding this comment.
Hi 👋 Thanks for the PR, it's on my list for this week.
johscheuer
left a comment
johscheuer
left a comment
There was a problem hiding this comment.
In general I think those changes look fine a few general comments:
- It would be great if you could add some additional documentation for this feature in our
docssection, e.g. about the use case and how to use it (with an example). - Right now, based on the way hoe it is implemented it will not have am affect if a user enabled this setting until a pod will be deleted for another unrelated reason. This could be confusing and could lead to a period where some pods have the new label and some others are missing it.
- We have an e2e test suite, do you think you are able to add a new e2e test for this new feature?
Sorry for the delay and thanks again for creating the PR!
| // PodMetadataCorrect call. | ||
| if cluster.ShouldIncludePodTemplateGenerationLabel() { | ||
| hash, hashErr := GetPodGenerationHash(cluster, processGroup.ProcessClass) | ||
| if hashErr != nil { |
There was a problem hiding this comment.
Is there a reason we are not returning the error? It could be hard to debug if the operator only logs the error in cases where it was not able to compute the hash.
There was a problem hiding this comment.
you are right, changed the behaviour.
| // podMetadataCorrect, so there's no need to recompute it on every | ||
| // PodMetadataCorrect call. | ||
| if cluster.ShouldIncludePodTemplateGenerationLabel() { | ||
| hash, hashErr := GetPodGenerationHash(cluster, processGroup.ProcessClass) |
There was a problem hiding this comment.
I think this should be fine, but we will compute the same hash multiple times if we are adding multiple pods for the same process class, e.g. if we replace multiple pods, during the cluster creation or when we scale up the cluster.
There was a problem hiding this comment.
yes, I think given this path only runs in addPods and never in steady-state reconciliation it's quite cheap to keep it as is.
|
@johscheuer, in the last commit, I've added the implementation of a marked-for-removal sentinel that we found useful to protect against the case of a pod that is (re)created while its process group is already marked for removal. Without it, the pod would be stamped with the current live generation hash so the scheduler counts a doomed pod as part of the new generation and can skew its spread. Because topology spread is only evaluated at schedule time, that skew isn't corrected once the doomed pod is deleted. Also added a few log fields. They made the behaviour much easier to trace while testing but happy to drop them if you consider them noise. |
2 participants
Description
This PR adds an opt-in feature that has the operator stamp a per-generation content hash on every pod it creates, under the label key
foundationdb.org/pod-template-generation. The value is the first 16 hex characters of a SHA-256 over the canonical renderedPodSpecfor the pod's process class (GetPodSpecinvoked with a sentinel ProcessGroupID per class, so all real pods of the same class produce the same hash). Anything that flows into pod construction automatically contributes to the hash.The label is meant to be referenced from
TopologySpreadConstraints.matchLabelKeyson the user's pod template, so the kube-scheduler can scope per-pod spread filtering to "pods built from the same generation" instead of comparing against the entire cluster.The PR also extends
podMetadataCorrectwith a small carve-out so the label is preserved on running pods and only rotates when a pod is actually recreated — mirroring the existing carve-out for theLastSpecKeyannotation.Emission is gated by a new field
LabelConfig.IncludePodTemplateGenerationLabel(default false). Existing clusters are unaffected unless they opt in.Motivation
TopologySpreadConstraints.matchLabelKeyslets a pod's spread filter be scoped to other pods that share specific label values with it. The intended use is rolling replacements: while old-generation pods are being replaced, the scheduler should be allowed to schedule the new-generation pods independently — otherwise the existing-pods inflate the topological zone count and lead to skew across zonesReplaces Donouyi#1 as a more complete solution. Fixes issue here
Type of change
Please select one of the options below.
Discussion
Are there any design details that you would like to discuss further?
Testing
Please describe the tests that you ran to verify your changes. Unit tests?
Manual testing?
Do we need to perform additional testing once this is merged, or perform in a larger testing environment?
Documentation
Did you update relevant documentation within this repository?
If this change is adding new functionality, do we need to describe it in our user manual?
If this change is adding or removing subreconcilers, have we updated the core technical design doc to reflect that?
If this change is adding new safety checks or new potential failure modes, have we documented and how to debug potential issues?
Follow-up
Are there any follow-up issues that we should pursue in the future?
Does this introduce new defaults that we should re-evaluate in the future?