fix(monitor): add missing lock in Observe to prevent data race - #2588
fix(monitor): add missing lock in Observe to prevent data race#2588Rickydama3 wants to merge 5 commits into
Conversation
Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: leodon33 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesContainer observation safety
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
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 `@cmd/vGPUmonitor/feedback_test.go`:
- Around line 163-169: Update TestObserve to hold lister.Lock() before invoking
Observe in a goroutine, then verify Observe cannot complete while the lock is
held and does complete after lister.UnLock(). Preserve the empty-lister setup
while making the test assert mutex contention and release behavior.
🪄 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: 456f0246-03e9-4323-a741-f28a981cde4d
📒 Files selected for processing (2)
cmd/vGPUmonitor/feedback.gocmd/vGPUmonitor/feedback_test.go
Codecov Report✅ All modified and coverable lines are covered by tests.
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:
|
Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
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 `@cmd/vGPUmonitor/feedback_test.go`:
- Around line 179-186: Add a deterministic synchronization hook in the Observe
test flow that signals immediately before the goroutine attempts the lister
lock, and wait for that signal before asserting contention. Replace the fixed
time.After-based readiness assumption around done with this boundary signal,
while retaining the lock-held check so removing lister.Lock() still causes the
test to fail.
🪄 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: e7b7ee3c-908a-4551-b375-f7937b5c0b07
📒 Files selected for processing (1)
cmd/vGPUmonitor/feedback_test.go
Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
| if observeTestHook != nil { | ||
| observeTestHook() | ||
| } | ||
| lister.Lock() |
There was a problem hiding this comment.
issue #2573 says pod add/update informer events call update() at the same time as observe(). this file only registers deletefunc, and that one only logs, it never touches containers. where does the real concurrent write come from today? the lock is still correct and safe to add either way, just checking the reproduction story is accurate.
There was a problem hiding this comment.
The issue description doesn't quite get the informer events right. Update() and Observe() just run sequentially on the ticker loop, so they don't race with each other.
The real race is between Observe() modifying c.Info in the background and Prometheus concurrently scraping the /metrics endpoint. Since Collect() already grabs the lister lock when responding to Prometheus scrapes, grabbing the same lock in Observe() properly synchronizes the writes.
There was a problem hiding this comment.
@mesutoezdil I've answered your question inline! Let me know if you need anything else from my end.
| if observeTestHook != nil { | ||
| observeTestHook() | ||
| } | ||
| lister.Lock() |
There was a problem hiding this comment.
pr #2311 hit a real deadlock adding a lock inside listcontainers, because metrics.go already holds the lock when it calls listcontainers. why does this lock here not hit the same issue?
There was a problem hiding this comment.
The issue in #2311 happened because metrics.go already held the lock. When it called ListContainers(), adding a lock inside there made the exact same code try to lock it twice, which causes a deadlock.
The lock here is safe because metrics.go never actually calls Observe(). Observe() only runs in its own separate background loop watchAndFeedback. Since they run totally separate from each other, they just wait their turn for the lock. No double-locking can happen here. @mesutoezdil hope you liked it
|
This is being closed because it does not comply with the contribution guidelines. |
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
I noticed a data race in
vGPUmonitorthat causes a panic.Observe()incmd/vGPUmonitor/feedback.goreadslister.ListContainers()without grabbing the lock, butUpdate()incudevshr.gomutates that exact same map via the pod informer.This just adds
lister.Lock()anddefer lister.UnLock()toObserve()to fix it, matching howmetrics.gosafely reads it. I also added a quick test infeedback_test.goto cover the new lines.Which issue(s) this PR fixes:
Fixes #2573
Special notes for your reviewer:
Nothing special, just a straightforward concurrency fix.
Does this PR introduce a user-facing change?: