fix(monitor): return locked snapshot from ListContainers to prevent race with Update - #2311
fix(monitor): return locked snapshot from ListContainers to prevent race with Update#2311Aamod007 wants to merge 2 commits into
Conversation
|
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:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesContainer snapshot safety
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
|
please sign-off your commit |
d9275b4 to
0162700
Compare
|
@archlitchi Done — amended the commit with sign-off and force-pushed. Thanks for the review! |
57d6a0f to
4cfb3a6
Compare
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 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
please resolve these conflicts, we've dealt with unavailable directory name in #2371 |
|
/assign |
…ace with Update ListContainers returned the internal containers map without holding the mutex, while Update() concurrently mutates the map and munmaps entries under the lock (called every 5s from the feedback loop). The metrics collector and Observe() iterate that map at the same time, which is a data race and can dereference a shared-memory region after Munmap. Fix ListContainers to copy the map under the lock and return the snapshot. Also harden Update() against container directory names without an underscore, which previously panicked with index out of range when deriving the container name. Signed-off-by: Aamod007 <aamodkumar2006@gmail.com>
Signed-off-by: Aamod007 <aamodkumar2006@gmail.com>
4cfb3a6 to
d4c84f7
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Aamod007 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 |
|
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. |
|
The Fix : Collector then calls |
|
Fix : apply this diff --git a/cmd/vGPUmonitor/metrics.go b/cmd/vGPUmonitor/metrics.go
index f943a63..317dfc6 100644
--- a/cmd/vGPUmonitor/metrics.go
+++ b/cmd/vGPUmonitor/metrics.go
@@ -380,7 +380,9 @@ func (cc ClusterManagerCollector) collectPodAndContainerInfo(ch chan<- prometheu
cc.ClusterManager.containerLister.Lock()
defer cc.ClusterManager.containerLister.UnLock()
- containers := cc.ClusterManager.containerLister.ListContainers()
+ // We already hold the lister lock above, so use the non-locking accessor;
+ // ListContainers() would re-lock the non-reentrant mutex and deadlock.
+ containers := cc.ClusterManager.containerLister.Containers()
containerMap := make(map[string][]*nvidia.ContainerUsage) // podUID -> containers
for _, c := range containers {
if c.Info != nil && c.PodUID != "" {
diff --git a/pkg/monitor/nvidia/cudevshr.go b/pkg/monitor/nvidia/cudevshr.go
index 9b5b461..b6e567c 100644
--- a/pkg/monitor/nvidia/cudevshr.go
+++ b/pkg/monitor/nvidia/cudevshr.go
@@ -165,6 +165,14 @@ func (l *ContainerLister) ListContainers() map[string]*ContainerUsage {
return snapshot
}
+// Containers returns the live internal map without locking. The caller must
+// already hold Lock(); the metrics collector does so to keep the mmap'd
+// ContainerUsage.Info alive across a scrape, so it cannot use the locking
+// ListContainers() above without deadlocking on the non-reentrant mutex.
+func (l *ContainerLister) Containers() map[string]*ContainerUsage {
+ return l.containers
+}
+
func (l *ContainerLister) Clientset() *kubernetes.Clientset {
return l.clientset
} |
|
Thanks for investigating the container-map race. The current implementation introduces a deterministic deadlock: the metrics collector already holds this non-reentrant mutex when it calls ListContainers(), and the new code attempts to acquire the same mutex again. The unit-test failure reflects this regression. The unrelated directory fix included in the branch has already been merged through #2371. We are closing this implementation. If the underlying mmap lifetime race still exists, please open a new, focused proposal that defines lock ownership and includes a regression test for both concurrent update safety and scrape completion. |
4 participants
What type of PR is this?
/kind bug
What this PR does / why we need it
ListContainers()returned the internalcontainersmap without holding the lister mutex, whileUpdate()— called every 5s from the feedback loop — mutates that map andMunmaps removed entries under the lock. The metrics collector andObserve()iterate the same map concurrently, which is:-race), andc.Infofor an entry whose shared-memory regionUpdate()just unmapped — crashing the entire vGPUmonitor DaemonSet pod.This PR makes
ListContainers()copy the map under the mutex and return the snapshot. All existing callers only range over the result, so the snapshot is a drop-in replacement.It also hardens
Update()against container directory names without an underscore: previouslystrings.Split(entry.Name(), "_")[1]panicked with index out of range on any stray directory in the hook path; now such entries are skipped with a warning.Note: the snapshot shrinks the munmap race window to entries removed mid-scrape; fully eliminating it would require refcounting the mmap lifetime, which I left as a possible follow-up given the current behavior is always racy.
Which issue(s) this PR fixes
Fixes #2309
Tests
Test_ListContainers_snapshot— snapshot is isolated from subsequent internal-map mutationTest_ListContainers_concurrentUpdate— drivesUpdate()andListContainers()concurrently; fails under-raceon the previous implementationTest_ContainerLister_Update— new subtest: dirname without underscore is skipped, not panicked onAI assistance disclosure
Per CONTRIBUTING.md: AI tooling was used to help draft this change; I reviewed and verified the code, race analysis, and tests.
Summary by CodeRabbit
Bug Fixes
Tests