fix(cambricon): replace hand-rolled node lock with nodelock delegation - #2252
fix(cambricon): replace hand-rolled node lock with nodelock delegation#2252manmathbh wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: manmathbh 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 |
📝 WalkthroughWalkthroughCambricon node locking now delegates to the shared ChangesCambricon node locking
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CambriconDevice
participant nodelock
participant KubernetesAPI
CambriconDevice->>CambriconDevice: Check MLU resource request
CambriconDevice->>nodelock: LockNode or ReleaseNodeLock
nodelock->>KubernetesAPI: Create or clear shared node-lock annotation
CambriconDevice->>KubernetesAPI: Remove legacy DSMLU annotation
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Replace the custom lock logic in Cambricon with pkg/util/nodelock, matching every other backend (NVIDIA, Ascend, etc.). The old implementation had three bugs: 1) setNodeLock patched the apiserver but never wrote back to the callers Node, so ReleaseNodeLock could not find the annotation and silently returned nil without removing the lock. 2) The retry loop reused the same object every iteration, making it unable to clear 409 conflicts. 3) delete(n.Annotations, DsmluLockTime) mutated a shared informer object without a deep copy, causing a data race. Fixes Project-HAMi#2251. Signed-off-by: Manmath Hatte <manmathcode@gmail.com> Signed-off-by: Manmath Hatte <manmathcode@gmail.com>
bc26c24 to
8dad8a2
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes Cambricon MLU node-locking by removing its custom annotation/patch/retry implementation and delegating lock acquisition/release to the shared pkg/util/nodelock infrastructure used by other backends, addressing the failure modes described in #2251.
Changes:
- Replaced Cambricon’s hand-rolled node lock logic with
nodelock.LockNode/nodelock.ReleaseNodeLock. - Removed the legacy
cambricon.com/dsmlu.lockconstant and related patch/update retry code paths. - Updated Cambricon unit tests to cover the new nodelock-based behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/device/cambricon/device.go | Switches Cambricon’s node locking to the shared nodelock implementation and drops the old lock annotation code. |
| pkg/device/cambricon/device_test.go | Reworks locking tests to validate the new shared lock annotation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DsmluProfile = "CAMBRICON_DSMLU_PROFILE" | ||
| DsmluResourceAssigned = "CAMBRICON_DSMLU_ASSIGNED" | ||
| retry = 5 | ||
| NodeLockMLU = "hami.io/mutex.lock" | ||
| ) |
| config := CambriconConfig{ | ||
| ResourceCountName: MLUResourceCount, | ||
| ResourceMemoryName: MLUResourceMemory, | ||
| ResourceCoreName: MLUResourceCores, | ||
| } |
| updated, err := client.KubeClient.CoreV1().Nodes().Get(context.Background(), "test-node", metav1.GetOptions{}) | ||
| assert.NoError(t, err) | ||
| _, ok := updated.Annotations[nodelock.NodeLockKey] | ||
| assert.Equal(t, ok, tt.hasLock) |
| } | ||
| updated, err := client.KubeClient.CoreV1().Nodes().Get(context.Background(), "test-node", metav1.GetOptions{}) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, updated.Annotations[nodelock.NodeLockKey], "") |
![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 (1)
pkg/device/cambricon/device.go (1)
87-113: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the duplicated MLU-resource check into a shared helper.
LockNode(lines 88-94) andReleaseNodeLock(lines 102-108) contain the identical loop that checks whether any container inp.Spec.Containersrequests MLU resources. Extract this into a private helper, for examplehasMLURequest(p *corev1.Pod) bool, and call it from both methods. This avoids the two lock paths silently diverging if one is edited without the other, which was part of the pattern behind the original locking bugs.♻️ Proposed refactor
+func (dev *CambriconDevices) hasMLURequest(p *corev1.Pod) bool { + for _, val := range p.Spec.Containers { + if dev.GenerateResourceRequests(&val).Nums > 0 { + return true + } + } + return false +} + func (dev *CambriconDevices) LockNode(n *corev1.Node, p *corev1.Pod) error { - found := false - for _, val := range p.Spec.Containers { - if (dev.GenerateResourceRequests(&val).Nums) > 0 { - found = true - break - } - } - if !found { + if !dev.hasMLURequest(p) { return nil } return nodelock.LockNode(n.Name, NodeLockMLU, p) } func (dev *CambriconDevices) ReleaseNodeLock(n *corev1.Node, p *corev1.Pod) error { - found := false - for _, val := range p.Spec.Containers { - if (dev.GenerateResourceRequests(&val).Nums) > 0 { - found = true - break - } - } - if !found { + if !dev.hasMLURequest(p) { return nil } return nodelock.ReleaseNodeLock(n.Name, NodeLockMLU, p, false) }🤖 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/device/cambricon/device.go` around lines 87 - 113, Extract the duplicated container scan from LockNode and ReleaseNodeLock into a private helper such as hasMLURequest(p *corev1.Pod) bool, preserving the existing GenerateResourceRequests check and early return behavior. Replace both methods’ local found-loop logic with calls to this shared helper, while leaving their respective lock and release operations unchanged.
🧹 Nitpick comments (1)
pkg/device/cambricon/device.go (1)
43-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused Cambricon node-lock constant.
nodelock.SetNodeLockandnodelock.ReleaseNodeLockonly read/writeNodeLockKey; they ignore thelocknameargument, and Cambricon passesNodeLockMLUonly asnil-value context. Usenodelock.NodeLockKeydirectly, or pass the constant through a shared backend constant definition with a comment if the literal must remain local.🤖 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/device/cambricon/device.go` around lines 43 - 47, Remove the unused NodeLockMLU constant from the Cambricon constants block and update any Cambricon node-lock call sites to use nodelock.NodeLockKey directly instead. Preserve the existing behavior of nodelock.SetNodeLock and nodelock.ReleaseNodeLock, which already ignore the lockname argument.
🤖 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/device/cambricon/device.go`:
- Around line 87-113: Extract the duplicated container scan from LockNode and
ReleaseNodeLock into a private helper such as hasMLURequest(p *corev1.Pod) bool,
preserving the existing GenerateResourceRequests check and early return
behavior. Replace both methods’ local found-loop logic with calls to this shared
helper, while leaving their respective lock and release operations unchanged.
---
Nitpick comments:
In `@pkg/device/cambricon/device.go`:
- Around line 43-47: Remove the unused NodeLockMLU constant from the Cambricon
constants block and update any Cambricon node-lock call sites to use
nodelock.NodeLockKey directly instead. Preserve the existing behavior of
nodelock.SetNodeLock and nodelock.ReleaseNodeLock, which already ignore the
lockname argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b1d9ccda-e9a2-47ed-a70e-e0b0c96795b9
📒 Files selected for processing (2)
pkg/device/cambricon/device.gopkg/device/cambricon/device_test.go
mesutoezdil
left a comment
mesutoezdil
left a comment
There was a problem hiding this comment.
no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice
| DsmluProfile = "CAMBRICON_DSMLU_PROFILE" | ||
| DsmluResourceAssigned = "CAMBRICON_DSMLU_ASSIGNED" | ||
| retry = 5 | ||
| NodeLockMLU = "hami.io/mutex.lock" |
There was a problem hiding this comment.
Aware, but the old cambricon.com/dsmlu.lock was already a single
annotation on the whole node, so the contention surface didn't change.
It was just invisible because it wasn't sharing the key with anyone.
Now at least the central fix in #2243 covers it. Happy to leave a note
there.
Used Copilot for autocomplete while writing the change, no generated blocks. |
|
@manmathbh the body says an old cambricon.com/dsmlu.lock times out after 5 minutes, but that timeout belongs to hami.io/mutex.lock. nothing reads or deletes dsmlu.lock once this lands, so whatever is sitting on a node at upgrade time stays there for good. does cambricon's device plugin read that key? the cambricon.com/ prefix and the CAMBRICON_DSMLU_* envs right below it make me think it might, but I havent looked at the plugin side. would also wait for #2255 before this merges, otherwise a pod asking for mlu and nvidia takes the lock on the first call and then blocks on it on the second |
Remove NodeLockMLU constant, use nodelock.NodeLockKey directly. Extract hasMLURequest helper to deduplicate container loop. Add cleanupLegacyLock to strip orphaned cambricon.com/dsmlu.lock annotations at upgrade time. Fix test assertion arg order and empty-string check. Signed-off-by: Manmath Hatte <manmathcode@gmail.com>
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 (1)
pkg/device/cambricon/device.go (1)
29-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGroup local imports after external imports.
Lines 29-30 place
github.com/Project-HAMi/HAMi/...imports before Kubernetes imports. Rungoimports -local github.com/Project-HAMi/HAMiand place the local imports last.Proposed import order
- "github.com/Project-HAMi/HAMi/pkg/util/client" - "github.com/Project-HAMi/HAMi/pkg/util/nodelock" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + + "github.com/Project-HAMi/HAMi/pkg/util/client" + "github.com/Project-HAMi/HAMi/pkg/util/nodelock"As per coding guidelines, “Go import blocks must be grouped as standard library imports first, then external imports, then
github.com/Project-HAMi/HAMi/...imports.”🤖 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/device/cambricon/device.go` around lines 29 - 35, Reorder the imports in device.go so Kubernetes and other external imports appear before the github.com/Project-HAMi/HAMi/... imports, with the local client and nodelock imports in the final group; match goimports -local github.com/Project-HAMi/HAMi ordering.Source: Coding guidelines
🤖 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/device/cambricon/device.go`:
- Around line 29-35: Reorder the imports in device.go so Kubernetes and other
external imports appear before the github.com/Project-HAMi/HAMi/... imports,
with the local client and nodelock imports in the final group; match goimports
-local github.com/Project-HAMi/HAMi ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c27ae495-ea72-4d72-9333-48436e3a7d59
📒 Files selected for processing (2)
pkg/device/cambricon/device.gopkg/device/cambricon/device_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/device/cambricon/device_test.go
|
@iemAnshuman @mesutoezdil Thanks for the catches! I've pushed a commit addressing the feedback: Added cleanupLegacyLock() to strip the orphaned dsmlu.lock via JSON patch so nodes don't stay locked after an upgrade. |
also I checked the current Cambricon device plugin code and found a separate lifecycle mismatch. in DynamicSmlu mode, its allocation, startup, and pod cleanup paths still remove only cambricon.com/dsmlu.lock; they never release hami.io/mutex.lock. HAMi doesnt release the node lock after a successful Bind, so a Cambricon only DynamicSmlu pod can leave the new shared lock behind and block later accelerator pods on that node until the lock times out at five minutes by default. could we either keep the legacy key until the plugin supports the shared lock, or land a coordinated, owner safe plugin change first? also a focused test proving that a successful DynamicSmlu allocation releases the exact key written by LockNode() would be useful. The legacy cleanup helps with upgrades, but it doesnt fix this release path mismatch. |
|
@iemAnshuman You're right, this is a real lifecycle mismatch that can't be fixed solely on the scheduler side. nodelock.SetNodeLock and ReleaseNodeLock both hardcode hami.io/mutex.lock (the lockname parameter is accepted but unused), so there's no way to keep using cambricon.com/dsmlu.lock through the shared infra. The Cambricon device plugin's DynamicSmlu paths need to release hami.io/mutex.lock after allocation. Since the plugin isn't in this repo, two options:
Preference? I lean toward option 2; it avoids keeping dead code around. Happy to help draft the plugin-side change. |
|
You know the rule from previous prs. Reminder: Answers must be written by human being. You can view the relevant rule here. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Replaced Cambricon's hand-rolled node lock with pkg/util/nodelock. Every
other backend already delegates to nodelock — Cambricon was the only one
rolling its own.
Three bugs in the old code:
setNodeLock patched the apiserver but did not write back to the caller's
Node. ReleaseNodeLock read the stale in-memory object, found no lock
annotation, logged "Node lock not set", returned nil. Lock stayed.
Retry loop sent the same object every attempt. Same resourceVersion,
no backoff. Could not clear 409 conflicts.
delete(n.Annotations, DsmluLockTime) on the shared informer's Node
object, no deep copy. Race with Filter reading the same map.
Which issue(s) this PR fixes:
Fixes #2251
Special notes for your reviewer:
Old DsmluLockTime (cambricon.com/dsmlu.lock) replaced with the shared
hami.io/mutex.lock annotation. Any stale lock from a previous version
expires via the 5-minute nodelock timeout.
Does this PR introduce a user-facing change?:
Cambricon MLU node locking uses the shared nodelock infrastructure.
Old lock annotations expire naturally (5-minute TTL).
Summary by CodeRabbit
Bug Fixes
Tests