fix(device-plugin): re-check lock file state to prevent vGPUmonitor hang - #2451
Conversation
Redesign WatchLockFile to return an event notification channel (chan struct{}) instead of a boolean payload channel, and have consumers determine lock state by inspecting the filesystem via os.Stat (IsMigApplyLockExist).
This prevents dropped or reordered fsnotify events from causing watchAndFeedback in vGPUmonitor to block indefinitely on lock release. Also handles initial lock file presence at startup and watcher shutdown.
Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AyushSrivastava1818 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe MIG lock watcher now sends state-change notifications through a ChangesMIG lock flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant fsnotify
participant watchLockFile
participant watchAndFeedback
participant monitorLoop
participant MIGLockFile
fsnotify->>watchLockFile: Create or remove event
watchLockFile->>watchAndFeedback: struct{} notification
watchAndFeedback->>MIGLockFile: IsMigApplyLockExist()
MIGLockFile-->>watchAndFeedback: lock state
watchAndFeedback->>monitorLoop: errTemporaryClosed when locked
monitorLoop->>MIGLockFile: re-check until lock removal
MIGLockFile-->>monitorLoop: lock removed
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 |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-plugin/nvidiadevice/nvinternal/plugin/lock_test.go`:
- Around line 78-87: Update both signal receive cases in
pkg/device-plugin/nvidiadevice/nvinternal/plugin/lock_test.go at lines 78-87 and
116-125 to capture the receive-success boolean from sigChan and fail the test
when the channel is closed before a notification arrives; retain the existing
filesystem assertions and timeout behavior for valid notifications.
- Around line 162-199: Update the RapidCreateAndRemove test so its consumer
goroutine starts and observes the lock file’s existence before
os.Remove(testFile) is called. Coordinate the test with a readiness signal, then
remove the file and retain the assertion that the consumer exits through the
notification or filesystem re-check path.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/lock.go`:
- Around line 91-94: Update isLockFileExist to return true when os.Stat
succeeds, return false only when os.IsNotExist(err) is true, and treat all other
stat errors as locked by returning true.
- Around line 114-125: Update the watcher loop around watcher.Events and
watcher.Errors to handle fsnotify overflow errors, including ErrEventOverflow.
When overflow occurs, either stop and close the unusable watcher or send a
coalesced sigChan wake-up so vGPUmonitor and feedback.go re-evaluate the lock
state; preserve the existing event signaling and shutdown 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: 6de6e78c-d228-44ee-9cde-e5e9a51538fa
📒 Files selected for processing (4)
cmd/vGPUmonitor/feedback.gocmd/vGPUmonitor/main.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/lock.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/lock_test.go
GitHub Actions
Inactive
…t tests Address review comments on PR Project-HAMi#2451: - Fail closed in isLockFileExist on unknown stat errors - Handle fsnotify.ErrEventOverflow in WatchLockFile with non-blocking wake-up - Add channel receive ok-checks in lock_test.go and restructure RapidCreateAndRemove subtest - Extract waitForLockRemoval in main.go and add unit tests for feedback.go and main.go Signed-off-by: AyushSrivastava1818 <ayush.sri0705@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 329-354: The coalesced-burst test must avoid scheduling-dependent
signal processing. Update the producer and setup around watchAndFeedback so a
stale notification is queued while the state is locked, then set the final state
to unlocked before invoking watchAndFeedback; cancel the context only after the
function consumes that notification. Preserve the assertion that
watchAndFeedback returns nil, and ensure the revised synchronization is safe
under the race detector.
🪄 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: a4664da5-e5ff-4276-b991-e9716a158441
📒 Files selected for processing (6)
cmd/vGPUmonitor/feedback.gocmd/vGPUmonitor/feedback_test.gocmd/vGPUmonitor/main.gocmd/vGPUmonitor/main_test.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/lock.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/lock_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/device-plugin/nvidiadevice/nvinternal/plugin/lock_test.go
- cmd/vGPUmonitor/feedback.go
Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>
GitHub Actions
Inactive
|
Hi @archlitchi @chaunceyjiang @peachest, PR #2451 (
Could you please review and apply |
|
Why not use a blocking send or expand the buffer size? |
|
@FouoF Considered both, went with neither — here's the reasoning: Blocking send — ties the watcher goroutine to the consumer's readiness. Worse, the channel is explicitly closed on watcher shutdown (defer close(sigChan)), so any blocking send racing that close either panics (send on a closed channel) or, if it lands just before the close, blocks forever with nothing left to read it. A blocking send here isn't just slower, it's a crash/deadlock risk built into the shutdown path. Bigger buffer — only raises the burst size needed to reproduce #2450, it doesn't remove the failure mode, and it doesn't fix staleness: even with zero drops, a queued event can be read after the real state has already moved past it. You can see the same tension even in fsnotify's own error channel — when watcher.Errors overflows, we don't try to force every error through either; we just fire a non-blocking coalesced wake-up and let the consumer re-check state (lines 138–151). Same principle, applied consistently everywhere in this file. So the channel is now purely a wake-up signal (chan struct{}, buffered 1, always non-blocking send/default), and IsMigApplyLockExist() is the single source of truth on every wake. However many notifications coalesce, drop, or race the shutdown close, the consumer converges to the actual current filesystem state — so correctness doesn't depend on the channel being lossless or ordered at all, which neither a bigger buffer nor a blocking send can guarantee. RapidCreateAndRemove in lock_test.go covers the burst case #2450 was actually failing on. |
|
You can view the relevant rule here. |
3 participants
Summary
WatchLockFile()previously reported MIG apply lock file transitions over a single bufferedchan bool, using non-blocking sends for both lock creation and removal events. During a rapid create/remove sequence, the "lock removed" notification could be dropped if the channel buffer was already occupied. The consumer would then wait indefinitely for a notification that had already been discarded, permanently stalling the vGPU monitor feedback loop until the process was restarted.This PR changes the notification mechanism from event-based state propagation to state-based synchronization. The notification channel is now used only to signal that the lock state may have changed, while the filesystem (
os.Stat) becomes the single source of truth for determining whether the MIG apply lock currently exists.pkg/device-plugin/nvidiadevice/nvinternal/plugin/lock.go: replacedchan boolwith a notification-onlychan struct{}, addedIsMigApplyLockExist(), and closed the notification channel on watcher shutdown.cmd/vGPUmonitor/feedback.go: added an initial lock-state check and re-evaluates the filesystem after receiving notifications instead of relying on queued channel values.cmd/vGPUmonitor/main.go: replaced the direct<-lockChannelwait with a state-check loop that waits until the lock file is actually removed before restartingwatchAndFeedback().pkg/device-plugin/nvidiadevice/nvinternal/plugin/lock_test.go: updated existing tests for the new notification API and added aRapidCreateAndRemoveregression test covering rapid lock file transitions.Which issue(s) this PR fixes:
Fixes #2450
Special notes for your reviewer:
The notification channel is no longer treated as the source of truth. It acts only as a wake-up signal, while
IsMigApplyLockExist()determines the current lock state from the filesystem. Even if notifications are coalesced or dropped due to non-blocking sends, consumers always re-check the filesystem before proceeding, preventing the feedback loop from blocking indefinitely because of a lost notification.Testing
WatchLockFileunit tests.RapidCreateAndRemoveregression coverage.go test ./pkg/device-plugin/nvidiadevice/nvinternal/plugin/... -count=1GOOS=linux).go veton the modified packages with no new issues reported.Does this PR introduce a user-facing change?
Yes. The vGPU monitor no longer risks becoming permanently stalled due to a lost lock-file notification during rapid MIG apply operations. Metrics collection and utilization feedback now recover correctly even when intermediate filesystem notifications are coalesced or dropped.
AI Disclosure:
AI assistance was used for code inspection, concurrency analysis, and drafting the regression tests and PR description. The implementation, testing, and final changes were reviewed before submission.
Summary by CodeRabbit