fix: return deep copies in GetScheduledPods to prevent data races - #2164
fix: return deep copies in GetScheduledPods to prevent data races#2164Gaurav-205 wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Gaurav-205 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 |
|
Welcome @Gaurav-205! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
📝 WalkthroughWalkthrough
ChangesScheduled pod isolation
Estimated code review effort: 2 (Simple) | ~10 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.
🧹 Nitpick comments (1)
pkg/device/pod_test.go (1)
556-561: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the embedded Pod copy as well.
The test verifies
NodeIDandDevices, but notPodInfo.Pod. Mutate a field such asscheduled["uid-1"].Pod.Nameand assert the cached pod remains unchanged; otherwise a regression to shallow-copying the embeddedcorev1.Podwould go undetected.Suggested assertion
scheduled["uid-1"].NodeID = "mutated" scheduled["uid-1"].Devices["dev"][0][0].UUID = "mutated" +scheduled["uid-1"].Pod.Name = "mutated" inner := pm.pods[k8stypes.UID("uid-1")] assert.Equal(t, "node-1", inner.NodeID) assert.Equal(t, "GPU-0", inner.Devices["dev"][0][0].UUID) +assert.Equal(t, "p", inner.Pod.Name)🤖 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/pod_test.go` around lines 556 - 561, Extend the copy-isolation test around scheduled["uid-1"] to mutate a field on its embedded Pod, such as Pod.Name, and assert the cached entry in pm.pods retains the original Pod.Name alongside the existing NodeID and Devices checks.
🤖 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/pod_test.go`:
- Around line 556-561: Extend the copy-isolation test around scheduled["uid-1"]
to mutate a field on its embedded Pod, such as Pod.Name, and assert the cached
entry in pm.pods retains the original Pod.Name alongside the existing NodeID and
Devices checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ed79512-4e75-49c1-805e-e1ee675b4fc1
📒 Files selected for processing (2)
pkg/device/pod_test.gopkg/device/pods.go
928e598 to
f25cd8b
Compare
Signed-off-by: Gaurav-205 <gauravkhandelwal205@gmail.com>
Eshiv-Pandey
left a comment
Eshiv-Pandey
left a comment
There was a problem hiding this comment.
Ahh... i mean it looks good to me from a code-read perspective! Returning copied PodInfo values here makes sense because the metrics collector reads this data after the pod manager lock is released.
Small follow-up suggestion: CustomInfo is still only copied at the top level, so nested values could remain shared. That does not seem to block this PR since the metrics path does not use CustomInfo.
Now let's see what the maintainers think!
There was a problem hiding this comment.
also the description doesnt follow the pr template, no "what type of pr is this?" block or /kind line, and no ai disclosure. if any ai tool was used it has to be disclosed, see https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice
| podsCopy := make(map[k8stypes.UID]*PodInfo, podCount) | ||
| maps.Copy(podsCopy, m.pods) | ||
| for uid, pod := range m.pods { | ||
| podsCopy[uid] = pod.DeepCopy() |
There was a problem hiding this comment.
ContainerDevice.DeepCopy just does maps.Copy on CustomInfo map[string]any, and metax stores pod.Annotations in there at sdevice.go:465, so what does the copy share w/ the original on a metax node?
There was a problem hiding this comment.
You are right. ContainerDevice.DeepCopy previously only cloned the outer CustomInfo map, leaving nested maps like Metax's Pod.Annotations shared with the original.
In the latest commit, GetScheduledPods uses DeepCopyForMetrics(), which intentionally sets CustomInfo = nil on snapshot devices because cmd/scheduler/metrics.go only consumes scalar allocation fields (UUID, Type, Usedmem, Usedcores). This completely avoids nested map aliasing without introducing an unsafe or complex generic deep-copy implementation.
There was a problem hiding this comment.
You are right. ContainerDevice.DeepCopy previously only cloned the outer CustomInfo map, leaving nested maps like Metax's Pod.Annotations shared with the original.
In the latest commit, GetScheduledPods uses DeepCopyForMetrics(), which intentionally sets CustomInfo = nil on snapshot devices because cmd/scheduler/metrics.go only consumes scalar allocation fields (UUID, Type, Usedmem, Usedcores). This completely avoids nested map aliasing without introducing an unsafe or complex generic deep-copy implementation.
Reminder: Answers must be written by human being. You can view the relevant rule here.
https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#contribution-gates
"4. Review replies. The reply you post must be written by you and must address the specific point raised. Verbatim or canned AI replies, or replies that do not engage the comment, lead to the PR being closed."
There was a problem hiding this comment.
Sorry @mesutoezdil. It was literally my first time contributing to an open-source project. I just wanted everything to be perfect. I understand my mistake now, and from now on I'll make sure all my answers and contributions are written by me
| // Return a deep copy of the pods map and its PodInfo values to avoid race conditions. | ||
| podsCopy := make(map[k8stypes.UID]*PodInfo, podCount) | ||
| maps.Copy(podsCopy, m.pods) | ||
| for uid, pod := range m.pods { |
There was a problem hiding this comment.
the only caller is cmd/scheduler/metrics.go:318 and it reads Namespace, Name, NodeID and Devices only, so cloning the whole corev1.Pod spec and status per pod per scrape is a lot of garbage for nothing, did u consider copying just PodInfo w/ the pod ptr left alone?
There was a problem hiding this comment.
Excellent point. I revised GetScheduledPods to construct a metrics-specific PodInfo snapshot: it retains the original Pod pointer for name/namespace identity checks while copying NodeID and cloning device allocations via DeepCopyForMetrics(). This avoids generating garbage by copying the full corev1.Pod spec and status on every Prometheus scrape cycle.
Signed-off-by: Gaurav-205 <gauravkhandelwal205@gmail.com>
Thanks for pointing this out. I have updated the PR description to follow the official repository template, added the /kind bug line, and included the required AI assistance disclosure notice. |
f25cd8b to
e62ae08
Compare
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/device/pods.go`:
- Around line 213-220: Update ContainerDevice.DeepCopy in pkg/device/pods.go to
preserve CustomInfo via an isolated copy, while leaving DeepCopyForMetrics
redacted. In pkg/device/pod_test.go lines 537-545, assert generic copies retain
CustomInfo without sharing mutable data, and preserve the existing nil assertion
for metrics snapshots.
🪄 Autofix (Beta)
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: c6a06fe9-4798-4eab-8a7c-a3c1355c0ade
📒 Files selected for processing (3)
cmd/scheduler/metrics.gopkg/device/pod_test.gopkg/device/pods.go
💤 Files with no reviewable changes (1)
- cmd/scheduler/metrics.go
| func (c ContainerDevice) DeepCopy() ContainerDevice { | ||
| dup := ContainerDevice{ | ||
| return ContainerDevice{ | ||
| Idx: c.Idx, | ||
| UUID: c.UUID, | ||
| Type: c.Type, | ||
| Usedmem: c.Usedmem, | ||
| Usedcores: c.Usedcores, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep CustomInfo omission limited to metrics snapshots.
ContainerDevice.DeepCopy() is also used by PodInfo.DeepCopy() through PodDevices.DeepCopy(), so Line 213 now silently discards metadata for every generic copy, not only GetScheduledPods(). DeepCopyForMetrics() already provides the intended redaction.
pkg/device/pods.go#L213-L220: restoreCustomInfopreservation/isolation in the generic copy path; retain its omission only inDeepCopyForMetrics().pkg/device/pod_test.go#L537-L545: assert generic copies preserveCustomInfo; keep the nil assertion in the metrics-snapshot test.
📍 Affects 2 files
pkg/device/pods.go#L213-L220(this comment)pkg/device/pod_test.go#L537-L545
🤖 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/pods.go` around lines 213 - 220, Update ContainerDevice.DeepCopy
in pkg/device/pods.go to preserve CustomInfo via an isolated copy, while leaving
DeepCopyForMetrics redacted. In pkg/device/pod_test.go lines 537-545, assert
generic copies retain CustomInfo without sharing mutable data, and preserve the
existing nil assertion for metrics snapshots.
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
GetScheduledPodsreturned pointers to PodManager-ownedPodInfovalues afterreleasing its lock. The metrics collector reads the returned device allocations
outside that lock, so concurrent updates could race with collection.
This PR returns a metrics-focused snapshot: it copies PodInfo allocation data
and scalar device fields while retaining the existing Pod pointer for identity
fields.
CustomInfois intentionally omitted because the metrics collectordoes not consume it.
Which issue(s) this PR fixes:
Fixes #2163
Special notes for your reviewer:
The snapshot intentionally avoids
Pod.DeepCopy()and avoids copyingCustomInfo; the only caller reads Namespace, Name, NodeID, UUID, Usedmem,and Usedcores.
Does this PR introduce a user-facing change?:
No.
AI assistance disclosure:
I used Codex to help audit the code path and plan validation. I reviewed,
implemented, and verified the final change myself.
Summary by CodeRabbit