fix(e2e): safely generalize Windows PIS images - #9042
Open
r2k1 wants to merge 4 commits into
Open
Conversation
Run Sysprep asynchronously with /shutdown, wait for the VM to stop before deallocation, and preserve terminal RunCommand failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f4c66a0-992f-414b-bd60-647d28365281
Apply the existing Windows PIS ceiling to both the managed command and host-side shutdown observation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f4c66a0-992f-414b-bd60-647d28365281
Document parity with AKS RP Windows PIS and reuse one VM instance-view power-state parser for running and stopped waits. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f4c66a0-992f-414b-bd60-647d28365281
r2k1
requested review from
AbelHu,
Devinwong,
SriHarsha001,
awesomenix,
calvin197,
cameronmeissner,
djsly,
ganeshkumarashok,
karenychen,
lilypan26,
mxj220,
pdamianov-dev,
phealy,
runzhen,
sulixu,
timmy-wright,
titilambert,
xuexu6666 and
zachary-bailey
as code owners
July 25, 2026 11:30
Contributor
Windows Unit Test Results 3 files 11 suites 48s ⏱️ Results for commit b466777. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Windows image-capture path in AgentBaker e2e to more closely mirror AKS RP’s Prepared Image Specification (PIS) generalization flow: Sysprep shuts the VM down, the harness waits for PowerState/stopped, then deallocates and captures the disk, while also centralizing and hardening power-state parsing and RunCommand terminal failure detection.
Changes:
- Centralize VMSS VM instance-view power state parsing via a shared helper and add unit coverage for it.
- Introduce
runWindowsSysprep()that starts managed RunCommand asynchronously, waits for VM shutdown, and checks terminal RunCommand failures during the wait. - Add unit tests pinning the Sysprep script semantics and validating RunCommand terminal-error classification.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| e2e/vmss.go | Adds vmPowerState() helper and refactors running-state wait to use it. |
| e2e/vmss_test.go | Adds unit test coverage for vmPowerState(). |
| e2e/test_helpers.go | Adds runCommandTerminalError, pins Sysprep timeout, and introduces runWindowsSysprep() flow used by CreateImage(). |
| e2e/test_helpers_test.go | Adds tests to pin Sysprep shutdown behavior and validate RunCommand terminal-error logic. |
Comment on lines
+784
to
+788
| for _, status := range vm.Properties.InstanceView.Statuses { | ||
| if status.Code != nil && strings.HasPrefix(*status.Code, "PowerState/") { | ||
| return strings.TrimPrefix(*status.Code, "PowerState/") | ||
| } | ||
| } |
Comment on lines
+863
to
+882
| powerState := vmPowerState(vm.VirtualMachineScaleSetVM) | ||
| if powerState == "stopped" { | ||
| return view, nil | ||
| } | ||
| if powerState != "" { | ||
| toolkit.Logf(ctx, "VM is in power state: %s, waiting for stopped state...", powerState) | ||
| } | ||
|
|
||
| getResp, err := config.Azure.VMSSVMRunCommands.Get(ctx, rg, s.Runtime.VMSSName, instanceID, runCommandName, &armcompute.VirtualMachineScaleSetVMRunCommandsClientGetOptions{ | ||
| Expand: to.Ptr("instanceView"), | ||
| }) | ||
| if err != nil { | ||
| return view, fmt.Errorf("failed to verify sysprep result while waiting for VM shutdown: %w", err) | ||
| } | ||
| if getResp.Properties != nil && getResp.Properties.InstanceView != nil { | ||
| view = *getResp.Properties.InstanceView | ||
| if err := runCommandTerminalError(view); err != nil { | ||
| return view, err | ||
| } | ||
| } |
awesomenix
reviewed
Jul 26, 2026
| } | ||
| } | ||
|
|
||
| func TestRunCommandTerminalError(t *testing.T) { |
Contributor
There was a problem hiding this comment.
feels like adding a test for e2e test is a bit overkill
Mirror AKS RP's separate RunCommand and shutdown wait timeouts, tolerate guest-agent interruption during shutdown, and reject premature VM deallocation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f4c66a0-992f-414b-bd60-647d28365281
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
e2e/test_helpers.go:883
- runWindowsSysprep returns success immediately when the VM reaches PowerState/stopped, without checking the RunCommand instance view one last time. If Sysprep returns a non-zero exit code but still shuts down the VM, this path would incorrectly treat the run as successful (contradicting the stated goal of preserving native Sysprep failures). Consider validating the RunCommand instance view before returning on "stopped".
lastPowerState = vmPowerState(vm.VirtualMachineScaleSetVM)
if lastPowerState == "stopped" {
return view, nil
}
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:
Mirrors AKS RP's Windows Prepared Image Specification generalization sequence in AgentBaker E2E:
/oobe /generalize /mode:vm /quiet /shutdownPowerState/stoppedbefore deallocation and SIG capture$LASTEXITCODEas shutdown beginsdeallocatingordeallocatedVM states before Sysprep reachesstoppedThis intentionally differs from the Windows Packer VHD flow, where Packer owns the lifecycle and uses an in-guest ImageState poll.
Tests:
Which issue(s) this PR fixes:
N/A