fix: give the worker handoff endpoints a Windows path - #1238
Conversation
The four handoff endpoint constants were POSIX paths with no build tag, so a Windows worker binary looked for its peer under /run/eraser.sh while the manager mounts the shared-data volume at C:\run\eraser.sh. The pod started and the collector then failed to bind, which is why "cleanup works on a Windows node" was not reachable even with an OS-aware pod spec. They now live in the platform files alongside CRIPath, which already splits for the same reason. Full paths rather than a shared root plus separators: the two are genuinely different strings, and joining them in a Linux-compiled manager would emit forward slashes into a Windows path that only fails at runtime. The Windows endpoints are Unix domain sockets, so the constants themselves have to fit in sun_path, not just the guard inside listen. The test pins that, since a deployment uses the constants rather than anything the guard sees. Signed-off-by: Charles Wu <yuewu2@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Moves worker handoff endpoint paths into platform-specific files so Windows workers use valid Windows-mounted socket paths while Unix behavior remains unchanged.
Changes:
- Defines platform-specific handoff endpoint constants.
- Adds Windows validation for absolute paths and
sun_pathlimits.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pkg/utils/utils.go |
Removes platform-dependent constants. |
pkg/utils/platform_unix.go |
Preserves existing Unix endpoint paths. |
pkg/utils/platform_windows.go |
Adds Windows endpoint paths. |
pkg/utils/platform_windows_test.go |
Validates Windows path shape and length. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 41 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| // on this platform. These are Unix domain sockets, so the whole path has to fit | ||
| // in sun_path -- see maxSocketPath in handoff_windows.go. | ||
| const ( | ||
| ScanErasePath = `C:\run\eraser.sh\shared-data\scanErase` |
There was a problem hiding this comment.
Could we use sandbox-relative paths here instead of hard-coding C:\run? Kubernetes documents that containerd 1.6 and fallback hosts expose these mounts beneath %CONTAINER_SANDBOX_MOUNT_POINT%. Would the current paths miss the shared volume and break every worker handoff? If those hosts are intentionally unsupported, could we enforce and document that requirement?
What
Gives the four worker handoff endpoints a Windows path. They were POSIX strings with no build tag:
Why
The manager mounts the shared-data
emptyDirat a Windows path on a Windows node, but the worker binary compiled forwindows/amd64still looked for its peer under/run/eraser.sh. The pod starts, and the collector then fails to bind its endpoint.This is the gap between "a worker pod reaches Running on a Windows node" and "unused image cleanup works on a Windows node". #1229 made the CRI reachable and #1231 made the handoff itself work; this is the last piece of the worker's own view of the filesystem.
How
The constants move into the platform files, next to
CRIPath, which already splits for exactly the same reason:platform_unix.goplatform_windows.goScanErasePath/run/eraser.sh/shared-data/scanEraseC:\run\eraser.sh\shared-data\scanEraseCollectScanPath…/collectScan…\collectScanEraseCompleteCollectPath…/eraseCompleteCollect…\eraseCompleteCollectEraseCompleteScanPath…/eraseCompleteScan…\eraseCompleteScanFull paths rather than a shared root plus separator constants. The two are genuinely different strings, and a root-plus-join scheme invites the manager — which is compiled for Linux but emits Windows pod specs — to build a Windows path with
filepath.Joinand get forward slashes that only fail at runtime, on the node.Linux values are byte-for-byte what they were. No caller changes: every consumer already refers to the constants by name.
Testing
TestHandoffEndpointsFitInSunPath(Windows-only) asserts each constant is an absolute Windows path and fits insun_path. Those endpoints are Unix domain sockets on Windows, so the constants themselves are subject to the limit — not just the guard insidelisten, and the constants are what a real deployment uses. Longest is 49 bytes against the 107 ceiling, so there is room, but the check means a future path change can't silently overrun it.Verified:
GOOS=linuxandGOOS=windowsbuild + vet clean,golangci-lintclean on both,go test ./pkg/...green.Coordination with #1236
#1236 adds
LinuxSharedDataPath/WindowsSharedDataPathfor the manager, which needs both values at once because it runs on Linux and emits specs for Windows. This PR is the worker half, which needs only its own and gets it at compile time.The two are complementary but touch the same
constblock inpkg/utils/utils.go, so whichever merges second needs a small rebase. Happy to go in either order — if #1236 lands first I'll rebase this onto it.Follow-up, not in this PR
The remover's single five-minute context still spans
ListImages+ListContainers+ everyDeleteImage. Measured on an AKS Windows node, one image takes 15–74s to delete and a freshly created node carried 30 unused images, so that budget is exhausted long before the work is. Sending that separately.