Fix ListAndWatch race#16
Open
mvgorbunov wants to merge 2 commits into
Open
Conversation
The kubelet never re-opens a broken ListAndWatch stream on its own; it only opens a new one in response to a Register call. Until now the only re-registration trigger was the fsnotify watch on kubelet.sock, so a stream dropped without a socket re-creation (kubelet-side gRPC failure, connection loss) left the plugin's devices invisible to the kubelet until the pod was manually restarted. - ListAndWatch now invokes an onStreamBroken callback when the stream ends with an error while the plugin itself is still running. The registry re-registers the plugin asynchronously, retrying with a 1s delay to avoid a tight register/drop loop against an unhealthy kubelet, and gives up if the plugin is stopped (e.g. a kubelet restart already replaced it). - The fsnotify watcher's Errors channel was never drained. After the first watcher error (typically an inotify queue overflow) fsnotify's sender goroutine blocks forever and kubelet-restart detection is permanently wedged. Drain it and resync by re-registering all plugins, since events (possibly the kubelet socket CREATE) were lost. - The "closing ListAndWatch connection" log always printed err = <nil> because the defer evaluated its arguments immediately; moved into a closure so the actual error is logged. Adds an e2e test that drops the ListAndWatch connection without recreating the kubelet socket and verifies the plugin registers again. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
go-udev's DeviceChan returns a device channel and an error channel, and its internal goroutine closes BOTH channels when it exits. The monitor select loop received from the device channel without the comma-ok form: a receive from the closed channel yields a nil device, and the subsequent method calls (Action, Syspath) panic. Use comma-ok on both receives. When the device channel is closed, nil it out so the select falls through to the error-channel case, which already performs the reconnect. When the error channel is closed, synthesize an error so the reconnect path still runs and logs meaningfully. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves resilience of the device plugin lifecycle by recovering from broken kubelet ListAndWatch streams (without relying solely on kubelet socket recreation), and hardens event-monitoring loops to avoid deadlocks/panics when underlying channels fail/close.
Changes:
- Trigger asynchronous plugin re-registration when
ListAndWatchterminates with an error while the plugin is still running. - Drain
fsnotify.Watcher.Errorsand force a resync (re-register all plugins) on watcher errors to avoid wedging kubelet restart detection. - Harden udev monitor select loop against closed go-udev channels; add an e2e test covering dropped
ListAndWatchstreams.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/udev/udev.go | Prevent panics by handling closed udev device/error channels and preserving reconnect behavior. |
| internal/plugin/registry.go | Add async re-registration on broken streams; drain fsnotify errors and resync registrations. |
| internal/plugin/plugin.go | Detect broken ListAndWatch streams and invoke a callback to trigger re-registration; fix deferred logging to report the real error. |
| cmd/udev-manager/e2e_test.go | Add e2e coverage ensuring the plugin re-registers when kubelet drops ListAndWatch without restarting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+92
to
+107
| for { | ||
| select { | ||
| case <-time.After(reregisterDelay): | ||
| case <-p.stopped: | ||
| // The plugin was stopped (e.g. kubelet restart triggered hup); | ||
| // the replacement plugin registers itself. | ||
| return | ||
| case <-r.ctx.Done(): | ||
| return | ||
| } | ||
| klog.Warningf("%s: ListAndWatch stream broken by kubelet; re-registering", p.resource.Name()) | ||
| if err := r.register(p); err == nil { | ||
| return | ||
| } | ||
| // register failed; loop and retry after another delay. | ||
| } |
Comment on lines
+88
to
+91
| func (r *Registry) reregister(p *plugin) { | ||
| r.wg.Add(1) | ||
| go func() { | ||
| defer r.wg.Done() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-register plugins when kubelet drops the ListAndWatch stream
The kubelet never re-opens a broken ListAndWatch stream on its own; it
only opens a new one in response to a Register call. Until now the only
re-registration trigger was the fsnotify watch on kubelet.sock, so a
stream dropped without a socket re-creation (kubelet-side gRPC failure,
connection loss) left the plugin's devices invisible to the kubelet
until the pod was manually restarted.
ListAndWatch now invokes an onStreamBroken callback when the stream
ends with an error while the plugin itself is still running. The
registry re-registers the plugin asynchronously, retrying with a 1s
delay to avoid a tight register/drop loop against an unhealthy
kubelet, and gives up if the plugin is stopped (e.g. a kubelet
restart already replaced it).
The fsnotify watcher's Errors channel was never drained. After the
first watcher error (typically an inotify queue overflow) fsnotify's
sender goroutine blocks forever and kubelet-restart detection is
permanently wedged. Drain it and resync by re-registering all
plugins, since events (possibly the kubelet socket CREATE) were lost.
The "closing ListAndWatch connection" log always printed err =
because the defer evaluated its arguments immediately; moved into a
closure so the actual error is logged.
Guard udev monitor loop against closed go-udev channels
go-udev's DeviceChan returns a device channel and an error channel, and
its internal goroutine closes BOTH channels when it exits. The monitor
select loop received from the device channel without the comma-ok form:
a receive from the closed channel yields a nil device, and the
subsequent method calls (Action, Syspath) panic.
Use comma-ok on both receives. When the device channel is closed, nil it
out so the select falls through to the error-channel case, which already
performs the reconnect. When the error channel is closed, synthesize an
error so the reconnect path still runs and logs meaningfully.
Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com