fix(device-plugin): prevent mutex pass-by-value in device replication - #2652
fix(device-plugin): prevent mutex pass-by-value in device replication#2652Rickydama3 wants to merge 1 commit into
Conversation
Pass Device structs by pointer instead of by value across rm methods and device_map replication to prevent copying the underlying Mutex. Copying a Mutex by value silently spawns independent lock states, creating race conditions that bypass synchronization. This updates AlignedAllocationSupported, IsMigDevice, and GetUUID to use pointer receivers, and explicitly initializes replicatedDevice instead of shallow copying the struct in device_map.go. Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Rickydama3 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 |
|
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 (3)
📝 WalkthroughWalkthroughThe NVIDIA device manager now uses pointer receivers for ChangesDevice lock copy prevention
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This localized change prevents mutex state from being copied during device replication, with no actionable merge-blocking risk remaining beyond normal checks and review. 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 |
|
The vet warning is from protobuf's zero-size DoNotCopy marker, not a real lock, |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
What type of PR is this?
/kind bug
What this PR does / why we need it:
This fixes the
go vetwarnings regarding Mutex lock copying in thenvidiadeviceRM package. By passingDeviceby pointer instead of value, we ensure the embedded mutex is not silently cloned, which would otherwise defeat synchronization.AI Disclosure: I manually authored and reviewed this code, but I used an AI assistant to help me run
go vetacross the repository and draft the initial fix. The final implementation and understanding of the code is my own work.Which issue(s) this PR fixes:
Fixes #2651
Special notes for your reviewer:
Note:
make verifycurrently fails on themasterbranch due to a pre-existing syntax error in.golangci.yaml(Error: can't load config: the format is required), but standardgo vetpasses cleanly on these changes.Does this PR introduce a user-facing change?: