Repository navigation
feat: build aks-node-controller for windows - #10035
Open
Abdul Asfari (aboodasfari) wants to merge 1 commit into
Open
Abdul Asfari (aboodasfari) wants to merge 1 commit into
Abdul Asfari (aboodasfari) wants to merge 1 commit into
Conversation
Contributor
Windows Unit Test Results 3 files 21 suites 1m 18s ⏱️ Results for commit d40e57e. |
Contributor
|
Contributor
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the Windows compilation changes.
No open issues 🎉
What changed in this PR
Enables aks-node-controller to compile for Windows as a first step toward Windows scriptless provisioning, while preserving Linux behavior. Windows provisioning itself remains out of scope.
Changes:
- Isolates file locking by platform, retaining Unix
flockand returning unsupported on Windows. - Adds Windows-specific paths and disables the default Linux waagent Ready-report handoff on Windows.
- Adds Windows cross-build and
go vetchecks to CI.
| File | Description |
|---|---|
aks-node-controller/main.go |
Uses the platform-specific events directory. |
aks-node-controller/goal_state.go |
Calls the platform-specific file-lock helper. |
aks-node-controller/filelock_windows.go |
Returns unsupported for Linux KVP file locking. |
aks-node-controller/filelock_unix.go |
Preserves the existing exclusive flock behavior. |
aks-node-controller/const_windows.go |
Defines Windows paths separate from legacy CSE state. |
aks-node-controller/const_unix.go |
Keeps existing non-Windows paths and adds the events constant. |
aks-node-controller/app.go |
Skips Ready reporting when no marker path is configured. |
.github/workflows/go-test.yml |
Adds Windows build and vet checks for the controller. |
Review effort: Balanced
Contributor
|
| @@ -1,3 +1,5 @@ | |||
| //go:build !windows | |||
Contributor
There was a problem hiding this comment.
I don't like these flags. We won't be able to test linux logic on windows machines. And windows logic on linux.
Should avoid them unless it required for some system calls
Contributor
There was a problem hiding this comment.
Test may fail on some system unexpectedly
Contributor
There was a problem hiding this comment.
There are multiple alternatives. Not sure which one is better.
One example:
type OSConfig struct {...}
var LinuxConfig = OSConfig{...}
var WindowsConfig = OSConfig{...}
| go test $(go list ./... | grep -v e2e) -coverprofile=coverage/agentbaker.out -covermode=atomic | ||
| (cd aks-node-controller && go build -mod=readonly ./... && go test ./... -coverprofile=../coverage/aks-node-controller.out -covermode=atomic) | ||
| # aks-node-controller will also run on Windows nodes; keep it (and its tests) compiling there. | ||
| (cd aks-node-controller && GOOS=windows go build -mod=readonly ./... && GOOS=windows go vet ./...) |
Contributor
There was a problem hiding this comment.
I don't think we need to do this.
Eventually we're going to compile it. Don't need to do it twice
This branch has not been deployed
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.
What this PR does / why we need it:
First step towards running aks-node-controller (ANC) on Windows nodes (Windows scriptless). ANC did not compile for
GOOS=windowsbecausegoal_state.goused the Linux-onlysyscall.Flock. This PR makes it build on Windows with no Linux behaviour change. It does not yet make provisioning work on Windows.filelock_unix.go(unchangedflock) andfilelock_windows.go(returnserrors.ErrUnsupported; its only caller writes the Linux Hyper-V KVP pool file, and Windows exposes KVP through the registry).const.gotoconst_unix.go(//go:build !windows) and addconst_windows.gowith Windows paths underC:\AzureData\aks-node-controller\. This is a separate directory because the legacy Windows CSE already writesC:\AzureData\provision.completewith different contents. The events dir matches$global:EventsLoggingDirinparts/windows/windowscsehelper.ps1.reportProvisionStatus).GOOS=windows go buildandgo vetfor aks-node-controller ingo-test.yml.Validation: build and vet pass for linux, windows and darwin;
go test ./...passes (macOS); golangci-lint is clean for linux and windows. Tests were not run on Windows.Which issue(s) this PR fixes:
ADO task 40462515 (story 39927311, Windows Scriptless)