fix(scheduler): prevent concurrent over-allocation and quota leak in Filter (#1330, #1896) - #2523
Conversation
…hPodAnnotations Kubernetes node names can be up to 253 characters (RFC 1123 DNS subdomain), but label values are hard-limited to 63 characters and must match (([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?. PatchPodAnnotations was previously copying the raw node name from annotations[AssignedNodeAnnotations] directly into a pod label, which caused the Kubernetes API server to reject the entire patch request with a validation error whenever the node name exceeded 63 characters -- breaking scheduling for any pod bound to such a node. Fix: - Use k8s.io/apimachinery/pkg/util/validation.IsValidLabelValue to validate the node name before setting label[AssignedNodeAnnotations]. - If valid (the common case for short node names), set the label as before. - If invalid (e.g. node name > 63 chars or illegal characters), skip setting that label and emit a klog.Warningf noting the pod, node name, and reason. - Annotation patching is unchanged in all cases. Tests: - Extended TestPatchPodAnnotations in pkg/util/util_test.go with three new table-driven cases: * short (valid) node name -> label set correctly * node name > 63 chars -> patch succeeds, annotation set, label absent * node name with invalid characters (<= 63 chars) -> same as above All existing pkg/util tests continue to pass. Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>
…arification comment Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>
…nd vGPUmonitor pod lister - Implement exported helper util.SafeLabelValue(value string) string that deterministically derives a valid k8s label value (<= 63 chars) using a prefix + sha256 hash. - Update PatchPodAnnotations to ALWAYS set label[AssignedNodeAnnotations] = SafeLabelValue(v) (never skipping the label). Log V(4) info when derived value differs from original. - Update cmd/vGPUmonitor/metrics.go (collectPodAndContainerInfo and collectPodAndContainerMigInfo) to wrap odeName with util.SafeLabelValue(nodeName) when constructing the pod lister selector. - Update unit tests in pkg/util/util_test.go and cmd/vGPUmonitor/metrics_test.go to assert label presence and write/read selector consistency for long node names. Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>
…ect-HAMi#1330, Project-HAMi#1896) - Protect Filter() critical section with filterMutex across capacity check and allocation commit to prevent concurrent over-allocation (Project-HAMi#1330). - Roll back both podManager and quotaManager allocations when PatchPodAnnotations fails (Project-HAMi#1896). - Add regression unit tests and BenchmarkFilter. Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AyushSrivastava1818 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds deterministic Kubernetes label sanitization for node names and applies it to metric selectors and pod annotations. It also serializes scheduler allocation state, restores quota usage after annotation failures, and adds concurrency and regression tests. ChangesAllocation consistency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant podManager
participant quotaManager
participant KubernetesAPI
Scheduler->>podManager: select and record device allocation
Scheduler->>quotaManager: record quota usage
Scheduler->>KubernetesAPI: patch pod annotations
KubernetesAPI-->>Scheduler: patch result
alt patch succeeds
Scheduler-->>Scheduler: report selected node
else patch fails
Scheduler->>podManager: remove allocation
Scheduler->>quotaManager: remove quota usage
Scheduler-->>Scheduler: return patch error
end
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: 3
🧹 Nitpick comments (8)
pkg/scheduler/scheduler_test.go (4)
2687-2689: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the quota assertion the doc comment promises.
The function comment states that the test verifies no phantom state in
podManagerorquotaManager. The body only checkspodManager. Add the matching quota check.💚 Proposed addition
// Ensure no phantom allocation was created in podManager. _, inCache := s.podManager.GetPod(pod) require.False(t, inCache, "pod must not be recorded in podManager cache when Fit fails") + + // Ensure no phantom quota usage was recorded. + for _, dq := range s.quotaManager.GetResourceQuota() { + for name, q := range *dq { + require.Equal(t, int64(0), q.Used, "quota %s must remain unused when Fit fails", name) + } + }🤖 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 2687 - 2689, Add the missing quotaManager assertion in the test alongside the existing podManager check, using the relevant quota lookup to verify no phantom quota allocation is recorded when Fit fails. Preserve the existing podManager assertion and failure message style.
2462-2466: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip the 100-iteration loop in short mode.
Each iteration builds a scheduler, a fake clientset, an informer factory, and five pods, then runs five goroutines. Under
-racethis adds significant time to the package test run. The repository test convention uses-short.♻️ Proposed change
const ( ns = "race-test-ns" concurrency = 5 // pods racing to claim the only GPU slot iterations = 100 // repeat to expose statistical races ) + + loops := iterations + if testing.Short() { + loops = 5 + }Then use
loopsas the loop bound.As per 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 2462 - 2466, The race test’s fixed 100-iteration loop should honor Go short-test mode. In the test around constants ns, concurrency, and iterations, derive a loops bound that skips the iterations when testing.Short() is true, then use loops as the loop bound while preserving the existing full iteration count otherwise.Source: Coding guidelines
2742-2800: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove pod construction and creation out of the timed region.
Both sub-benchmarks build a
corev1.Podand callPods().Create()inside the measured loop. The fake clientsetCreateperforms a deep copy, takes the object-tracker lock, and walks the reactor chain. That cost is attributed toFilter(). UnderRunParallelthe tracker lock also adds contention that is unrelated tofilterMutex, so the benchmark cannot show the lock throughput it is meant to measure.Pre-create the pods into a slice before
b.ResetTimer()and index into it inside the loop.A second point: every successful
Filter()commits an allocation intopodManager, sogetNodesUsagecost grows withb.N. The benchmark is therefore not measuring steady state. Consider clearing the pod cache periodically or documenting this in the comment.🤖 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 2742 - 2800, Move pod construction and fake-clientset creation out of the timed sections of both “Uncontended” and “ContendedParallel” benchmarks by pre-creating pods in slices before b.ResetTimer(), then index the appropriate pod inside each measured iteration. Prevent podManager state from growing across iterations by periodically clearing the pod cache, or document the intentional accumulation if that behavior must remain.
2395-2403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the patch failure explicit with a reactor.
Replace the implicit fake-clientset "missing pod causes
NotFound" setup with an explicitPrependReactor("patch", "pods", ...). Add at.Cleanupto closes.stopCh.🤖 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 2395 - 2403, Update the test setup around the fake KubeClient to register an explicit PrependReactor for patch operations on pods that returns the intended failure, rather than relying on a missing pod to produce NotFound. Add t.Cleanup to close s.stopCh after the test, while preserving the existing informer setup and failure scenario.pkg/scheduler/scheduler.go (4)
1028-1037: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the
bestNodedereference.The caller treats
err == nil && filterRes == nilas proof thatbestNodeis non-nil.tryAllocatesatisfies this contract today. A future edit that adds an earlyreturn nil, nil, nilpath would produce a nil-pointer panic at Line 1036. Add an explicit check.🛡️ Proposed guard
filterRes, bestNode, err := s.tryAllocate(args, resourceReqs) if err != nil || filterRes != nil { return filterRes, err } + if bestNode == nil { + return nil, fmt.Errorf("tryAllocate returned no node for pod %s/%s", args.Pod.Namespace, args.Pod.Name) + }🤖 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 1028 - 1037, Add an explicit nil check for bestNode after the tryAllocate result handling and before the klog.InfoS dereference. Preserve the existing return behavior for errors or filter results, and handle a nil bestNode safely without allowing the scheduling path to panic.
942-980: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
tryAllocateserializes full node scoring; confirm the scale target.The mutex now covers
getNodesUsageandcalcScorefor every candidate node. All concurrentFilter()calls are serialized for the full duration of scoring, not only for the commit. This is the correct fix for over-allocation, but it converts filtering into a single-threaded stage.BenchmarkFilteruses 200 nodes; large clusters with many candidate nodes and a high pod arrival rate will queue behind this lock.If the benchmark shows acceptable latency at your target cluster size, no change is needed. Otherwise consider a two-phase approach: score without the lock, then re-verify fit and commit under the lock, retrying when the re-check fails.
🤖 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 942 - 980, Benchmark tryAllocate and the Filter path at the target cluster size and pod arrival rate, including the 200-node BenchmarkFilter scenario, to verify whether holding filterMutex across getNodesUsage and calcScore meets latency requirements. If contention is unacceptable, refactor tryAllocate into an unlocked scoring phase followed by locked fit re-validation and allocation commit, retrying when the re-check fails; otherwise leave the current serialization unchanged.
982-987: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFour call sites re-implement the same locked cleanup sequence.
cleanupStalePodAllocationLockedand its wrapper already express "take the pod frompodManagerand remove its quota usage underfilterMutex". Three other sites repeat that sequence inline, and two of them use explicitUnlock()instead ofdefer, so a panic insideTakeAndDeletePod,RmUsage, orAddUsagewould leavefilterMutexheld and block every laterFilter()call.
pkg/scheduler/scheduler.go#L982-L987: replace the body ofrollbackAllocationwith a call tocleanupStalePodAllocation(pod).pkg/scheduler/scheduler.go#L155-L181: replace the terminated-pod branch withs.cleanupStalePodAllocation(pod), and move theAddPod/AddUsageblock into a small method that usesdefer s.filterMutex.Unlock().pkg/scheduler/scheduler.go#L212-L216: replace the block withs.cleanupStalePodAllocation(pod).🤖 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 982 - 987, Centralize pod allocation cleanup and ensure mutexes are released on panic: in pkg/scheduler/scheduler.go lines 982-987, replace rollbackAllocation’s locked body with cleanupStalePodAllocation(pod); in lines 155-181, use cleanupStalePodAllocation(pod) for the terminated-pod branch and move the AddPod/AddUsage logic into a small method that unlocks filterMutex with defer; in lines 212-216, replace the repeated cleanup block with cleanupStalePodAllocation(pod).
982-987: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
rollbackAllocationduplicatescleanupStalePodAllocation.Both methods take
filterMutexand callcleanupStalePodAllocationLocked. The bodies are identical. Keep one implementation, and if the rollback name adds value at the call site, define it as a direct call tocleanupStalePodAllocation.♻️ Proposed refactor
// rollbackAllocation rolls back podManager and quotaManager allocations on annotation patch failure (upstream `#1896`). func (s *Scheduler) rollbackAllocation(pod *corev1.Pod) { - s.filterMutex.Lock() - defer s.filterMutex.Unlock() - s.cleanupStalePodAllocationLocked(pod) + s.cleanupStalePodAllocation(pod) }🤖 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 982 - 987, Remove the duplicated locking and cleanup implementation from rollbackAllocation, and reuse cleanupStalePodAllocation directly. If rollbackAllocation remains for call-site clarity, make it a thin delegation to cleanupStalePodAllocation while preserving the existing locking behavior in the single shared implementation.
🤖 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_test.go`:
- Around line 2461-2468: Fix lint issues in pkg/scheduler/scheduler_test.go at
lines 2461-2468, 2691-2697, and 2721-2723: terminate the Run with comment with a
period, gofmt-align the const block, and modernize the iterations loop to range
iterations; preserve the intentional empty critical section by adding a
//lint:ignore SA2001 directive or use filterMutex.TryLock with a matching
Unlock; modernize the nodeCount loop to range nodeCount.
- Around line 2555-2561: Update the admission assertion in the test loop around
admitted.Load() to require exactly one admitted pod, replacing the at-most-one
check while preserving the existing diagnostic message. If a valid execution
path can admit zero pods per iteration, retain the upper-bound assertion and add
an aggregate assertion ensuring at least one pod is admitted across all
iterations.
- Around line 2826-2875: Update the test setup around podDev and onAddPod to
encode podDev with device.EncodePodDevices and attach the resulting annotations
to pod.Annotations, exercising informer device decoding. Assert the second
podManager.AddPod call returns false, quota usage remains unchanged after
onAddPod, and the cached PodInfo.Devices still equals podDev.
---
Nitpick comments:
In `@pkg/scheduler/scheduler_test.go`:
- Around line 2687-2689: Add the missing quotaManager assertion in the test
alongside the existing podManager check, using the relevant quota lookup to
verify no phantom quota allocation is recorded when Fit fails. Preserve the
existing podManager assertion and failure message style.
- Around line 2462-2466: The race test’s fixed 100-iteration loop should honor
Go short-test mode. In the test around constants ns, concurrency, and
iterations, derive a loops bound that skips the iterations when testing.Short()
is true, then use loops as the loop bound while preserving the existing full
iteration count otherwise.
- Around line 2742-2800: Move pod construction and fake-clientset creation out
of the timed sections of both “Uncontended” and “ContendedParallel” benchmarks
by pre-creating pods in slices before b.ResetTimer(), then index the appropriate
pod inside each measured iteration. Prevent podManager state from growing across
iterations by periodically clearing the pod cache, or document the intentional
accumulation if that behavior must remain.
- Around line 2395-2403: Update the test setup around the fake KubeClient to
register an explicit PrependReactor for patch operations on pods that returns
the intended failure, rather than relying on a missing pod to produce NotFound.
Add t.Cleanup to close s.stopCh after the test, while preserving the existing
informer setup and failure scenario.
In `@pkg/scheduler/scheduler.go`:
- Around line 1028-1037: Add an explicit nil check for bestNode after the
tryAllocate result handling and before the klog.InfoS dereference. Preserve the
existing return behavior for errors or filter results, and handle a nil bestNode
safely without allowing the scheduling path to panic.
- Around line 942-980: Benchmark tryAllocate and the Filter path at the target
cluster size and pod arrival rate, including the 200-node BenchmarkFilter
scenario, to verify whether holding filterMutex across getNodesUsage and
calcScore meets latency requirements. If contention is unacceptable, refactor
tryAllocate into an unlocked scoring phase followed by locked fit re-validation
and allocation commit, retrying when the re-check fails; otherwise leave the
current serialization unchanged.
- Around line 982-987: Centralize pod allocation cleanup and ensure mutexes are
released on panic: in pkg/scheduler/scheduler.go lines 982-987, replace
rollbackAllocation’s locked body with cleanupStalePodAllocation(pod); in lines
155-181, use cleanupStalePodAllocation(pod) for the terminated-pod branch and
move the AddPod/AddUsage logic into a small method that unlocks filterMutex with
defer; in lines 212-216, replace the repeated cleanup block with
cleanupStalePodAllocation(pod).
- Around line 982-987: Remove the duplicated locking and cleanup implementation
from rollbackAllocation, and reuse cleanupStalePodAllocation directly. If
rollbackAllocation remains for call-site clarity, make it a thin delegation to
cleanupStalePodAllocation while preserving the existing locking behavior in the
single shared implementation.
🪄 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: 224f4496-34f0-4e6d-bf88-1fca1b9dd875
📒 Files selected for processing (6)
cmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_test.gopkg/scheduler/scheduler.gopkg/scheduler/scheduler_test.gopkg/util/util.gopkg/util/util_test.go
| admittedCount := admitted.Load() | ||
| require.LessOrEqualf(t, admittedCount, int32(1), | ||
| "iteration %d: admitted %d pods against a node/quota with capacity for 1 (Bug #1330)", | ||
| iter, admittedCount) | ||
|
|
||
| close(s.stopCh) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert exactly one admission, not at most one.
require.LessOrEqualf(admittedCount, int32(1), ...) passes when admittedCount is 0. A regression that makes Filter() reject every pod would keep this test green. The node and the quota both have capacity for exactly one pod, and filterMutex serializes the calls, so exactly one pod must be admitted.
💚 Proposed fix
admittedCount := admitted.Load()
- require.LessOrEqualf(t, admittedCount, int32(1),
- "iteration %d: admitted %d pods against a node/quota with capacity for 1 (Bug `#1330`)",
- iter, admittedCount)
+ require.Equalf(t, int32(1), admittedCount,
+ "iteration %d: admitted %d pods against a node/quota with capacity for exactly 1 (Bug `#1330`)",
+ iter, admittedCount)If a legitimate path can admit zero pods, keep LessOrEqual and add a separate assertion that at least one pod was admitted across all iterations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| admittedCount := admitted.Load() | |
| require.LessOrEqualf(t, admittedCount, int32(1), | |
| "iteration %d: admitted %d pods against a node/quota with capacity for 1 (Bug #1330)", | |
| iter, admittedCount) | |
| close(s.stopCh) | |
| } | |
| admittedCount := admitted.Load() | |
| require.Equalf(t, int32(1), admittedCount, | |
| "iteration %d: admitted %d pods against a node/quota with capacity for exactly 1 (Bug `#1330`)", | |
| iter, admittedCount) | |
| close(s.stopCh) | |
| } |
🤖 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 2555 - 2561, Update the
admission assertion in the test loop around admitted.Load() to require exactly
one admitted pod, replacing the at-most-one check while preserving the existing
diagnostic message. If a valid execution path can admit zero pods per iteration,
retain the upper-bound assertion and add an aggregate assertion ensuring at
least one pod is admitted across all iterations.
| pod := &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "tracked-pod", | ||
| Namespace: ns, | ||
| UID: "tracked-uid", | ||
| Annotations: map[string]string{ | ||
| util.AssignedNodeAnnotations: "node1", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| podDev := device.PodDevices{ | ||
| nvidia.NvidiaGPUDevice: device.PodSingleDevice{ | ||
| { | ||
| { | ||
| Idx: 0, | ||
| UUID: "gpu0", | ||
| Type: nvidia.NvidiaGPUDevice, | ||
| Usedmem: 2048, | ||
| Usedcores: 20, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // 1. Add pod to podManager and quotaManager directly (simulating prior Filter allocation). | ||
| added := s.podManager.AddPod(pod, "node1", podDev) | ||
| require.True(t, added, "initial AddPod must return true for new pod") | ||
| s.quotaManager.AddUsage(pod, podDev) | ||
|
|
||
| getUsage := func() int64 { | ||
| q := s.quotaManager.GetResourceQuota() | ||
| if nsq, ok := q[ns]; ok { | ||
| if entry, ok := (*nsq)[memResourceName]; ok { | ||
| return entry.Used | ||
| } | ||
| } | ||
| return 0 | ||
| } | ||
|
|
||
| initialUsage := getUsage() | ||
| require.Greater(t, initialUsage, int64(0), "quota usage should be recorded initially") | ||
|
|
||
| // 2. Trigger onAddPod (simulating informer resync or event for already-tracked pod). | ||
| s.onAddPod(pod) | ||
|
|
||
| usageAfterInformerEvent := getUsage() | ||
| require.Equal(t, initialUsage, usageAfterInformerEvent, | ||
| "quota usage must NOT be double-counted when onAddPod runs for an already-tracked pod") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm DecodePodDevices behavior for annotations with no device keys,
# and confirm countPodDevices returns empty for empty PodDevices.
ast-grep run --pattern 'func DecodePodDevices($$$) { $$$ }' --lang go pkg/device
ast-grep run --pattern 'func countPodDevices($$$) { $$$ }' --lang go pkg/device
rg -nP --type=go -C3 'func EncodePodDevices' pkg/deviceRepository: Project-HAMi/HAMi
Length of output: 532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)scheduler_test\.go$|(^|/)devices\.go$|(^|/)quota|podManager|addusage' | head -100
echo
echo "== relevant test context =="
sed -n '2700,2890p' pkg/scheduler/scheduler_test.go
echo
echo "== onAddPod implementation =="
rg -n -C4 'func \(s \*Scheduler\) onAddPod|AddUsage|DecodePodDevices|countPodDevices' pkg/scheduler -S
echo
echo "== device APIs outline/sections =="
sed -n '360,490p' pkg/device/devices.go
rg -n -C4 'func countPodDevices|type PodDevices|DecodePodDevices|EncodePodDevices' pkg/device pkg/scheduler devices.go -SRepository: Project-HAMi/HAMi
Length of output: 25960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scheduler.go relevant onAddPod section =="
sed -n '120,185p' pkg/scheduler/scheduler.go
echo
echo "== device quota countPodDevices/AddUsage sections =="
sed -n '60,150p' pkg/device/quota.go
echo
echo "== podManager AddPod/GetPod sections =="
rg -n -C4 'func \(m \*.*PodManager\) AddPod|func \(m \*.*PodManager\) GetPod|type .*PodManager' pkg/scheduler pkg/device -S
echo
echo "== existing Issue1368 test context =="
sed -n '1990,2065p' pkg/scheduler/scheduler_test.go
echo
echo "== imports in scheduler_test.go for required helper =="
sed -n '1,80p' pkg/scheduler/scheduler_test.goRepository: Project-HAMi/HAMi
Length of output: 10114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '54,125p' pkg/device/pods.go
sed -n '430,540p' pkg/device/devices.goRepository: Project-HAMi/HAMi
Length of output: 5104
Encode device annotations so this test covers the informer path.
onAddPod decodes devices from device.EncodePodDevices(device.SupportDevices, ...), so this test currently passes podDev directly into AddPod without exercising that decode path. Copy the encoded annotations onto the pod, then assert AddPod returns false, quota usage is unchanged, and the cached PodInfo.Devices still matches podDev.
[functional_and_code_integrity]
🤖 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 2826 - 2875, Update the test
setup around podDev and onAddPod to encode podDev with device.EncodePodDevices
and attach the resulting annotations to pod.Annotations, exercising informer
device decoding. Assert the second podManager.AddPod call returns false, quota
usage remains unchanged after onAddPod, and the cached PodInfo.Devices still
equals podDev.
- Add missing periods to comments per godot rules. - Format range loop statements to use range over int values per modernize. - Add nolint annotation for SA2001 empty critical section intentional sync point. Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
GitHub Actions
Inactive
|
The quota rollback after a failed annotation patch is a real local-state bug, but the current branch is not a reviewable fix for it. It fully stacks #2516, serializes all Filter calls with a process-local mutex that cannot solve the multi-scheduler-replica case, and changes the success-message score list. Please remove the #2516 and concurrency changes and retain only the symmetric #1896 quota rollback with a focused regression test. The #1330 cross-replica problem needs a separate supported-topology design rather than this mutex. |
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it
This PR fixes two concurrency and state-consistency issues in the HAMi Kubernetes scheduler's
Filter()extender path.Bug vGPU count on single GPU is over the splitcount limit #1330 — Concurrent capacity check and commit race
Scheduler.Filter()previously performedFit/FitQuotachecks separately frompodManager.AddPod/quotaManager.AddUsage. SinceFilter()can be invoked concurrently for different pods, multiple requests could pass the same capacity check before either one committed its allocation.This could result in GPU memory, device cores, or quota resources being over-allocated beyond the available capacity.
Bug scheduler: quota usage may not be rolled back when PatchPodAnnotations fails in Filter #1896 — Missing quota rollback on annotation patch failure
If
util.PatchPodAnnotations()failed after the allocation had already been committed, the existing error path could remove the pod allocation without rolling back the corresponding quota usage.This left stale quota accounting in memory and could cause subsequent pods to be incorrectly rejected even though the resources were actually available.
Which issue(s) this PR fixes
Fixes #1330
Fixes #1896
Key Technical Changes
Atomic
Filter()allocation withfilterMutexfilterMutex sync.MutextoScheduler.AddPod/AddUsage.tryAllocate()androllbackAllocation().Deadlock-safe stale allocation cleanup
Split
cleanupStalePodAllocation()into:cleanupStalePodAllocationLocked()for callers already holdingfilterMutex.cleanupStalePodAllocation()as the locking entry point.This avoids recursive locking while keeping the existing cleanup path safe.
Symmetric allocation rollback
PatchPodAnnotations()fails after a successful allocation,rollbackAllocation()now removes the pod frompodManagerand rolls back its usage fromquotaManagerunder the same mutex.Informer event accounting audit
onAddPodonly callsquotaManager.AddUsage()whenpodManager.AddPod()actually records a new pod.Verification — Race Detector
Verified under a CGO-enabled Linux environment (
go1.26.5 linux/amd64, 12 logical cores).The following scheduler tests pass under the Go race detector:
The tests cover concurrent over-allocation prevention, quota rollback after patch failure, non-contending scheduling, lock release on failed allocation, and duplicate quota accounting during informer events.
AI Disclosure
AI tools like Claude were used during the development of this PR for code review, debugging assistance, and exploring potential concurrency issues.
All proposed changes were manually reviewed, implemented, and verified by the contributor. The fix was validated with the Go race detector and the relevant scheduler tests.
Summary by CodeRabbit
Bug Fixes
Reliability