Repository navigation
Automated cherry pick of #15641: Use the Deployment UID for the job-uid label on Deployment Workloads - #16005
Merged
kubernetes-prow[bot] merged 2 commits intoSep 22, 2026
Conversation
…ubernetes-sigs#15641) * Use the Deployment UID for the job-uid label on Deployment Workloads A Deployment produces one Workload per Pod, each labelled with its own Pod UID, so nothing ties the Workloads of a single Deployment together. Topology spreading therefore needs workloadLabelSelectors to group them. Changing the label value is a semantic change for anyone already selecting Workloads by job-uid, so it is gated behind DeploymentJobUIDLabel. * Read the owning ReplicaSet as metadata and tolerate its absence Fetching the typed ReplicaSet would start a cluster-wide informer caching every ReplicaSet pod template just to read a UID; the owner traversal in jobframework already reads unmanaged owners as metadata for that reason. Treating a missing ReplicaSet as an error left an orphaned Pod gated forever with the gate on, where the gate off produced a working Workload. A Pod that does not resolve to a Deployment keeps its own UID instead. * Cover the rolling update and gate-disabled paths A Deployment keeps an outgoing and an incoming ReplicaSet alive at the same time, and both sets of Pods have to land on the same job-uid for the label to be usable as a grouping key. The gate is meant to be inert when disabled, which is stronger than the resulting label being unchanged: no ReplicaSet may be read at all. * Explain why a missing ReplicaSet falls back instead of retrying The branch reads as a swallowed error without the two facts that justify it: a live Deployment's Pod always has its ReplicaSet, and the label is written once at Workload creation and never re-synced, so retrying could not converge anyway. * Soften the ReplicaSet-presence claim in the fallback comment The comment asserted that the ReplicaSet is always there, one line above the branch that handles it not being there. * Scope the job-uid documentation to resolvable Deployments Both the feature-gate comment and the label reference read as though enabling the gate always yields the Deployment UID. A Pod that carries its own queue-name under an unmanaged Deployment, or whose ReplicaSet is gone, keeps the Pod UID. * Identify Deployment Pods by the parent that carried the queue-name The suspended-by-parent annotation names the user-owned object the queue-name was placed on, so that value is what decides whether the Deployment UID applies. Matching on the annotation merely being present inferred the same thing from the shape of the ownership chain, which is a weaker statement about the same fact. The framework name moves to its own package because the pod integration cannot import the deployment one: the latter's tests reach back into the pod package through test/util, so a direct import is a cycle. * Graduate DeploymentJobUIDLabel to Beta in 0.21 Sharing one job-uid across a parent's Workloads is what LeaderWorkerSet already does, so the per-Pod value is the inconsistency rather than the new behaviour. * Resolve the owning Deployment through the shared ancestor walk FindAncestorJobManagedByKueue already traverses Pod -> ReplicaSet -> Deployment, reads the interim ReplicaSet as metadata, and yields an ancestor only when Kueue manages it and it carries the queue-name. That last condition is the property the label is meant to name, so asserting the returned type beats inferring the same thing from an annotation value, and the framework-name constant no longer needs a package of its own. The walk also surfaces a failed owner read instead of falling back. A cached NotFound is not proof that the ReplicaSet is gone -- the Pod and ReplicaSet informers advance independently -- and because the label is never re-synced, one stale read would have pinned the Pod UID on that Workload for good. * Cover the unqueued-ancestor paths and build fixtures from the wrappers Threading manageJobsWithoutQueueName into the walk decides whether a Deployment without a queue-name counts as the owner, and nothing exercised either answer. The Deployment and StatefulSet fixtures now come from the testingjobs wrappers, as the ancestor-walk test in jobframework already does, and the rolling update is two more rows of the table rather than a second copy of the setup. * Hold the feature gate at Alpha Graduating it belongs with the release that ships integration coverage, not with its introduction. * Exercise the Deployment job-uid label against a live API server The unit tests register a stand-in Deployment integration because the pod package cannot import the real one, so nothing showed that the registration Kueue actually ships resolves through the ancestor walk, or that the annotation the Deployment webhook writes is the one the pre-filter reads. The envtest suite runs no Deployment controller, so the ownership chain is built by hand; that is also what keeps the test deterministic. * Update pkg/controller/jobs/pod/pod_controller.go Co-authored-by: Mykhailo Bobrovskyi <mikhail.bobrovsky@gmail.com> * Restore the early return when the suspending parent is not a Deployment The applied suggestion replaced the guard in place, which left it returning early for exactly the Pods the label is meant for. The value is compared rather than merely tested for presence because the annotation is documented as free-form and users set it themselves for external controllers, so its presence says nothing about a Deployment being the parent. * Tighten the comment on the suspending-parent check The two reasons a reader needs are why the value is compared and why it is a literal; the rest was restating the code. * Share the Deployment framework name with the pod integration The pod integration matched the suspending parent against its own copy of the string, so the value the Deployment webhook writes and the value read back could drift apart. A constants package with no Kueue imports lets both sides name the same constant, which a direct import cannot: the deployment package's tests reach back into the pod package through test/util. * Trim the constants package comment The cycle is the reason the constant lives apart; which test import closes it is not something a reader needs to carry. * Cover the queue-name sitting on the Pod rather than the Deployment A Pod can carry the queue-name itself under a Deployment Kueue does not manage, and that Pod is managed standalone, so its Workload has to keep the Pod UID. The previous case put the queue-name on neither object, which is a Pod Kueue would not manage at all. * Build the integration fixtures from the wrappers and drop a dead remap The Deployment in the integration test was assembled by hand while the unit test already used the wrapper, and the expected-UID remap never fired because every case that wants the Pod UID uses a Pod whose UID is that literal. * Shape the unmanaged-Deployment case the way production produces it A Pod carrying the suspending-parent annotation under a Deployment without a queue-name cannot occur: the webhook stamps that annotation only when it manages the Deployment. The realistic shape is the Pod holding the queue-name itself with nothing stamped on it at all. The label reference now also says that enabling the gate leaves existing Workloads alone, which the write-once behaviour otherwise hides. * Update pkg/features/kube_features.go Co-authored-by: Michał Woźniak <mimowo@users.noreply.github.com> * Take feature gates as a map in the job-uid test and inline its assertion A map lets a case set whatever gates it needs, which a single bool cannot, and the gate now defaults to on, so every case states its own rather than relying on the default. Regenerates the feature-gate lists, which the in-place gate edit left behind. --------- Co-authored-by: Mykhailo Bobrovskyi <mikhail.bobrovsky@gmail.com> Co-authored-by: Michał Woźniak <mimowo@users.noreply.github.com>
kubernetes-prow
Bot
added
release-note
kubernetes-prow
Bot
added
the
kind/bug
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: kubernetes-sigs/kueue/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
Hi @rjgoyln. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
needs-ok-to-test
rjgoyln
marked this pull request as ready for review
September 22, 2026 14:00
kubernetes-prow
Bot
removed
the
do-not-merge/work-in-progress
This branch's PodWrapper derives an owner reference's UID from the name, so the ReplicaSet in that case was never the one the Pod pointed at and the walk stopped on the UID check, which the next case already covers. The parent registration also carries its job type in the table rather than deriving it again, and the integration setup drops the custom-labels wiring this branch's own suite does not use.
Contributor
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
Contributor
|
/lgtm |
kubernetes-prow
Bot
added
the
lgtm
|
LGTM label has been added. DetailsGit tree hash: b30b5fe9f4820e622660fc8bbaaaf97cbbb2baef |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mimowo, rjgoyln The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
kubernetes-prow
Bot
merged commit Sep 22, 2026
acbf5d2
into
kubernetes-sigs:release-0.18
58 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Cherry pick of #15641 on release-0.18.
#15641: Use the Deployment UID for the job-uid label on Deployment Workloads
For details on the cherry pick process, see the cherry pick requests page.
What type of PR is this?
/kind bug
Useful notes for your reviewer
DeploymentJobUIDLabelis Alpha and off by default here, not the Beta it is on main.IntegrationManager.