fix: infinite loop in Device Plugin caused by stale SocketWatcher state #4163

isaac-dasan · 2025-12-11T18:09:36Z

Description

This PR fixes a critical bug in the SocketWatcher that caused the device plugin to enter a tight restart loop if the socket file was deleted (e.g., by Kubelet) or if the plugin restarted for any reason.

The Problem:

The SocketWatcher maintains a map socketChans of active watchers. When a watcher goroutine exited (due to socket deletion or context cancellation), it closed its notification channel but failed to remove the entry from the map.

Consequently, when the PluginManager attempted to restart the plugin:

It called WatchSocket with the same socket path.
WatchSocket found the existing entry in the map and returned the already-closed channel.
The plugin's main loop (Run) received from this closed channel immediately and exited.
The loop repeated indefinitely, causing the plugin to restart every ~100ms.

Symptoms:

Logs showing "starting device plugin for resource" and "registering with kubelet" repeating rapidly with no intermediate errors.
Pods failing with "UnexpectedAdmissionError: Allocate failed due to no healthy devices present". This occurred because the plugin was constantly churning, leaving no stable window for Kubelet to allocate devices.

The Fix:

Updated SocketWatcher to ensure that the socket entry is deleted from the socketChans map when the watcher gor
outine exits. This ensures that subsequent calls to WatchSocket create a fresh watcher and channel.

Testing:

Added a regression test TestWatchSocketCleanup in socketwatcher_test.go to verify that the map entry is cleaned up and new watchers can be established successfully.

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Signed-off-by: GitHub <[email protected]>

Copilot

Pull request overview

This PR fixes a critical infinite loop bug in the device plugin's SocketWatcher that occurred when socket files were deleted or plugins restarted. The root cause was that the socketChans map wasn't being cleaned up when watcher goroutines exited, causing subsequent WatchSocket calls to return already-closed channels. This led to the plugin immediately exiting and restarting in a tight loop (~100ms intervals), preventing stable device allocation.

Key Changes:

Added cleanup logic to remove map entries when watcher goroutines exit
Added regression test to verify map cleanup and fresh watcher creation

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
cns/deviceplugin/socketwatcher.go	Modified defer block to delete socket entry from map before closing channel, ensuring subsequent watchers create fresh channels
cns/deviceplugin/socketwatcher_test.go	Added TestWatchSocketCleanup to verify map cleanup and that new watchers can be established after previous ones exit

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

thatmattlong · 2025-12-11T18:23:01Z

cns/deviceplugin/socketwatcher_test.go

+
+func TestWatchSocketCleanup(t *testing.T) {
+	// Create a temporary directory
+	tempDir, err := os.MkdirTemp("", "socket-watcher-test-")


I tend to prefer adding a .gitignore file with entries for every filename that we might create so you don't accidentally check in test data

This will create a file in /tmp/socket-watcher-test-123456 and the defer os.RemoveAll(tempDir) will clean it up, so no need to add entries to .gitignore

you can't guarantee that your os.RemoveAll will run, if your tests crash then the files will be left behind. On linux this is fine since the temp directory goes under /tmp but on windows I think os.MkdirTemp uses the current directory (not 100% sure about that). If you can guarantee that these temp dirs never get created in the scope of the git repo, then it is fine to not add a .gitignore

https://pkg.go.dev/os#MkdirTemp
https://pkg.go.dev/os#TempDir

Yes it is guaranteed to not be created in repo for windows as well. It typically looks like:
C:\Users\<Username>\AppData\Local\Temp\socket-watcher-test-<random>\to-be-deleted.sock

do what matt said

/tmp may not be accessible to a user with restricted permissions, may cross filesystem boundaries, may not actually be as temporary as you think (ie these files will persist until reboot, on Linux; and forever, on Windows). Etc. Also, this means your test has side-effects, and as you pointed out, escapes the repo scope - that's explicitly bad.

conventionally tests use the testdata folder (go test will set the current working directory to the directory of your test file so you don't have to do anything tricky when building the base path), so just create testdata/.gitignore and add the filenames that your tests create, then you can just read from testdata/<file> in the test without needing to construct any special base paths

At least use T.TempDir. But keeping test filesystem I/O within the repo tree is best imo.

updated with t.TempDir() since it is designed to handle cleanup automatically, even if the test fails or panics.

cns/deviceplugin/socketwatcher_test.go

Signed-off-by: GitHub <[email protected]>

isaac-dasan · 2025-12-11T23:19:31Z

/azp run Azure Container Networking PR

azure-pipelines · 2025-12-11T23:19:42Z

Azure Pipelines successfully started running 1 pipeline(s).

rbtr · 2025-12-12T00:22:46Z

cns/deviceplugin/socketwatcher_test.go

+
+func TestWatchSocketCleanup(t *testing.T) {
+	// Create a temporary directory
+	tempDir, err := os.MkdirTemp("", "socket-watcher-test-")


do what matt said

Signed-off-by: GitHub <[email protected]>

Replaces usage of `t.TempDir()` with a local `testdata` directory for temporary socket files in `socketwatcher_test.go` and `server_test.go`. This change addresses two issues on Windows: 1. `t.TempDir()` creates files in the system temp directory, which can cause cleanup failures and disk space issues on CI agents if files are left behind. 2. Explicitly closes file handles after `os.Create()` to prevent "Access is denied" errors during cleanup, as Windows does not allow deleting open files. Also adds a `.gitignore` to `cns/deviceplugin/testdata` to ensure temporary socket files are not tracked. Signed-off-by: GitHub <[email protected]>

isaac-dasan · 2025-12-12T22:33:46Z

/azp run Azure Container Networking PR

azure-pipelines · 2025-12-12T22:33:57Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: GitHub <[email protected]>

isaac-dasan · 2025-12-12T23:41:08Z

/azp run Azure Container Networking PR

azure-pipelines · 2025-12-12T23:41:18Z

Azure Pipelines successfully started running 1 pipeline(s).

fix: remove closed socker watcher from slice

6041975

Signed-off-by: GitHub <[email protected]>

isaac-dasan requested a review from a team as a code owner December 11, 2025 18:09

isaac-dasan requested review from Copilot and vakalapa December 11, 2025 18:09

Copilot started reviewing on behalf of isaac-dasan December 11, 2025 18:10 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

thatmattlong reviewed Dec 11, 2025

View reviewed changes

cns/deviceplugin/socketwatcher_test.go Outdated Show resolved Hide resolved

thatmattlong reviewed Dec 11, 2025

View reviewed changes

cns/deviceplugin/socketwatcher_test.go Outdated Show resolved Hide resolved

thatmattlong reviewed Dec 11, 2025

View reviewed changes

cns/deviceplugin/socketwatcher_test.go Outdated Show resolved Hide resolved

thatmattlong reviewed Dec 11, 2025

View reviewed changes

cns/deviceplugin/socketwatcher_test.go Outdated Show resolved Hide resolved

isaac-dasan added 3 commits December 11, 2025 21:24

fix: remove the socket if it already exists

f538d71

Signed-off-by: GitHub <[email protected]>

test: update test comments and error message

6ac6473

Signed-off-by: GitHub <[email protected]>

fix: lint

8e12d5c

Signed-off-by: GitHub <[email protected]>

isaac-dasan requested review from a team as code owners December 11, 2025 21:42

isaac-dasan requested a review from ashvindeodhar December 11, 2025 21:42

isaac-dasan force-pushed the users/isaac/watcherbugfix branch from e7ad386 to 8e12d5c Compare December 11, 2025 21:44

lint: fix err

27004f3

Signed-off-by: GitHub <[email protected]>

thatmattlong previously approved these changes Dec 11, 2025

View reviewed changes

miguelgoms previously approved these changes Dec 11, 2025

View reviewed changes

isaac-dasan enabled auto-merge December 11, 2025 23:18

isaac-dasan added this pull request to the merge queue Dec 11, 2025

rbtr removed this pull request from the merge queue due to a manual request Dec 12, 2025

rbtr requested changes Dec 12, 2025

View reviewed changes

nit: tempDir()

426cc35

Signed-off-by: GitHub <[email protected]>

isaac-dasan dismissed stale reviews from miguelgoms and thatmattlong via 426cc35 December 12, 2025 00:52

lint: fix err name

e7f64e1

Signed-off-by: GitHub <[email protected]>

fix: infinite loop in Device Plugin caused by stale SocketWatcher state #4163

Are you sure you want to change the base?

fix: infinite loop in Device Plugin caused by stale SocketWatcher state #4163

Uh oh!

Conversation

isaac-dasan commented Dec 11, 2025

Description

The Problem:

Symptoms:

The Fix:

Testing:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isaac-dasan commented Dec 11, 2025

Uh oh!

azure-pipelines bot commented Dec 11, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaac-dasan commented Dec 12, 2025

Uh oh!

azure-pipelines bot commented Dec 12, 2025

Uh oh!

isaac-dasan commented Dec 12, 2025

Uh oh!

azure-pipelines bot commented Dec 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants