DRA: Pair a requeued Workload with quota charges computed for its current generation - #14562
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @Anjali-Chauhan1. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Anjali-Chauhan1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe queue now retains DRA workload updates received during inflight scheduling. Requeue logic selects total requests that match the workload generation, while workload rebuilding adopts and clones externally computed requests. Unit and integration tests cover lifecycle cleanup, generation handling, and DRA admission. ChangesDRA requeue consistency
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR addresses stale DRA quota-charge pairing without any identified current-head merge-blocking risk; it is merge-ready after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Anjali-Chauhan1 <anjalichauhan1036@gmail.com>
Signed-off-by: Anjali-Chauhan1 <anjalichauhan1036@gmail.com>
Signed-off-by: Anjali-Chauhan1 <anjalichauhan1036@gmail.com>
Signed-off-by: Anjali-Chauhan1 <anjalichauhan1036@gmail.com>
1833558 to
70b5cbc
Compare
|
/ok-to-test |
What type of PR is this?
/kind bug
/area dra
What this PR does / why we need it:
When a DRA-backed Workload is updated while inflight in the scheduler, it can be requeued with its newest object paired with quota charges preprocessed for an older generation.
PushOrUpdatedrops updates for the workload the scheduler is processing, assuming requeue re-reads the object anyway. That holds for requests rebuilt from the object, but not for DRA: those charges are resolved by the workload controller and theworkload.Infois the only place they exist, so the dropped Info is the only copy pairing the newer object with charges computed for it.RequeueWorkloadthen fetches the newest object but keeps the charges it already holds, and the scheduling-equivalence hash is derived from that mix — one cycle of over- or under-admission, plus a hash describing neither shape.This PR sources the charges from a computation made for the generation being requeued:
PushOrUpdatecaptures the Info it would have dropped while a workload is inflight. It is set and cleared together withinflight, so no later cycle can be charged for a workload shape nobody asked for. The capture is only taken whileKueueDRAIntegrationis enabled; with the gate off, the path is unchanged.RequeueWorkloadprefers that capture when its generation matches the object; otherwise, it preserves its own charges while the object has not moved on.SchedulingHashUnknown, so the mismatch cannot bulk-move unrelated workloads.Rebuilding from the object is deliberately not an option: the spec carries
resourceClaims, not the quota keys they translate to, so a rebuild would requeue the workload asking for almost nothing. Checking the capture before falling back to preservation also covers footprint changes that don't bump the generation, such as a LimitRange default surfacing or a DeviceClass mutating while inflight.Which issue(s) this PR fixes:
Fixes #14535
Open design question. Case 3 is mitigated, not eliminated: the workload is requeued with one-generation-stale charges and no hash, and self-heals when the reconcile for the current generation re-pushes it. Closing the window fully would mean either firing
draReconcileChannelfrom the requeue path (needsRequeueWorkloadrestructured to send outsidem.Lock(), asaddLocalQueueLockedalready does) or requeuing toinadmissibleWorkloadsunder a new requeue reason. Both are left out to keep the diff reviewable — happy to follow up with either.Re-running DRA preprocessing at requeue was rejected: it needs the client, ResourceSlice cache and DeviceClass mapper, and
RequeueWorkloadholdsm.Lock().Distinct from #13930 and #14035, which are about the reconciler calling
AddOrUpdateWorkload; builds on the preserve-path added in #11927.Testing. Unit coverage for the new
WithTotalRequestsFromoption (including that adopted requests are deep-copied), the capture lifecycle, and all three cases through the realManager.RequeueWorkload— reverting the fix fails 3 of those 4 cases. The integration test covers the requeue branch but not the capture path: there is no hook to hold a workload inflight deterministically in envtest, and a timing-based spec would just become a flake. Commits are split so each builds and passes on its own.Does this PR introduce a user-facing change?
Summary by CodeRabbit
Bug Fixes
Improvements