fix(scheduler): serialize pod quota updates to avoid double counting - #2487
fix(scheduler): serialize pod quota updates to avoid double counting#2487Lcos-000 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Lcos-000 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 |
|
Welcome @Lcos-000! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
📝 WalkthroughWalkthroughThe scheduler adds a mutex that serializes pod cache and quota updates across pod callbacks, stale allocation cleanup, and the live ChangesPod State Synchronization
Estimated code review effort: 3 (Moderate) | ~15 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ 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: Lcos <3349888061@qq.com>
9394e30 to
f4a847e
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: 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 969-973: The podMu lock currently spans external event recording
through Filter return; narrow it to the in-memory TakeAndDeletePod,
getNodesUsage/calcScore, AddPod, quota, annotation-failure, and rollback
transaction. Replace deferred unlocking with explicit unlock before
recordScheduleFilterResultEvent, while preserving lock coverage for annotation
failures and rollback.
🪄 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: 5d75e2bb-7a8d-4eda-9f50-3460af34e4da
📒 Files selected for processing (1)
pkg/scheduler/scheduler.go
|
|
||
| // Keep the pod absent from the manager for the complete usage calculation. | ||
| // Informer callbacks must not re-add it with stale devices in this window. | ||
| s.podMu.Lock() | ||
| defer s.podMu.Unlock() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline pkg/scheduler/scheduler.go \
--items all \
--type function \
--match 'Filter|onAddPod|onDelPod|cleanupStalePodAllocation'
fi
rg -n -C 8 'PatchPodAnnotations|recordScheduleFilterResultEvent' .
rg -n -C 5 'podMu\.(Lock|Unlock)|getNodesUsage\s*\(' \
pkg/scheduler --glob '*.go'Repository: Project-HAMi/HAMi
Length of output: 25350
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "scheduler.Filter and surrounding handler:"
sed -n '924,1036p' pkg/scheduler/scheduler.go
echo
echo "PatchPodAnnotations implementation:"
sed -n '163,190p' pkg/util/util.go
echo
echo "recordScheduleFilterResultEvent implementation:"
sed -n '58,82p' pkg/scheduler/event.go
echo
echo "informer callbacks using podMu:"
sed -n '138,170p' pkg/scheduler/scheduler.go
sed -n '192,214p' pkg/scheduler/scheduler.go
sed -n '806,818p' pkg/scheduler/scheduler.goRepository: Project-HAMi/HAMi
Length of output: 8139
Shorten the podMu critical section before external calls.
defer s.podMu.Unlock() holds the scheduler-wide mutex until Filter returns. Because Filter calls event recording while still holding the lock, slow Kubernetes event writes can block pod callbacks and live filter requests. Keep podMu only around the in-memory TakeAndDeletePod → getNodesUsage/calcScore → AddPod/quota transaction. Move recordScheduleFilterResultEvent outside the lock; handle annotation failures and rollback under the lock.
🤖 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 969 - 973, The podMu lock currently
spans external event recording through Filter return; narrow it to the in-memory
TakeAndDeletePod, getNodesUsage/calcScore, AddPod, quota, annotation-failure,
and rollback transaction. Replace deferred unlocking with explicit unlock before
recordScheduleFilterResultEvent, while preserving lock coverage for annotation
failures and rollback.
|
duplicated |
2 participants
What does this PR do?
Fixes #2478.
Synchronizes pod device/quota accounting between
Scheduler.Filter()and the pod informer callbacks. A dedicatedpodMumutex now serializes theTakeAndDeletePod → recompute → AddPodcycle inFilter()and all informer handlers (onAddPod,onDelPod,cleanupStalePodAllocation) that mutatePodManager+QuotaManager.Why is this fix needed?
Without the lock,
onAddPodcould fire betweenFilter()'sTakeAndDeletePod(line 956) and its subsequentAddPod(line 998). Since thePodManagerkeys pods by UID:Filterdeletes the pod from the cache.onAddPodre-adds it with stale devices and callsAddUsage(stale_devices).Filter's laterAddPod(new_devices)returnsadded=false, so the new usage is never recorded while the stale usage stays counted.This causes double-counted / leaked quota usage that is never cleaned up.
Description
Scheduler.podMu sync.Mutex.Filter()live-path usage window.onAddPod,onDelPod, andcleanupStalePodAllocation.Validation
go test ./pkg/scheduler/... -short -count=1passes.go test -race ./pkg/scheduler/... -short -count=1passes.AI assistance disclosure
This PR was written primarily by opencode (an AI coding assistant). Per CONTRIBUTING.md, AI assistance is disclosed here.
Summary by CodeRabbit