fix(cambricon): prevent node lock leaks by reading from apiserver in ReleaseNodeLock - #2329
Conversation
|
Welcome @jianzhangbjz! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCambricon node lock acquisition now patches the lock annotation without relying on stale local state. Lock release fetches the current node, retries with refreshed resource versions, and handles concurrent lock removal. Fake-client tests cover these behaviors. ChangesCambricon node lock lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CambriconDevices
participant KubernetesAPIServer
participant ConcurrentActor
CambriconDevices->>KubernetesAPIServer: Fetch current node
CambriconDevices->>KubernetesAPIServer: Remove lock annotation
KubernetesAPIServer-->>CambriconDevices: Return update conflict
CambriconDevices->>KubernetesAPIServer: Re-fetch node
ConcurrentActor->>KubernetesAPIServer: Remove lock annotation
KubernetesAPIServer-->>CambriconDevices: Return node without lock
Possibly related issues
Possibly related PRs
Suggested reviewers: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/device/cambricon/device_test.go (1)
590-616: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a stale-caller regression case.
This test passes a Node whose annotations match the persisted Node. It does not verify the fresh-state behavior in
ReleaseNodeLock.Create a persisted Node with
DsmluLockTime. CallReleaseNodeLockwith a separate Node that has only the same name and no lock annotation. Then verify that the persisted lock annotation is removed.🤖 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_test.go` around lines 590 - 616, The ReleaseNodeLock test currently passes the persisted Node instead of a stale caller object, so it does not cover fresh-state behavior. Add a regression case that persists a Node with DsmluLockTime, invokes CambriconDevices.ReleaseNodeLock with a separate Node containing only the same name and no annotations, and verifies via a fresh API read that the persisted lock annotation was removed.
🤖 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.
Nitpick comments:
In `@pkg/device/cambricon/device_test.go`:
- Around line 590-616: The ReleaseNodeLock test currently passes the persisted
Node instead of a stale caller object, so it does not cover fresh-state
behavior. Add a regression case that persists a Node with DsmluLockTime, invokes
CambriconDevices.ReleaseNodeLock with a separate Node containing only the same
name and no annotations, and verifies via a fresh API read that the persisted
lock annotation was removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01310aee-cc66-463a-bd15-ae18708e105e
📒 Files selected for processing (2)
pkg/device/cambricon/device.gopkg/device/cambricon/device_test.go
|
dco check fails, missing signed-off-by. pls sign off your commit and push again |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/assign |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 37 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/lgtm cancel |
|
please sync with master to pass the UT |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Hi @archlitchi , I've updated it. Could you help approve it when you get a chance? Thanks! |
- setNodeLock patches the apiserver without updating the caller's in-memory Node, causing ReleaseNodeLock to read stale state and leak locks - the retry loop reused the same DeepCopy without re-fetching resourceVersion, causing 409 conflicts to never resolve - delete on the shared informer's annotation map raced with concurrent readers Signed-off-by: Jian Zhang <jiazha666@gmail.com>
|
Hi @mesutoezdil , I've updated it. Could you help have a review? Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, jianzhangbjz, 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
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fix three bugs in ReleaseNodeLock:
Which issue(s) this PR fixes:
Fixes #2251
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
Bug Fixes
Tests