fix(scheduler): copy node info into nodeManager instead of storing the caller's - #2504
Conversation
…e caller's RegisterFromNodeAnnotations builds nodeInfo around a *corev1.Node taken from nodeLister.List and hands it to addNode, which stored the pointer. Those objects belong to the shared informer, and cambricon's ReleaseNodeLock deletes an annotation from the node Bind fetched out of the same cache (pkg/device/cambricon/device.go:178). GetNode and ListNodes were already changed to deep copy on the way out (Project-HAMi#2333), but the write side kept a reference to the shared object, so their DeepCopy could run while that delete was in progress. go test --race reports it on master. Copy Node and the device lists when storing, on both the insert and the update path. Signed-off-by: Lakshya77089 <lakshyasharma7708@gmail.com>
📝 WalkthroughWalkthrough
ChangesNode state detachment
Estimated code review effort: 3 (Moderate) | ~20 minutes 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.
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/nodes_test.go`:
- Around line 517-541: Replace the legacy cambricon.com/dsmlu.lock annotation
key with the corresponding hami.io/ key in the nodes_test concurrency setup and
the Cambricon device implementation. Ensure both locations use the same updated
key; if legacy compatibility requires retaining the old key, define and document
that exception at the key’s declaration.
🪄 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: d1519808-0f5f-4937-afb4-32251d75f93b
📒 Files selected for processing (2)
pkg/scheduler/nodes.gopkg/scheduler/nodes_test.go
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:
|
|
/lgtm |
|
/lgtm |
FouoF
left a comment
FouoF
left a comment
There was a problem hiding this comment.
Re-approve to refresh GitHub review:approved search index for Tide.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FouoF, Lakshya77089 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 |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
nodeManager.addNodestored the*device.NodeInfoit was handed, including the*corev1.Nodeinside it:and on the update path assigned the caller's slices and node straight in:
The only caller is
RegisterFromNodeAnnotations, which buildsnodeInfoaround anode taken from
s.nodeLister.List(...)— an object owned by the shared informer.That would be harmless if nothing wrote to it, but something does.
Bindfetchesthe same node out of the same cache, and on the release path cambricon's
ReleaseNodeLockdeletes an annotation from it:Meanwhile
GetNodeandListNodesdeep copy the stored node on the Filter andBind goroutines.
go test ./pkg/scheduler/ --racereports the read inside(*Node).DeepCopyIntoagainst that delete.#2333 fixed the read side of this cache by making
GetNodereturn a deep copy.The write side kept a reference to the shared object, so the copy it makes can
still run while the object is being modified.
The fix
Copy
Nodeand the device lists when storing, on both the insert and the updatepath, reusing
device.DeepCopyDeviceInfoswhichGetNodeandListNodesalready use.
Tests
TestAddNodeCopiesSharedNodeObjectdrives the annotation delete on one goroutinewhile another calls
GetNode/ListNodes, which is the shape of the realinterleaving.
TestAddNodeStoresDetachedCopymutates everything the caller stillholds, on both the insert and the update path, and asserts the manager is
unaffected. Both fail on master with
DATA RACE, both pass here.Which issue(s) this PR fixes:
None filed — raising the fix directly.
Special notes for your reviewer:
Worth being explicit about one thing, since it changes how the bug reads: shared
informers replace objects in their store rather than mutating them in place, so
holding a lister pointer is not automatically a race. What makes this one real is
that HAMi mutates the object itself, in
ReleaseNodeLock. #2329 is open againstthat mutation. If it lands, this race goes quiet, but the manager would still be
holding a reference it does not own, and the next writer brings it back. The two
changes are worth having independently.
addNodenow costs a deep copy per node per registration cycle. That is the samecost
GetNodealready pays per call, and registration is far less frequent thanscheduling, so it should not be noticeable.
Not validated on real hardware. The change is confined to the scheduler's
in-memory node cache and is covered by unit tests, which CONTRIBUTING allows for
scheduler-scoped changes.
Does this PR introduce a user-facing change?:
AI assistance disclosure: this change was developed with Claude Code — finding
the race, the fix, and the tests. Flagging the extent up front per CONTRIBUTING.
Summary by CodeRabbit
Bug Fixes
Tests