fix(scheduler): serialize Filter device selection to prevent double G… - #2559
fix(scheduler): serialize Filter device selection to prevent double G…#2559yxxhero wants to merge 2 commits into
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 |
📝 WalkthroughWalkthroughThe scheduler now serializes concurrent device selection and cache updates. It performs annotation patching after selection and rolls back reservation state when patching fails. Device initialization errors now propagate through startup. ChangesScheduler consistency and initialization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/scheduler/scheduler.go (1)
1001-1019: 🚀 Performance & Scalability | 🔵 TrivialConsider narrowing the critical section or measuring its cost.
filterLockcoversgetNodesUsageandcalcScore.getNodesUsageiterates every cached pod, every device, and every node on each call. All extenderFilterrequests now run this stage one at a time.The serialization is required for the read-select-commit sequence to be atomic, so the current design is correct. For large clusters, add a latency metric around
selectAndCommitDeviceso the queueing cost stays visible.🤖 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 1001 - 1019, Add a latency metric for the complete selectAndCommitDevice operation, including time spent waiting for filterLock and executing getNodesUsage/calcScore, and record it on every return path. Use the existing scheduler metrics conventions and label the measurement consistently with this operation.pkg/scheduler/scheduler_test.go (2)
1918-1923: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the quota rollback too.
The test verifies only the pod-cache rollback.
Filteralso callsquotaManager.RmUsage, but only whenselection.addedis true. A regression in the quota path would pass this test.Add an assertion on the quota state:
💚 Proposed addition to cover the quota rollback
_, inCache := s.podManager.GetPod(pod) require.False(t, inCache, "reservation must be rolled back when the annotation patch fails") + + for _, v := range *s.quotaManager.GetResourceQuota()[pod.Namespace] { + require.Equal(t, int64(0), v.Used, "quota usage must be rolled back when the annotation patch fails") + } }
Test_Filter_EvictsStaleEntryat Line 1763 uses the same accessor, so the helper exists.🤖 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 1918 - 1923, Extend the failure assertions in the Filter test to verify quota usage is rolled back as well as pod-cache state. Reuse the existing quota-manager accessor used by Test_Filter_EvictsStaleEntry, and assert the affected pod’s quota usage is absent or restored after the annotation patch failure.
1850-1859: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollect the
Filtererrors from the goroutines.The goroutines discard both return values. If
Filterfails for one pod, the test fails later atDecodePodDevicesor at the empty-device-id check. That failure message hides the real cause.Store the errors and assert them after
wg.Wait:♻️ Proposed refactor to surface Filter errors
start := make(chan struct{}) + errs := make([]error, numPods) var wg sync.WaitGroup 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]) + _, errs[idx] = s.Filter(extenderv1.ExtenderArgs{Pod: pod, NodeNames: &nodeNames}) + }(i, pods[i]) } close(start) wg.Wait() + for i, err := range errs { + require.NoError(t, err, "Filter failed for pod %s", pods[i].Name) + }Each goroutine writes a distinct slice element, so the race detector stays clean.
🤖 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 1850 - 1859, Update the concurrent Filter test around the goroutine loop to capture each call’s error in a distinct per-pod slice element instead of discarding both return values. After wg.Wait, assert that every collected error is nil before continuing to DecodePodDevices or the empty-device-id checks, preserving race-free writes and surfacing the original Filter failure.
🤖 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 974-982: In pkg/scheduler/scheduler.go lines 974-982, update the
rollback in Filter’s PatchPodAnnotations failure path so
podManager.DelPod(args.Pod) is gated by selection.added, matching
quotaManager.RmUsage and keeping reservation cleanup consistent. In
pkg/scheduler/scheduler_test.go lines 1918-1923, extend the existing inCache
assertion with a quota assertion that verifies both reservation effects are
rolled back.
---
Nitpick comments:
In `@pkg/scheduler/scheduler_test.go`:
- Around line 1918-1923: Extend the failure assertions in the Filter test to
verify quota usage is rolled back as well as pod-cache state. Reuse the existing
quota-manager accessor used by Test_Filter_EvictsStaleEntry, and assert the
affected pod’s quota usage is absent or restored after the annotation patch
failure.
- Around line 1850-1859: Update the concurrent Filter test around the goroutine
loop to capture each call’s error in a distinct per-pod slice element instead of
discarding both return values. After wg.Wait, assert that every collected error
is nil before continuing to DecodePodDevices or the empty-device-id checks,
preserving race-free writes and surfacing the original Filter failure.
In `@pkg/scheduler/scheduler.go`:
- Around line 1001-1019: Add a latency metric for the complete
selectAndCommitDevice operation, including time spent waiting for filterLock and
executing getNodesUsage/calcScore, and record it on every return path. Use the
existing scheduler metrics conventions and label the measurement consistently
with this operation.
🪄 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: f1b6d4d0-3c29-41ce-9a39-73d12e45bb19
📒 Files selected for processing (2)
pkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.go
| // Patch the annotation outside the lock; roll back the cache on failure. | ||
| if err = util.PatchPodAnnotations(args.Pod, selection.annotations); err != nil { | ||
| s.recordScheduleFilterResultEvent(args.Pod, EventReasonFilteringFailed, "", err) | ||
| if selection.added { | ||
| s.quotaManager.RmUsage(args.Pod, m.Devices) | ||
| } | ||
| s.podManager.DelPod(args.Pod) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The rollback path treats the pod cache and the quota inconsistently, and no test binds them together. Filter deletes the pod-cache entry unconditionally but removes the quota usage only when selection.added is true, so the two states can diverge after a patch failure.
pkg/scheduler/scheduler.go#L974-L982: gates.podManager.DelPod(args.Pod)onselection.added, matching thes.quotaManager.RmUsagecall, so both reservation effects are undone together.pkg/scheduler/scheduler_test.go#L1918-L1923: add a quota assertion after theinCachecheck, so the test covers both halves of the rollback.
📍 Affects 2 files
pkg/scheduler/scheduler.go#L974-L982(this comment)pkg/scheduler/scheduler_test.go#L1918-L1923
🤖 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 974 - 982, In
pkg/scheduler/scheduler.go lines 974-982, update the rollback in Filter’s
PatchPodAnnotations failure path so podManager.DelPod(args.Pod) is gated by
selection.added, matching quotaManager.RmUsage and keeping reservation cleanup
consistent. In pkg/scheduler/scheduler_test.go lines 1918-1923, extend the
existing inCache assertion with a quota assertion that verifies both reservation
effects are rolled back.
klog.Fatalf calls os.Exit on transient API errors (informer list failures, label patch failures, config loading), which kills the scheduler process instead of allowing retry. Replace with: - updateSchedulerLabel(): klog.ErrorS + return instead of klog.Fatalf - InitDevices(): return error instead of klog.Fatalf - InitDefaultDevices(): return error instead of klog.Fatalf Update all callers to handle the returned errors. Signed-off-by: yxxhero <aiopsclub@163.com>
…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>
b0aacdc to
4ac00e1
Compare
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:
|
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/config/config.go`:
- Around line 281-295: Make device initialization atomic across
InitDevicesWithConfig and InitDefaultDevices: prevent partially initialized
backends from remaining in the global device maps when any initializer fails.
Build temporary maps and publish them only after all initialization succeeds, or
clear both global maps on every failure path, so subsequent InitDevices calls
retry instead of incorrectly returning success.
🪄 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: 974cda96-3ce5-414e-9c11-115078389dc2
📒 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
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/scheduler/scheduler.go
- pkg/scheduler/scheduler_test.go
| func InitDevices() error { | ||
| if len(device.DevicesMap) > 0 { | ||
| klog.Info("Devices are already initialized, skipping initialization") | ||
| return | ||
| return nil | ||
| } | ||
| klog.Infof("Loading device configuration from file: %s", configFile) | ||
| config, err := LoadConfig(configFile) | ||
| if err != nil { | ||
| klog.Fatalf("Failed to load device config file %s: %v", configFile, err) | ||
| return fmt.Errorf("failed to load device config file %s: %w", configFile, err) | ||
| } | ||
| klog.Infof("Loaded config: %v", config) | ||
| err = InitDevicesWithConfig(config) | ||
| if err != nil { | ||
| klog.Fatalf("Failed to initialize devices: %v", err) | ||
| return fmt.Errorf("failed to initialize devices: %w", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make failed device initialization atomic.
InitDevicesWithConfig adds successful backends to the global maps before it returns initialization errors. If a later backend fails, a subsequent InitDevices call can see the non-empty map and return nil. InitDefaultDevices can leave the same partial state after its nested call fails. Build the maps locally and publish them only after all initializers succeed, or clear both global maps on failure.
Also applies to: 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 281 - 295, Make device
initialization atomic across InitDevicesWithConfig and InitDefaultDevices:
prevent partially initialized backends from remaining in the global device maps
when any initializer fails. Build temporary maps and publish them only after all
initialization succeeds, or clear both global maps on every failure path, so
subsequent InitDevices calls retry instead of incorrectly returning success.
Pls write only your own sentences, as specified in the contribution guidelines. |
There was a problem hiding this comment.
this looks like the follow up to closed pr #2470.
left 2 questions inline about this version.
| // selectAndCommitDevice selects a device and commits it to the cache under | ||
| // filterLock. It does not publish the pod annotation; the caller patches it. | ||
| func (s *Scheduler) selectAndCommitDevice(args extenderv1.ExtenderArgs, resourceReqs device.PodDeviceRequests) (*filterSelection, error) { | ||
| s.filterLock.Lock() |
There was a problem hiding this comment.
this lock is global, not per node or per device. a big concurrent deployment create now queues through one lock for every node. was a per node lock considered, not only per pod uid?
| ) | ||
|
|
||
| config.InitDevices() | ||
| if err := config.InitDevices(); err != nil { |
There was a problem hiding this comment.
this initdevices error change is not about the filterlock race. this repo closed a past pr for bundling unrelated changes together. should this be its own pr?
|
@yxxhero I think the root cause for #2232 is that specific Kubernetes versions, kube-scheduler cannot guarantee that the Extender's Filter is called only once during a Pod's scheduling cycle. Due to timing issues between Filter and Bind, the mismatch happens. That is why @spencercjh cannot reproduce it in K3s, I guess him testing with a higher version than v1.23. |
|
|
||
| // selectAndCommitDevice selects a device and commits it to the cache under | ||
| // filterLock. It does not publish the pod annotation; the caller patches it. | ||
| func (s *Scheduler) selectAndCommitDevice(args extenderv1.ExtenderArgs, resourceReqs device.PodDeviceRequests) (*filterSelection, error) { |
There was a problem hiding this comment.
which k8s version, and where in kube-scheduler's source does that happen? without that this fix may be locking against a race that never occurs in real scheduling.
|
This scenario happens to k8s version < v1.23, and conflicts with prerequisities which requires k8s>=v1.23. Reopen this PR if encounter the same issue with k8s version >= v1.23 |
4 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