fix: restore pod device usage in scheduler Filter on error paths (#1491) - #2044
fix: restore pod device usage in scheduler Filter on error paths (#1491)#2044pingxin403 wants to merge 10 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pingxin403 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe scheduler’s ChangesPod rollback fix in scheduler Filter
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Filter as Filter()
participant PodManager as podManager
participant QuotaManager as quotaManager
participant Util as util.PatchPodAnnotations
Filter->>PodManager: remove pod conditionally
Filter->>QuotaManager: remove usage conditionally
Filter->>Filter: getNodesUsage()
alt usage or scoring failure
Filter->>PodManager: restorePod()
Filter->>QuotaManager: restore usage
end
Filter->>Util: PatchPodAnnotations()
alt patch fails
Filter->>QuotaManager: revert tentative usage
Filter->>PodManager: restore original pod or delete new entry
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a pod leak issue (bug #1491) where a pod is permanently removed from the scheduler's cache if the Filter operation fails. It introduces a restorePod helper function to re-add the pod and its quota usage on failure paths, and adds a corresponding unit test. The review feedback highlights a critical bug in the error handling of util.PatchPodAnnotations: calling restorePod() alone does not clean up the newly added pod allocation and quota usage, leading to state mismatch and resource leaks. A code suggestion is provided to properly revert the new allocation before restoring the old state.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/scheduler/scheduler.go (1)
821-831: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRollback the tentative pod allocation before restoring the old entry.
restorePod()only re-addspi.Devices; afterAddPod(args.Pod, m.NodeID, m.Devices)andAddUsage(args.Pod, m.Devices), a patch failure leaves the new quota charge in place and the cached pod entry onm.NodeID. Undo the tentative allocation first, then callrestorePod().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/scheduler/scheduler.go` around lines 821 - 831, The patch-failure rollback in scheduler.go is incomplete because `restorePod()` only restores `pi.Devices` and does not undo the tentative allocation made by `podManager.AddPod` and `quotaManager.AddUsage` in the scheduling flow. In the `AddPod(args.Pod, m.NodeID, m.Devices)` block, first roll back the newly added pod and quota usage from `podManager` and `quotaManager`, then call `restorePod()` so the old entry is restored after the tentative allocation is fully reverted.
🧹 Nitpick comments (1)
pkg/scheduler/scheduler_test.go (1)
1503-1510: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also asserting quota state and covering the patch-failure path.
The test only verifies the pod count/presence in
podManageron the empty-score branch. Adding an assertion onquotaManager.GetResourceQuota()(asTest_Filter_EvictsStaleEntrydoes) would guard against quota drift, and a case that fails atPatchPodAnnotationswould cover the other rollback branch — which is currently untested and where the quota double-count noted inscheduler.gooccurs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/scheduler/scheduler_test.go` around lines 1503 - 1510, The current test only checks podManager recovery on the empty-score path in the Filter flow; extend it to also assert quota state via quotaManager.GetResourceQuota, matching the existing stale-entry coverage. Add a separate test case around the scheduler Filter path that forces PatchPodAnnotations to fail so the rollback branch is exercised, and verify both pod re-addition and quota consistency for the affected podA/scheduler.go logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/scheduler/scheduler.go`:
- Around line 821-831: The patch-failure rollback in scheduler.go is incomplete
because `restorePod()` only restores `pi.Devices` and does not undo the
tentative allocation made by `podManager.AddPod` and `quotaManager.AddUsage` in
the scheduling flow. In the `AddPod(args.Pod, m.NodeID, m.Devices)` block, first
roll back the newly added pod and quota usage from `podManager` and
`quotaManager`, then call `restorePod()` so the old entry is restored after the
tentative allocation is fully reverted.
---
Nitpick comments:
In `@pkg/scheduler/scheduler_test.go`:
- Around line 1503-1510: The current test only checks podManager recovery on the
empty-score path in the Filter flow; extend it to also assert quota state via
quotaManager.GetResourceQuota, matching the existing stale-entry coverage. Add a
separate test case around the scheduler Filter path that forces
PatchPodAnnotations to fail so the rollback branch is exercised, and verify both
pod re-addition and quota consistency for the affected podA/scheduler.go logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aa8a36cf-742c-4f83-84d4-ea8e4e88f60d
📒 Files selected for processing (2)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
- Fix nodeID not restored in AddPod else branch (mesutoezdil) - Fix orphaned pod entry when removed=false on PatchPodAnnotations failure (mesutoezdil) - Add test coverage for PatchPodAnnotations failure path (mesutoezdil) - All existing Filter tests pass
- Fix nodeID not restored in AddPod else branch (mesutoezdil) - Fix orphaned pod entry when removed=false on PatchPodAnnotations failure (mesutoezdil) - Add test coverage for PatchPodAnnotations failure path (mesutoezdil) - All existing Filter tests pass Signed-off-by: pingxin403 <pingxin403@163.com>
5096de7 to
fa4e6eb
Compare
…ject-HAMi#1491) Signed-off-by: pingxin403 <pingxin403@163.com>
Signed-off-by: pingxin403 <pingxin403@163.com>
…ns failure path (Project-HAMi#1491) Signed-off-by: pingxin403 <pingxin403@163.com>
- Fix nodeID not restored in AddPod else branch (mesutoezdil) - Fix orphaned pod entry when removed=false on PatchPodAnnotations failure (mesutoezdil) - Add test coverage for PatchPodAnnotations failure path (mesutoezdil) - All existing Filter tests pass Signed-off-by: pingxin403 <pingxin403@163.com>
fa4e6eb to
e75cb89
Compare
|
if you wouldn't mind doing merge commits instead of force pushing, that would make it a bit clearer what (if anything has changed). |
I'm a bit confused here: how is it guaranteed that the retry will be scheduled to the same card? |
Signed-off-by: yunpenghan <yunpenghan@futunn.com>
|
Closing this PR. The issue does not meet the contribution guidelines and the fix is no longer needed. Thanks for the reviews. |
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes a GPU accounting leak in the scheduler Filter path. When
TakeAndDeletePodsuccessfully removes a pod frompodManagerbut a subsequent step fails (getNodesUsage,calcScore, no candidate nodes, orPatchPodAnnotations), the pod is permanently removed from the cache without restoring its device/quota state. A subsequent retry of the same pod may land on a different card, or the quota reservation is silently lost.Which issue(s) this PR fixes:
Fixes #1491
Special notes for your reviewer:
The fix introduces a
restorePodclosure that re-adds the pod topodManagerand restores its quota usage on the four error paths afterTakeAndDeletePodsucceeds.A regression test
Test_Filter_PodLeakOnFailurevalidates that the pod remains inpodManagerafter a failed Filter attempt on a fully occupied node.AI Assistance Disclosure:
This contribution was created with the assistance of AI tools (OpenCode/Anthropic Claude). The code logic, testing strategy, and final implementation were reviewed and verified by a human contributor.
Summary by CodeRabbit