refactor(e2e): centralize scenario cleanup

[r2k1]

What this PR does / why we need it:

Adds Scenario.Cleanup so E2E resource teardown no longer depends on testing.T at each call site. Cleanup callbacks run concurrently-safe in LIFO order with a detached five-minute overall context and a fresh one-minute timeout per step. Long-running VMSS diagnostic collection retains its four-minute budget. Errors are aggregated, remaining cleanups continue after panics, and failures are reported at the test-runner boundary.

Migrates VMSS, SIG image, Kata, and NVIDIA DaemonSet cleanup while preserving two-stage VHD-caching lifetimes and idempotent Kubernetes deletion.

Which issue(s) this PR fixes:

N/A

[github-actions]

Windows Unit Test Results

  3 files   13 suites   52s ⏱️
404 tests 404 ✅ 0 💤 0 ❌
407 runs  407 ✅ 0 💤 0 ❌

Results for commit 797f121.

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

Centralizes E2E teardown through Scenario.Cleanup, but cleanup timeout and SIG deletion handling need correction.

Changes:

• Adds concurrent-safe LIFO cleanup with panic/error aggregation.
• Migrates VMSS, SIG, Kata, and NVIDIA cleanup.
• Adds unit tests for cleanup ordering and concurrency.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
e2e/cleanup.go	Implements centralized scenario cleanup.
e2e/cleanup_test.go	Tests ordering, errors, panics, and concurrency.
e2e/types.go	Adds cleanup state to Scenario.
e2e/test_helpers.go	Integrates cleanup into scenario execution and SIG teardown.
e2e/vmss.go	Migrates VMSS logs and deletion cleanup.
e2e/validators_kata.go	Migrates Kata resource cleanup.
e2e/scenario_gpu_daemonset_test.go	Migrates NVIDIA DaemonSet cleanup.
e2e/config/azure.go	Clarifies ignored SIG deletion errors.

Suppressed comments (1)

e2e/test_helpers.go:1065

• 🟡 Medium Risk — 🏗️ Architecture: This migrated callback always returns nil because DeleteSIGImageVersion discards every BeginDelete error, so the new cleanup aggregation cannot report authentication, throttling, timeout, or dependency failures and the image version can leak. The noted stage-2 VMSS race makes this especially likely: beginning VMSS deletion does not mean the image is no longer referenced. Return and propagate deletion errors, retry the expected in-use conflict after VMSS deletion, and suppress only an idempotent not-found response.

	s.Cleanup(func(ctx context.Context) error {
		config.Azure.DeleteSIGImageVersion(ctx, rg, *gallery.Name, *image.Name, version)
		return nil

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

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

Suppressed comments (2)

e2e/test_helpers.go:1065

• 🟡 Medium Risk — This callback always reports success because DeleteSIGImageVersion discards every BeginDelete error. In the documented stage-2 race, the delete can fail while the VMSS deletion is still in progress, and no retry occurs, leaving the image version behind; authorization and throttling failures are also hidden from the new cleanup aggregator. Return and propagate deletion errors, treat 404 as idempotent success, and retry dependency conflicts within the cleanup context.

	s.Cleanup(func(ctx context.Context) error {
		config.Azure.DeleteSIGImageVersion(ctx, rg, *gallery.Name, *image.Name, version)
		return nil

e2e/vmss.go:94

• 🟡 Medium Risk — The deletion callback shares the single five-minute cleanup context with the log-extraction callback that runs immediately before it. Linux extraction executes roughly 20 SSH commands sequentially, and Windows extraction may consume four minutes; if that work reaches the deadline, deleteVMSS receives an already-canceled context and cannot start teardown. The previous implementation detached a fresh one-minute context for deletion. Please reserve an independent deletion budget or strictly bound log collection so VMSS teardown cannot be starved.

		return deleteVMSS(ctx, s)

[Copilot]

Pull request overview

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

Suppressed comments (2)

e2e/vmss.go:97

• 🟡 Medium Risk — 🔧 Script Logic: This migrated callback is now capped by the global one-minute step deadline, but extractLogsFromVMWindows explicitly allows four minutes and its Azure Run Command says collection may take a few minutes (vmss.go:869, 889-939). Failed Windows scenarios can therefore lose their diagnostic bundle after one minute, whereas the previous cleanup allowed up to five minutes. The cleanup API needs a per-step timeout (or another way to preserve the longer log-collection budget) while still honoring the five-minute overall deadline.

	s.Cleanup(func(ctx context.Context) error {
		extractLogsFromVM(ctx, s, vm)

e2e/config/azure.go:820

• 🟡 Medium Risk — ⚡ Operational Implications: Ignoring the delete error means this cleanup always reports success even when the image version remains. The comment identifies a concrete conflict window: stage-2 cleanup only starts VMSS deletion, so BeginDelete can fail while that deletion is in progress, and there is no later retry. Return the Azure error through the cleanup callback and retry the transient dependency conflict within the cleanup context (or wait for stage-2 VMSS deletion) so central cleanup reporting does not silently leak SIG versions.

	// Ignore errors because the stage-2 VMSS deletion can still be in progress.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

e2e/cleanup.go:50

• 🟡 Medium Risk — 🏗️ Architecture: Every callback still receives the same five-minute context, so this does not provide the stated fresh one-minute timeout per step. Because Kubernetes cleanups are registered after the VMSS handlers, one blocked delete can consume the entire shared deadline; log collection and VMSS deletion then run with an already-expired context, leaving the main resource behind. Store a timeout with each cleanup entry, wrap every invocation in a fresh one-minute child context, and register VMSS log collection with the four-minute override while retaining the five-minute overall parent.

		if err := runCleanup(ctx, fn); err != nil {