fix(scheduler): serialize Filter device selection to prevent double GPU allocation - #2470
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yxxhero 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 now serializes live ChangesConcurrent allocation serialization
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
59dc10d to
7ca3560
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/scheduler/scheduler_test.go (2)
1454-1462: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStop the informer factory when the test ends.
The test starts the informer factory at Line 1461 but never closes
s.stopCh. The informer goroutines keep running for the rest of the package test run. Leaked goroutines make-racereports harder to attribute, because a failure can surface during an unrelated later test.Add the channel close to the cleanup. Register it before the factory starts so cleanup order stays correct.
♻️ Proposed cleanup for the informer goroutines
client.KubeClient = fake.NewClientset() t.Cleanup(func() { client.KubeClient = nil }) s := NewScheduler() + t.Cleanup(func() { close(s.stopCh) }) s.kubeClient = client.KubeClient informerFactory := informers.NewSharedInformerFactoryWithOptions(client.KubeClient, time.Hour)Based on learnings from the coding guidelines: "Unit tests should be runnable with the race detector and repository test conventions, such as
go test ... -short --race -count=1."🤖 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 1454 - 1462, Update the test setup around NewScheduler and informerFactory.Start to register cleanup that closes s.stopCh before starting the informer factory, alongside the existing KubeClient cleanup. Preserve the current cleanup behavior while ensuring informer goroutines stop when the test ends.Source: Coding guidelines
1521-1530: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the
Filtererrors instead of discarding them.Line 1526 discards the error. If
Filterfails for every pod, the test still fails, but it fails at Line 1540 with "has no allocated device". That message hides the real cause. Collect the errors per index and assert them afterwg.Wait(), so a setup failure is distinguishable from a duplicate allocation.♻️ Proposed error capture
start := make(chan struct{}) var wg sync.WaitGroup + filterErrs := make([]error, numPods) for i := range numPods { wg.Add(1) - go func(pod *corev1.Pod) { + go func(idx int, pod *corev1.Pod) { defer wg.Done() <-start - _, _ = s.Filter(extenderv1.ExtenderArgs{Pod: pod, NodeNames: &nodeNames}) - }(pods[i]) + _, filterErrs[idx] = s.Filter(extenderv1.ExtenderArgs{Pod: pod, NodeNames: &nodeNames}) + }(i, pods[i]) } close(start) wg.Wait() + for i, err := range filterErrs { + require.NoError(t, err, "Filter failed for pod-%d", i) + }Each goroutine writes a distinct slice index, so this stays race-free under
-race.🤖 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 1521 - 1530, Update the concurrent Filter test around s.Filter to retain each goroutine’s error in a per-pod indexed collection instead of discarding it. After wg.Wait(), assert the collected errors before checking device allocation results, preserving distinct error reporting and race-free writes.
🤖 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.
Inline comments:
In `@pkg/scheduler/config/config.go`:
- Around line 282-284: Make InitDevicesWithConfig atomic by collecting device
registrations in local maps or slices while running all initializers, then
assign device.DevicesMap and device.DevicesToHandle only after every initializer
succeeds. On any initializer error, leave both globals empty or restore their
prior state so InitDevices and InitDefaultDevices can retry without exposing
partial registrations.
In `@pkg/scheduler/scheduler.go`:
- Around line 923-930: The live Filter path currently holds filterLock while
calling util.PatchPodAnnotations, serializing concurrent requests across the
apiserver round trip. Restructure the flow around Filter and filterSimulation so
only non-blocking cache reservation/rollback mutations occur under filterLock;
release the lock before PatchPodAnnotations, then perform annotation patching
and any required rollback without reacquiring the global lock during the network
call. Preserve the existing atomic reservation behavior and simulation path.
---
Nitpick comments:
In `@pkg/scheduler/scheduler_test.go`:
- Around line 1454-1462: Update the test setup around NewScheduler and
informerFactory.Start to register cleanup that closes s.stopCh before starting
the informer factory, alongside the existing KubeClient cleanup. Preserve the
current cleanup behavior while ensuring informer goroutines stop when the test
ends.
- Around line 1521-1530: Update the concurrent Filter test around s.Filter to
retain each goroutine’s error in a per-pod indexed collection instead of
discarding it. After wg.Wait(), assert the collected errors before checking
device allocation results, preserving distinct error reporting and race-free
writes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 026da7d9-8cef-446b-b606-150d9cb6d429
📒 Files selected for processing (6)
cmd/scheduler/main.gopkg/scheduler/config/config.gopkg/scheduler/config/config_test.gopkg/scheduler/nodes_test.gopkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
| if len(device.DevicesMap) > 0 { | ||
| klog.Info("Devices are already initialized, skipping initialization") | ||
| return | ||
| return nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make device initialization atomic on failure.
InitDevicesWithConfig populates device.DevicesMap and device.DevicesToHandle while it iterates. If a later initializer fails, it returns an error but leaves earlier registrations in both globals. InitDevices then sees a non-empty device.DevicesMap and skips a retry. InitDefaultDevices also returns while exposing the partial registry. Build the registrations locally and assign both globals only after all initializers succeed, or clear both globals before returning the error.
Also applies to: 292-295, 461-464
🤖 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/config/config.go` around lines 282 - 284, Make
InitDevicesWithConfig atomic by collecting device registrations in local maps or
slices while running all initializers, then assign device.DevicesMap and
device.DevicesToHandle only after every initializer succeeds. On any initializer
error, leave both globals empty or restore their prior state so InitDevices and
InitDefaultDevices can retry without exposing partial registrations.
There was a problem hiding this comment.
Out of scope for this PR — pkg/scheduler/config/config.go is not touched here. Happy to open a separate PR for the InitDevicesWithConfig partial-registration cleanup if the maintainers want it.
There was a problem hiding this comment.
@yxxhero, acknowledged. This does not block the scheduler allocation race fix in #2470.
Would you like me to create a follow-up GitHub issue for atomic InitDevicesWithConfig registration in pkg/scheduler/config/config.go?
You are interacting with an AI system.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
91785b2 to
99a8c12
Compare
|
@yxxhero please read the contribution.md before raising a pr https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md and please disclose if you use any AI Assistance . |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pkg/scheduler/scheduler.go`:
- Around line 983-991: The Filter flow must serialize requests for the same pod
UID across reservation selection, PatchPodAnnotations, and failure rollback; add
or reuse a per-pod UID lock or reservation token around these operations. Keep
filterLock scoped only to global device selection so different pods can continue
patching concurrently, and ensure rollback removes only the reservation owned by
the failing request.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7f11e24-9c17-4d6f-bf27-58e16fd2e4fe
📒 Files selected for processing (1)
pkg/scheduler/scheduler.go
| lock sync.RWMutex | ||
| synced bool | ||
|
|
||
| // filterLock serializes the device-selection critical section in Filter |
There was a problem hiding this comment.
Please write short comments . otherwise the comments will be more than the code. and please read https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md before raising the pr.
There was a problem hiding this comment.
Done — trimmed to one line:
// filterLock serializes device selection in Filter so concurrent requests cannot reserve the same device (issue #2232).
| "pod", klog.KObj(args.Pod), | ||
| "reason", "request does not contain full nodes", | ||
| "nodeNamesLen", nodeNamesLen(args.NodeNames)) | ||
| // Device selection and cache commit are serialized inside a helper that |
There was a problem hiding this comment.
Done here too — the call-site and selectAndCommitDevice comments are now 1–2 lines each.
…PU allocation Under concurrent pod creation (e.g. a multi-replica Deployment), two Filter extender calls can run in separate goroutines and both observe the same free GPU before either commits its reservation to the pod cache. Both pods then get the same card id annotated, so the card id in the pod annotation no longer matches the card the pod actually uses: the shared card gets over-allocated (causing OOM) while other idle cards cannot be handed out. This is issue Project-HAMi#2232. The scheduler extender is served over HTTP with one goroutine per request and provides no cross-request serialization guarantee, so the read-pick-commit sequence in Filter (read node usage -> pick devices -> commit to cache) must be made atomic. Add a filterLock and move the in-memory critical section into a selectAndCommitDevice helper that owns the lock through a single deferred Unlock (so a future early return can never leak it). The lock covers only device selection and cache commit; the PatchPodAnnotations API call and the on-failure rollback run outside it, so concurrent Filters are not blocked on network I/O. The rollback is safe because the pod/quota managers have their own locks. The simulation path does not reserve devices and is unchanged. In the normal kube-scheduler flow scheduling cycles are already serialized, so this lock is uncontended there; it only closes the window for concurrent extender requests. Tests: - Test_Filter_ConcurrentNoDoubleAllocation fires many pods at Filter simultaneously and asserts each is annotated with a distinct device id. It reliably reproduces the double allocation without the lock and passes with it. - Test_Filter_RollbackOnPatchFailure verifies the cache reservation is rolled back when the annotation patch fails. Signed-off-by: yxxhero <aiopsclub@163.com>
99a8c12 to
b0aacdc
Compare
|
@maishivamhoo123 thanks — I've read CONTRIBUTING.md.
|
|
We can fix it or request the user use k8s >= 1.23 which is old enough. Need more discussion about lt. |
|
/hold |
|
You can view the relevant rule here. |
|
Independent black-box evidence supports this fix: the race is straightforward to reproduce against the real Extender HTTP path, not only with a fake-client test. In an isolated k3d cluster I registered one mock GPU with All 12 requests succeeded and all 12 Pods received the same physical-device allocation annotation. The Scheduler's live metric afterwards was: This exceeds the mock card's |
|
@spencercjh so why close this PR? |
|
@yxxhero I think the HAMi community is currently experiencing a significant impact from AI, and several owners are finding it quite unbearable. Maybe you’ve been flagged as spam. |
|
@FouoF @mesutoezdil Thanks for the thorough validation and the detailed black-box test! I want to clarify that the approach in this PR wasn’t just AI-generated boilerplate. It was the result of careful deliberation regarding the concurrency issues in the Extender path. The filterLock was specifically chosen as a deliberate, minimal fix to address the TOCTOU window, and I’m glad to see it holds up under your rigorous testing. |
|
@spencercjh thanks so much. |
thx for your message. "https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#contribution-gates This rule is in place so that maintainers can communicate with a person and receive a direct, short, and clear answer to the specific question. Bcs no one's native language is English, and no one has time to read long llm texts. as i remember the reason was your this answer: "#2470 (comment)" would you like to open new PR? |
|
@spencercjh hi. any better ideas for this issue? if so please open an new PR or I open new PR? thanks so much. |
I don't know why you're sharing long LLM responses as answers. |
|
@mesutoezdil I'm sorry for taking up your time. I'll follow the community guidelines. |
5 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes #2232 — 并发创建 pod 情况下，存在 pod 中卡 id 的注解和实际 pod 中使用卡的 id 不一致问题 (under concurrent pod creation, the GPU id annotated on a pod is inconsistent with the GPU id the pod actually uses).
The reported consequence is duplicate card allocation: the shared card gets over-allocated (causing OOM) while other idle cards cannot be handed out.
Root cause
The scheduler extender is served over HTTP with one goroutine per request and provides no cross-request serialization guarantee. In
Filter, the device-selection critical section was not atomic:getNodesUsage)calcScore)podManager.AddPod)PatchPodAnnotations)Under concurrent pod creation (e.g. a multi-replica Deployment), two
Filtercalls can both read the same free GPU in step 1 before either commits it in step 3, so both pods are annotated with the same card id.In the normal kube-scheduler flow scheduling cycles are already serialized, which is why the problem only surfaces under concurrent/re-entrant extender requests.
Fix
Add a
filterLockand move the in-memory critical section (steps 1–3) into aselectAndCommitDevicehelper that owns the lock through a singledefer Unlock, so two pods can never reserve the same device. The lock is released before step 4 (PatchPodAnnotations), so concurrent Filters are never blocked on the apiserver round trip; the on-failure rollback runs outside the lock and is safe because the pod/quota managers have their own locks. The simulation path (filterSimulation) does not reserve devices and is unchanged.This keeps the change minimal — it does not move reservation from
FiltertoBind(the larger redesign from #2273 is tracked via the #2264 design discussion).Test
Test_Filter_ConcurrentNoDoubleAllocationfires 8 pods atFiltersimultaneously and asserts each is annotated with a distinct device id. Without the lock it reliably reportsdevice device-7 double-allocated to pod pod-0 and pod pod-2; with the lock it passes (-race).Test_Filter_RollbackOnPatchFailureverifies the cache reservation is rolled back when the annotation patch fails.Validation
Scoped to the scheduler extender, so per CONTRIBUTING's hardware-validation gate this is validated with unit tests (including a
-raceregression test) rather than physical GPU hardware.make verify(license, import-aliases, golangci-lint) andgo test -race ./pkg/scheduler/...pass locally.Which issue(s) this PR fixes:
Fixes #2232
Special notes for your reviewer:
defer Unlockin the helper means any early return added there in future can never leak the lock.filterLockcloses.Does this PR introduce a user-facing change?
AI assistance disclosure:
This PR was written with AI assistance for codebase analysis, implementation and test generation. I reviewed, built and tested the change myself (
make verify,go test -race) and can explain the locking semantics. Per CONTRIBUTING.md, disclosure belongs in the PR description only — no AI co-author trailers are used.Summary by CodeRabbit
Bug Fixes
Tests