bug(monitor): Memory Leak and Unmapped Memory Hazard in Container Lister Cache Cleanup - #2511
bug(monitor): Memory Leak and Unmapped Memory Hazard in Container Lister Cache Cleanup#2511aniket866 wants to merge 2 commits into
Conversation
…aks and unmapped memory hazards Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aniket866 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 |
📝 WalkthroughWalkthroughThe change adds ChangesContainer mapping cleanup
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: 2
🧹 Nitpick comments (1)
pkg/monitor/nvidia/cudevshr_test.go (1)
468-496: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a failed-unmap test.
The test covers nil, empty, successful, and repeated calls. It does not cover a non-nil
syscall.Munmaperror. Add a deterministic failure case and assert the returned error and the retained cleanup state. Also testContainerLister.Updatewhen unmapping fails.🤖 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/monitor/nvidia/cudevshr_test.go` around lines 468 - 496, Extend Test_ContainerUsage_Unmap with a deterministic syscall.Munmap failure, asserting the returned error and that data and Info retain the cleanup state expected after failure. Add coverage for ContainerLister.Update using a ContainerUsage whose unmap fails, and assert the update propagates or handles that error according to the existing contract.
🤖 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/monitor/nvidia/cudevshr.go`:
- Line 290: Update loadCache’s invalid-header and unsupported-version cleanup
paths to handle the error returned by usage.Unmap instead of discarding it.
Preserve the existing cache-format error while logging or propagating the unmap
failure alongside it, and retain a retry or equivalent cleanup path when
unmapping remains pending.
- Around line 83-90: Update ContainerUsage.Unmap to return the syscall.Munmap
error before clearing data or Info, preserving the mapping for retries when
unmapping fails. In Update, retain the container in l.containers whenever
c.Unmap() returns an error. In loadCache validation cleanup paths, propagate or
handle usage.Unmap() errors instead of discarding them, while preserving
successful cleanup behavior.
---
Nitpick comments:
In `@pkg/monitor/nvidia/cudevshr_test.go`:
- Around line 468-496: Extend Test_ContainerUsage_Unmap with a deterministic
syscall.Munmap failure, asserting the returned error and that data and Info
retain the cleanup state expected after failure. Add coverage for
ContainerLister.Update using a ContainerUsage whose unmap fails, and assert the
update propagates or handles that error according to the existing contract.
🪄 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: 1b97a651-e0e4-46ab-8f16-29d10e5c3ccf
📒 Files selected for processing (2)
pkg/monitor/nvidia/cudevshr.gopkg/monitor/nvidia/cudevshr_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:
|
…rors Update ContainerUsage.Unmap to return the syscall.Munmap error before clearing data or Info, preserving the mapping for retries when unmapping fails. In Update, retain the container in l.containers whenever c.Unmap() returns an error. In loadCache validation cleanup paths, handle usage.Unmap() errors instead of discarding them. Add unit tests for unmap error handling and container retention. Signed-off-by: aniket866 <iamaniketkumarmaner@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/monitor/nvidia/cudevshr_test.go`:
- Around line 518-523: Update the “unmap failure preserves data and returns
error” test for ContainerUsage to initialize Info with a non-nil sentinel before
calling Unmap, then assert that the exact same Info value remains after the
error. Keep the existing data-preservation and error assertions.
- Around line 369-388: Update ContainerLister.Update so the stale directory
associated with an Unmap failure is retained, allowing subsequent updates to
discover and retry the mapping; do not remove dirName until Unmap succeeds.
Extend the “old stale dir with unmap error retains container” test to assert
that ctrDir still exists after the failed update.
🪄 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: fe1670a1-b126-4f30-83d0-9d6a7e7cce9b
📒 Files selected for processing (2)
pkg/monitor/nvidia/cudevshr.gopkg/monitor/nvidia/cudevshr_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/monitor/nvidia/cudevshr.go
|
Thanks for investigating the monitor cache lifecycle. The stated root cause does not match current master: the existing deletion path already calls syscall.Munmap(c.data). The proposed wrapper primarily changes what remains cached when unmap fails, without a reproduction showing that the current behavior leaks or accesses unmapped memory. Because the premise is not established and the failure semantics would change, we are closing this PR. Please reopen only with a current-head reproducer and an explicit ownership contract for unmap failure. |
2 participants
Closes #2509
bug(monitor): Memory Leak and Unmapped Memory Hazard in Container Lister Cache Cleanup
/tmp/vgpu/containers/are removed from the filesystem. However,ContainerLister.Update()deletes entries froml.containerswithout explicitly invokingsyscall.Munmap()on the backing mmap'd byte slice.pkg/monitor/nvidia/cudevshr.go(inContainerLister.Update())syscall.Mmap(). In dense environments with high container turnover, unmapped memory regions remain allocated in RSS, causing a steady memory leak invGPUmonitor.Close()orUnmap()method toContainerUsagethat callssyscall.Munmap(c.data). Call this method insideUpdate()prior to removing stale container entries froml.containers.Before vs. After Architecture
flowchart LR subgraph Before["Before: Unmapped Memory Leak Hazard"] direction TB B1["Pod Termination Event"] B2["K8s Deletes Shared Dir /tmp/vgpu/containers/pod_ctr"] B3["Update Loop Scans Filesystem"] B4["Detect Missing Directory for PodUID"] B5["Execute delete(l.containers, key)"] B6["Go Pointer Removed from Map Cache"] B7["CRITICAL HAZARD: c.data NOT Unmapped"] B8["OS Virtual Memory Pages Retained in RSS"] B9["Continuous Process Memory Leak"] B1 --> B2 B2 --> B3 B3 --> B4 B4 --> B5 B5 --> B6 B6 --> B7 B7 --> B8 B8 --> B9 end Before ==>|Explicit syscall.Munmap Implementation| After subgraph After["After: Explicit Munmap & Safe Cleanup"] direction TB A1["Pod Termination Event"] A2["K8s Deletes Shared Dir /tmp/vgpu/containers/pod_ctr"] A3["Update Loop Scans Filesystem"] A4["Detect Missing Directory for PodUID"] A5["Retrieve ContainerUsage Reference"] A6["Invoke c.Unmap() syscall.Munmap(c.data)"] A7["OS Page Table Entry Released Safely"] A8["Set c.data = nil & c.Info = nil"] A9["Execute delete(l.containers, key)"] A10["Zero Memory Leak & Clean Cache Removal"] A1 --> A2 A2 --> A3 A3 --> A4 A4 --> A5 A5 --> A6 A6 --> A7 A7 --> A8 A8 --> A9 A9 --> A10 end classDef danger fill:#fee2e2,stroke:#ef4444,stroke-width:2px,color:#991b1b; classDef success fill:#dcfce7,stroke:#22c55e,stroke-width:2px,color:#166534; classDef neutral fill:#f3f4f6,stroke:#4b5563,stroke-width:1.5px,color:#1f2937; class B7,B8,B9 danger; class A6,A7,A10 success; class B1,B2,B3,B4,B5,B6,A1,A2,A3,A4,A5,A8,A9 neutral;Workflow & Component Diagram
graph TD A["ContainerLister.Update() Triggered"] --> B["Acquire l.mutex.Lock()"] B --> C["Read Physical Directory Entries: os.ReadDir(/tmp/vgpu/containers)"] C --> D["Fetch Active Pod List from K8s InformerCache"] D --> E["Build Set of Valid Active PodUIDs"] E --> F["Iterate Registered Cache Map: l.containers"] F --> G{"Is Container PodUID in Valid Active Set?"} G -- "Yes (Active)" --> H["Keep Entry in l.containers Map"] G -- "No (Terminated)" --> I{"Check Resync Window TTL Expiry"} I -- "Not Expired" --> J["Retain Container Temporarily"] I -- "Expired" --> K["Extract ContainerUsage Pointer"] K --> L["Check c.data Slice Non-Nil & Mapped"] L --> M["Call syscall.Munmap(c.data)"] M --> N{"Munmap Succeeded?"} N -- "Success" --> O["Clear Pointer c.data = nil"] N -- "Error" --> P["Log Klog Warning with Err Details"] O & P --> Q["Execute delete(l.containers, key)"] Q --> R["Log Info: Purged Container Cache & Unmapped Memory"] H & J & R --> S["Release l.mutex.Unlock()"]Summary by CodeRabbit
Bug Fixes
Tests