Skip to content

fix(monitor): prevent fatal data race during feedback observation - #2574

Closed
Rickydama3 wants to merge 1 commit into
Project-HAMi:masterfrom
Rickydama3:fix-vgpu-datarace
Closed

fix(monitor): prevent fatal data race during feedback observation#2574
Rickydama3 wants to merge 1 commit into
Project-HAMi:masterfrom
Rickydama3:fix-vgpu-datarace

Conversation

@Rickydama3

@Rickydama3 Rickydama3 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR fixes a critical data race in the HAMi-vGPUmonitor that causes the monitor to crash with a fatal concurrent map iteration and map write runtime panic.

In cmd/vGPUmonitor/feedback.go, the Observe() function fetches the containers map via lister.ListContainers() and iterates over it in a background ticker loop. However, it was missing a lock acquisition before doing so. At the same time, the Kubernetes Pod informer events (handled by podInformer.AddEventHandler in pkg/monitor/nvidia/cudevshr.go) concurrently trigger lister.Update(), which explicitly locks the mutex and modifies the exact same map.

Iterating over a map without a lock while another goroutine modifies it triggers a fatal runtime panic in Go. This PR adds the missing lister.Lock() and defer lister.UnLock() to Observe(), safely synchronizing it with the informer events. This strictly mirrors the correct locking behavior already implemented for the exact same map in cmd/vGPUmonitor/metrics.go.

Which issue(s) this PR fixes:
Fixes #2573

Special notes for your reviewer:
This is a standard Go concurrency bug fix. The lock is only held during the fast in-memory map iteration inside Observe(), so there is no risk of deadlock or blocking the informer goroutines for a significant amount of time.

Does this PR introduce a user-facing change?:

None

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->

## Summary by CodeRabbit

* **Bug Fixes**
  * Improved container state monitoring reliability by preventing concurrent updates during observation processing.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Aug 11, 2026
@hami-robot

hami-robot Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: leodon33
Once this PR has been reviewed and has the lgtm label, please assign wawa0210 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hami-robot
hami-robot Bot requested review from mesutoezdil and wawa0210 August 11, 2026 08:09
@hami-robot hami-robot Bot added the size/XS label Aug 11, 2026
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5974166d-72ea-4180-9b9e-a746c5b4642a

📥 Commits

Reviewing files that changed from the base of the PR and between 634bf2b and 0fbbc17.

📒 Files selected for processing (1)
  • cmd/vGPUmonitor/feedback.go

📝 Walkthrough

Walkthrough

Observe now holds the container lister lock while it lists and updates container state. This synchronizes observation with concurrent container lister updates.

Changes

vGPU monitor concurrency

Layer / File(s) Summary
Lock container observation
cmd/vGPUmonitor/feedback.go
Observe acquires the lister lock before processing containers and defers unlocking after processing completes.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: mesutoezdil

Poem

A rabbit guards the map tonight,
With locks held firm and timing right.
No racing paws, no startled cheer,
The monitor hops safely here.
🐇🔒

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the monitor race fix and matches the primary change.
Linked Issues check ✅ Passed The change adds mutex protection around container iteration, satisfying issue #2573 and matching the required concurrency-safe pattern.
Out of Scope Changes check ✅ Passed The changes are limited to locking in feedback observation and contain no unrelated scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/vGPUmonitor/feedback.go 0.00% 2 Missing ⚠️
Flag Coverage Δ
unittests 62.35% <0.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cmd/vGPUmonitor/feedback.go 20.93% <0.00%> (-0.50%) ⬇️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mesutoezdil

Copy link
Copy Markdown
Contributor

This is being closed because it does not comply with the contribution guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Fatal concurrent map iteration data race in vGPUmonitor feedback observation

2 participants