fix(nodelock): let a pod re-acquire its own still-valid node lock - #2255
Conversation
lockAllDevices calls LockNode once per device vendor a pod requests resources from (device.GetDevices() is iterated per vendor backend), all writing to the same shared node annotation (NodeLockKey). LockNode had no case for "this exact pod already holds the lock": the dangling- lock check only ran for a different pod, so a same-pod, non-expired lock fell straight through to the contention error. A pod requesting resources from two or more HAMi-managed vendors (e.g. nvidia.com/gpu and cambricon.com/vmlu together) would have its first LockNode call succeed and its second one immediately contend with its own lock, making it permanently unschedulable. Add an explicit branch: if the existing lock's namespace/name match the calling pod and it hasn't expired, treat it as already acquired instead of erroring. Test_LockNode's "node has been locked" case had (accidentally) used the exact same pod identity as both the lock holder and the requester, so it was asserting the buggy contention behavior as correct. Split it into a genuine third-party-contention case (locked by a different, still-live pod) and a new dedicated test reproducing lockAllDevices' actual multi-vendor call pattern. Signed-off-by: Aditya Raut <araut7798@gmail.com>
|
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)
📝 WalkthroughWalkthrough
ChangesNode lock reentrancy
Estimated code review effort: 2 (Simple) | ~10 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 |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
The reentrancy check added in the previous commit is a compound condition, ns == pods.Namespace && previousPodName == pods.Name. Existing tests only exercised the fully-false case (different namespace entirely) and the fully-true case (same pod, the reentrant scenario); nothing exercised same namespace with a different pod name, leaving codecov's patch coverage partial on that line. Add that case: a live pod in the same namespace, but a different name, holding the lock - which must still be treated as third-party contention, not reentrancy. Signed-off-by: Aditya Raut <araut7798@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/util/nodelock/nodelock_test.go (2)
71-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck all fixture creation errors.
The
Nodes().CreateandPods().Createresults are ignored in both conflict fixtures. If node creation fails,LockNodereturns a not-found error, sowantErr: truecan pass without exercising the lock-contention path. Fail the subtest when any fixture creation returns an error.Also applies to: 92-114
🤖 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 71 - 91, Check and handle the errors returned by every Nodes().Create and Pods().Create call in both conflict fixtures, including the fixture around the existing and additional conflict case. Make each subtest fail immediately when fixture creation fails, so wantErr: true validates lock contention rather than a missing-resource path.
275-300: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCreate the requesting pod before acquiring the nodelock.
LockNodechecks the previous lock owner pod via the fake client. Iftest-ns/multi-vendor-podis not created, the first call succeeds with a dangling lock, and the live ownership branch is not exercised.🤖 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 275 - 300, Update TestLockNodeReentrantSamePod to create test-ns/multi-vendor-pod in the fake Kubernetes client before the first LockNode call, and fail the test if creation fails. Keep the existing two LockNode calls unchanged so they exercise reentrant ownership by the live requesting pod.
🤖 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.
Outside diff comments:
In `@pkg/util/nodelock/nodelock_test.go`:
- Around line 71-91: Check and handle the errors returned by every
Nodes().Create and Pods().Create call in both conflict fixtures, including the
fixture around the existing and additional conflict case. Make each subtest fail
immediately when fixture creation fails, so wantErr: true validates lock
contention rather than a missing-resource path.
- Around line 275-300: Update TestLockNodeReentrantSamePod to create
test-ns/multi-vendor-pod in the fake Kubernetes client before the first LockNode
call, and fail the test if creation fails. Keep the existing two LockNode calls
unchanged so they exercise reentrant ownership by the live requesting pod.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af266436-75d2-4f53-a32f-ff799547f7be
📒 Files selected for processing (1)
pkg/util/nodelock/nodelock_test.go
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
pls add "fixes #2243" to the desc so it auto closes.
Addresses CodeRabbit review on PR Project-HAMi#2255: - Test_LockNode's "another pod" and "same namespace, different pod" cases ignored the Nodes().Create/Pods().Create fixture errors. If node creation silently failed, LockNode would return a not-found error instead, and wantErr: true would pass without actually exercising the lock-contention path it's meant to test. Fail the subtest via t.Fatalf on any fixture-creation error instead. - TestLockNodeReentrantSamePod never created the requesting pod ("multi-vendor-pod"/"test-ns") in the fake client, so the first LockNode call succeeded only because it was setting a fresh lock, not because anything verified a real, live pod. Create the pod before acquiring the lock so the test reflects lockAllDevices' actual runtime scenario, where the pod genuinely exists throughout. Signed-off-by: Aditya Raut <araut7798@gmail.com>
|
@adity1raut this handles taking the lock, but giving it back is still a problem. whichever device plugin finishes first deletes hami.io/mutex.lock while the other one is still working on the same pod, and the owner check passes for both since it's the same pod either way. wrote it up in #2243 under fix consideration. happy to take the release side as a separate pr if you want to keep this one to the acquire path. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adity1raut, Shouren 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 |
4 participants
/kind bug
What this PR does / why we need it:
lockAllDevices(pkg/scheduler/scheduler.go) callsnodelock.LockNodeonce per device vendor backend a pod requests resources from (device.GetDevices()is iterated per registered vendor), and every vendor writes to the same shared node annotation (hami.io/mutex.lock).LockNodehad no case for "this exact pod already holds the lock": the dangling-lock check only ran when the lock's namespace/name differed from the requesting pod, so a same-pod, non-expired lock fell straight through to the contention error. Concretely: a pod requesting resources from two or more HAMi-managed vendors in the same spec (e.g. one container asking fornvidia.com/gpu, another forcambricon.com/vmlu) would have its firstLockNodecall succeed and its second call immediately contend with its own still-valid lock — making that pod permanently unschedulable.This PR adds an explicit branch: if the existing lock's namespace/name match the calling pod and it hasn't expired,
LockNodetreats it as already acquired instead of erroring.It also fixes
Test_LockNode's "node has been locked" case, which had (accidentally) used the exact same pod identity as both the lock holder and the requester — so it was asserting the buggy contention behavior as correct. That case is now a genuine third-party-contention scenario (locked by a different, still-live pod), and a new dedicated test (TestLockNodeReentrantSamePod) reproduceslockAllDevices' actual multi-vendor call pattern.Which issue(s) this PR fixes:
Fixes #2243
Special notes for your reviewer:
Found this while reviewing other parts of the scheduler/monitor code for unrelated PRs; it's a distinct issue from any of my other open PRs.
Does this PR introduce a user-facing change?:
This PR was written with AI assistance (Claude Code), per the AI-assistance disclosure requirement in CONTRIBUTING.md.
Summary by CodeRabbit
Bug Fixes
Tests