fix(scheduler): preserve concurrent node locks - #2197
Conversation
Signed-off-by: ShiroKSH <kushidashiro@gmail.com>
|
Welcome @ShiroKSH! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughNode lock acquisition now reports contention and re-checks ownership during retries. Release logic avoids clearing changed locks, supports same-pod restamped locks, and adds conflict-focused tests. ChangesNode lock contention handling
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 |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: ShiroKSH <kushidashiro@gmail.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/util/nodelock/nodelock_test.go (1)
47-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPersist the simulated lock state in the fake tracker.
Handled GET reactors return objects only for those requests; they do not write the fake client’s
ObjectTracker. The retry reads see the original tracked node (no lock orholderA), so these conflict tests don’t observe the injected lock unless the getter reactor also storesholderBin the tracker when returning it.Update the tracker in the same place where the synthetic
Nodeis returned:
pkg/util/nodelock/nodelock_test.go#L47-L76: persistholderBbefore allowing the retry GET.pkg/util/nodelock/nodelock_test.go#L93-L121: persistholderBbefore allowing the retry GET.pkg/util/nodelock/nodelock_test.go#L137-L169: persistrestampedLockbefore allowing the retry GET, so both the retry GET and second merge-patch use the restamped version.🤖 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/util/nodelock/nodelock_test.go` around lines 47 - 76, Update the GET reactors in pkg/util/nodelock/nodelock_test.go at lines 47-76 and 93-121 to persist the synthetic node with holderB in the fake client tracker before returning it; at lines 137-169, persist the node with restampedLock before returning it so subsequent GET and merge-patch operations observe the restamped state. Use the existing clientSet tracker and reactor context in each test without changing the conflict assertions.
🤖 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/util/nodelock/nodelock.go`:
- Around line 213-215: Update the lock ownership check in the nodelock retry
path so timestamp-only legacy currentLock values must equal lockStr before
proceeding, while retaining suffix matching for owner-encoded locks containing
NodeLockSep. Ensure a concurrently replaced legacy lock returns nil instead of
being cleared.
---
Outside diff comments:
In `@pkg/util/nodelock/nodelock_test.go`:
- Around line 47-76: Update the GET reactors in
pkg/util/nodelock/nodelock_test.go at lines 47-76 and 93-121 to persist the
synthetic node with holderB in the fake client tracker before returning it; at
lines 137-169, persist the node with restampedLock before returning it so
subsequent GET and merge-patch operations observe the restamped state. Use the
existing clientSet tracker and reactor context in each test without changing the
conflict assertions.
🪄 Autofix (Beta)
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: c647e1d7-b3ae-416d-9cf4-b0a479b49051
📒 Files selected for processing (2)
pkg/util/nodelock/nodelock.gopkg/util/nodelock/nodelock_test.go
Signed-off-by: ShiroKSH <kushidashiro@gmail.com>
|
Looked through the unresolved review threads: Re: the restamped-lock silent-skip in The issue is in this retry block in if skipNodeLockOwnerCheck || !strings.Contains(currentLock, NodeLockSep) {
if currentLock != lockStr {
return nil // silent exit
}
}If The outer check for owner-encoded locks correctly uses suffix matching (pod ns/name), but this branch compares the exact original lock string, which breaks when the timestamp refreshes. A straightforward fix would be to apply the same suffix check here: if !skipNodeLockOwnerCheck {
if strings.Contains(currentLock, NodeLockSep) {
if !strings.HasSuffix(currentLock, lockOwner) {
return nil
}
} else if currentLock != lockStr {
// legacy format: only abort if another holder took over
return nil
}
}This is consistent with CodeRabbit's suggestion that was marked resolved, but Re: line 157 (SetNodeLock) — The current code already returns Happy to assist test or iterate on this if helpful. |
Shouren
left a comment
Shouren
left a comment
There was a problem hiding this comment.
Prevents concurrent scheduler replicas from replacing or clearing a node lock after a Kubernetes API conflict retry.
@ShiroKSH HAMi does not currently support active-active scheduler replicas. Its high-availability model uses leader election, so only the leader is expected to perform scheduling and binding while the other replicas remain standby. Therefore, concurrent node-lock updates from multiple scheduler replicas are not expected in a supported deployment, and the scenario this PR aims to fix does not appear to exist.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, ShiroKSH The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
/kind bug
What this PR does / why we need it:
Prevents concurrent scheduler replicas from replacing or clearing a node lock after a Kubernetes API conflict retry.
Which issue(s) this PR fixes:
Fixes none.
Special notes for your reviewer:
SetNodeLockandReleaseNodeLockpreviously checked the annotation before enteringretry.OnError. After a conflict, the retry fetched a newer Node and patched it without rechecking the lock. An acquisition could overwrite a concurrent holder; a release could remove it.The retry now treats the annotation observed before patching as a compare-and-set target. Acquisition returns
ErrNodeLockContentionwhen a refreshed Node is locked. Release only removes the exact lock value it originally observed. Focused fake-client regression tests cover both conflict paths.Validation:
make verifymake testgo test ./pkg/util/nodelock -run 'Test(SetNodeLockPreservesConcurrentLockAfterConflict|ReleaseNodeLockPreservesConcurrentLockAfterConflict)$' -count=1 -v -timeout=60sThis is scheduler-only; no device-allocation or in-container isolation path changed.
Does this PR introduce a user-facing change?:
No.
AI assistance disclosure:
AI assistance was used for repository exploration, race analysis, and drafting the focused implementation and tests. I reviewed the affected scheduler path and the validation results, and take responsibility for this contribution.
Summary by CodeRabbit
Bug Fixes
Tests