Repository navigation
fix(e2e): wait for RunCommand deletion before returning - #10034
Abigail Liang (abigailliang-aks-sig-node) merged 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c815829b-19d2-4af1-8269-5d704d14325c
Windows Unit Test Results 3 files 21 suites 1m 4s ⏱️ Results for commit 22f7bd3. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
When script execution and cleanup both fail, the wrapper returns only the cleanup error, losing the script’s exit code, stdout, and stderr instead of preserving both errors.
1 open issue
What changed in this PR
This PR makes the E2E RunCommand helper wait for resource deletion before returning, preventing cleanup from racing subsequent VM operations. It is a prerequisite for the SSH-disabled reboot scenarios in #10021; live ARM validation remains pending.
Changes:
- Poll deletion to completion using the independent two-minute cleanup context.
- Propagate cleanup failures, joining them with existing command errors.
- Add six regression cases and expand the existing helper-test CI selection.
| File | Description |
|---|---|
e2e/scenario/provision.go |
Waits for RunCommand deletion and propagates cleanup errors. |
e2e/scenario/exec_runcommand_test.go |
Tests asynchronous cleanup, cancellation, failures, and timeout. |
.github/workflows/go-test.yml |
Runs cleanup and Windows guest-validation tests alongside existing NVIDIA checks. |
Review effort: Balanced
| run: | | ||
| cd e2e | ||
| go test -short ./scenario -run '^TestNvidiaDriverVersionValidationScript' | ||
| go test -short ./scenario -run '^(TestNvidiaDriverVersionValidationScript|TestRunCommandCleanup|TestWindowsGuestValidationWithoutSSH)' |
There was a problem hiding this comment.
This doesn't feel right. We should use test names only in exceptional situtations
There was a problem hiding this comment.
restore it. curious the test step Test GRID driver version validation doesnt Illustrate the reason it pickup the specific test name. so i ll just keep it as original.
…r script failures
|
There was a problem hiding this comment.
🟡 Changes recommended
The CI workflow’s test filter excludes TestRunCommandCleanup, leaving the new cleanup regressions unexecuted; expand the filter while retaining the NVIDIA tests.
1 open issue
1 resolved since last review
Review effort: Balanced
| require.Equal(t, commands, deletes) | ||
| } | ||
|
|
||
| func TestRunCommandCleanup(t *testing.T) { |
|
|
2728e70
into
main
Summary
Prerequisite for #10021. Merge this fix first; then update #10021 from main and rerun its SSH-disabled reboot scenarios before merging that PR.
runCommandreturns, preventing subsequent VM operations from racing an in-flight deletion.Background
The three SSH-disabled reboot scenarios in #10021 submitted a VM restart immediately after an asynchronous RunCommand deletion was accepted. ARM activity records show both operations remained outstanding until VMSS teardown; the RunCommand deletion was then preempted and the restart recorded completion after the test deadline.
The shared cleanup code discarded the
BeginDeletepoller. This PR fixes that sequencing independently of the SSH policy changes. It does not change SSH configuration, images, or the VMSS test timeout.Validation
go vet ./scenario, formatting/whitespace checks, and repository-pinned actionlint passed.Live ARM before/after confirmation remains pending. The SSH reboot E2E scenarios in #10021 must be rerun after this prerequisite merges.