trident-acl-agent: add storm trident-acl-agent E2E test harness, test images, and pipeline

[bfjelds (bfjelds)]

Summary

Adds the storm-trident E2E scenario that validates trident-acl-agent end-to-end against real tridentd (while mocking kubelet/Nebraska) on a VM, testing update, rollback, reboot, and commit.

Includes storm code, pipeline, test images, and development documentation on how to run these tests locally.

Context

This is the last step in enabling trident-acl-agent to run updates and rollbacks. Related PRs:

• trident: promote rollback to stable v1; adjust rollback return type to detect no-servicing #729
• feat(acl-agent): nebraska client module (Omaha protocol, invariant-encoded) #733 (via cherry-pick, not actually stacked)
• trident-acl-agent: rewrite as annotation-based ACL update agent #730
• [this PR] trident-acl-agent: add storm trident-acl-agent E2E test harness, test images, and pipeline #731

Validation

• https://dev.azure.com/mariner-org/ECF/_build/results?buildId=1192263&view=results

PR details

Storm code

• proxies/ — fake apiserver (serves the Node annotation protocol the agent watches/patches, including a real K8s-compatible watch stream), fake Nebraska/image servers, a minimal kubelet API shim, and an RP client used to drive scenarios and poll status annotations.
• tests/ — update.go (stage/finalize/commit against a real reboot), rollback.go (rollback stage/finalize/commit against a real reboot, plus a regression test that a second rollback against an empty rollback chain is detected as a no-op via servicing_kind rather than reporting a false Success and rebooting again), vm.go/logs.go (VM lifecycle and log collection helpers).
• trident.go, utils/config — scenario wiring and config plumbing specific to the aclagent suite.
• README.md — usage instructions for running the suite locally.

Registered in tools/cmd/storm-trident/main.go alongside storm-trident's other scenarios.

Test images (tests/images)

• baseimg-acl-agent.yaml / updateimg-acl-agent.yaml — VM image configurations for the base and post-update ACL test images.
• testimages.py — wires the update image into the existing COSI-based image build path (the base qcow2 is built via the Makefile target below, not testimages.py, since the qcow2 tooling differs from COSI).

Build tooling

New artifacts/trident-vm-acl-agent-testimage.qcow2 Makefile target for the base image.

Pipeline

New trident-acl-agent-test.yml stage wired into e2e-template.yml, running the aclagent storm scenario in CI.

Docs

docs/Development/Testing/TridentAclAgent-Tests.md documents the suite; Testing.md links to it.

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Adds a new Storm E2E scenario to validate trident-acl-agent end-to-end against a real tridentd on a VM (with fake Kubernetes apiserver + Nebraska/image server), along with the VM test images, CI pipeline stage wiring, and developer docs to run the suite locally.

Changes:

• Introduces storm-trident run aclagent scenario with test cases for deploy, update (stage/finalize/reboot/commit), rollback, log collection, and cleanup.
• Adds fake in-process infrastructure (apiserver, Omaha/Nebraska proxy, image server) used by the scenario to drive and observe Node annotation state transitions.
• Adds new ACL-agent VM image configs + Makefile target(s), plus a new CI stage to build images and run the scenario.

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tools/storm/utils/vm/qemu/qemu.go	Adds configurable qcow2 image selection regex for QEMU VM deployment.
tools/storm/aclagent/utils/config/config.go	Defines Storm scenario configuration/flags for the ACL-agent suite.
tools/storm/aclagent/trident.go	Registers the new aclagent Storm scenario and test case sequencing.
tools/storm/aclagent/tests/vm.go	VM deploy/check/cleanup wrappers for QEMU/Azure platforms.
tools/storm/aclagent/tests/update.go	Implements the A/B update E2E flow, including fake services and post-reboot commit validation.
tools/storm/aclagent/tests/rollback.go	Implements rollback E2E flow and regression coverage for no-op rollback.
tools/storm/aclagent/tests/logs.go	Hooks into existing VM log-fetch helper for the scenario.
tools/storm/aclagent/README.md	Scenario usage and behavior documentation.
tools/storm/aclagent/proxies/scenario.go	Scenario YAML schema + validation for driving test steps.
tools/storm/aclagent/proxies/rp.go	“RP client” to patch requests / poll status via the fake apiserver.
tools/storm/aclagent/proxies/nebraska.go	Fake Nebraska/Omaha endpoint for update availability + package metadata.
tools/storm/aclagent/proxies/kubelet.go	Kubelet-ish helper for simulated ready/reboot signaling (currently unused by the main flow).
tools/storm/aclagent/proxies/imageserver.go	Simple HTTP server to serve a real update artifact (e.g. .cosi).
tools/storm/aclagent/proxies/constants.go	Shared constants for annotations/labels and marker defaults.
tools/storm/aclagent/proxies/apiserver.go	Minimal fake Kubernetes apiserver with LIST/WATCH/PATCH support for Node.
tools/go.mod	Adds Kubernetes API dependencies required by the fake apiserver/client types.
tools/go.sum	Updates module sums to reflect new Kubernetes-related dependencies.
tools/cmd/storm-trident/main.go	Registers the aclagent scenario with the storm-trident binary.
tests/images/trident-vm-testimage/README.md	Documents the new ACL-agent VM image variant.
tests/images/trident-vm-testimage/base/updateimg-acl-agent.yaml	MIC config for the ACL-agent update image (.cosi) that enables the agent.
tests/images/trident-vm-testimage/base/baseimg-acl-agent.yaml	MIC config for the ACL-agent base qcow2 image (agent installed, not enabled).
tests/images/testimages.py	Wires the ACL-agent update image into the test-image build plumbing.
Makefile	Adds a new qcow2 build target for the ACL-agent base test image.
docs/Development/Testing/TridentAclAgent-Tests.md	Adds developer documentation for building/running the ACL-agent Storm scenario.
docs/Development/Testing/Testing.md	Links to the new ACL-agent testing documentation.
.pipelines/templates/stages/testing_acl_agent/trident-acl-agent-test.yml	Adds CI stages/jobs to build images and run the ACL-agent Storm scenario.
.pipelines/templates/e2e-template.yml	Wires the new ACL-agent test stage into the E2E template.

Suppressed comments (1)

tools/storm/aclagent/proxies/scenario.go:81

• Validate() currently counts assert-failure-reason as a valid mutually-exclusive step kind, but that step kind is not runnable. If removing assert-failure-reason, update the validation error accordingly so invalid scenarios fail fast with a correct message.

		if step.AssertFailureReason != "" {
			kinds++
		}
		if kinds != 1 {
			return fmt.Errorf("scenario step %d must set exactly one of patch/expect/assert-failure-reason", index)

[Copilot]

Pull request overview

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

Suppressed comments (8)

docs/Development/Testing/TridentAclAgent-Tests.md:176

• The "Reboot Choice" section describes shim-based reboot interception, but the harness code treats finalize as a real reboot and waits for SSH to go down/up. The docs should describe the real reboot flow, or the implementation should be updated to actually use a reboot shim.

## Reboot Choice

This scenario uses shim-based reboot interception rather than a full VM
reboot: a `reboot`/`systemctl reboot` shim on `PATH` inside the VM signals the
scenario's controller and exits the agent process instead of actually
rebooting. The scenario then restarts `trident-acl-agent` fresh, exercising
its post-reboot resume logic without tearing down the SSH session or the
in-process fake services. This is less realistic than a full reboot, but it
keeps the test deterministic and fast.

tests/images/trident-vm-testimage/README.md:21

• This section says the ACL-agent image "enables trident-acl-agent.service" and that the scenario writes "localhost" tunnel endpoints, but the base qcow2 config (baseimg-acl-agent.yaml) leaves the service disabled, and the storm harness configures endpoints via --host-endpoint-ip (not localhost reverse tunnels). Updating the README will keep the image docs consistent with the scenario and image configs.

For both, a set of corresponding update images is available. The ACL-agent
variant reuses the servicing-style VM image layout but additionally installs
`trident-acl-agent` and enables `trident-acl-agent.service` so storm ACL-agent
scenarios can drive a real in-guest agent talking to `tridentd`. The image does
not preseed /etc/trident/trident-acl-agent.conf with runner-specific tunnel
ports; the test scenario should SSH in after boot and write the real localhost
proxy endpoints for Nebraska and the Kubernetes API server.

tools/storm/aclagent/README.md:89

• The "Reboot choice" section says the scenario uses a reboot shim to avoid a full reboot, but run-ab-update/run-rollback explicitly wait for SSH to drop and come back, and describe a real systemctl reboot triggered by the agent. The docs should match the harness's real-reboot behavior.

## Reboot choice

This scenario keeps the shim-based reboot interception from the old tester.
That is less realistic than a full VM reboot, but it keeps the test deterministic
and lets the storm runner hold the reverse SSH tunnels and in-process fake services
steady while the agent drives the finalize path.

tools/storm/aclagent/proxies/apiserver.go:285

• handlePatch calls Decoder.DisallowUnknownFields(), but the body is decoded into map[string]any, so unknown fields are never rejected. This can hide mismatches between what the harness expects and what the agent actually patches.

Decode directly into metadataPatch (with DisallowUnknownFields) before applying the patch.

	defer r.Body.Close()
	body := json.NewDecoder(r.Body)
	body.DisallowUnknownFields()
	var raw map[string]any
	if err := body.Decode(&raw); err != nil {
		http.Error(w, fmt.Sprintf("invalid patch body: %v", err), http.StatusBadRequest)
		return
	}

tools/storm/aclagent/README.md:46

• This README describes prepareVmForAclAgent as using localhost:<port> via reverse-SSH-forwarded tunnels and running enable --now, but the harness configures the agent to talk to http://<host-endpoint-ip>:<port> and does an explicit restart (to reconnect between test cases). Keeping the README aligned with the actual harness behavior will reduce confusion when running locally.

`prepareVmForAclAgent` writes `/etc/trident/trident-acl-agent.conf` pointing
at the `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM,
then runs `systemctl enable --now trident-acl-agent.service`. Before that
runs, the service simply isn't started -- no crash-looping, no log noise.

docs/Development/Testing/TridentAclAgent-Tests.md:153

• The "Test Cases" list is out of sync with the current harness: config/service setup happens in run-ab-update/run-rollback (not check-deployment), the scenario includes run-rollback, and the harness performs a real reboot (SSH goes down/up) rather than a shim-based simulated reboot.

1. **deploy-vm** — Copies the base qcow2 image and creates a QEMU VM
2. **check-deployment** — Verifies the VM booted and is accessible via SSH;
   writes `/etc/trident/trident-acl-agent.conf` pointing at the
   `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM, then
   runs `systemctl enable --now trident-acl-agent.service`
3. **run-ab-update** — Starts the fake apiserver and fake Nebraska/Omaha
   endpoints in-process, seeds bootstrap node labels, patches the desired
   update-image label, and waits for `trident-acl-agent` to drive a real
   Trident A/B update to completion (including the shim-based simulated
   reboot)

docs/Development/Testing/TridentAclAgent-Tests.md:159

• The flags table lists --artifacts-dir default as /tmp, but the scenario's TestConfig sets ArtifactsDir default to .. This mismatch can cause local runs to look in the wrong directory by default.

| Flag | Description | Default |
|------|-------------|---------|
| `--artifacts-dir` | Directory containing VM images | `/tmp` |
| `--output-path` | Output directory for logs | `./output` |
| `--platform` | `qemu` or `azure` | `qemu` |
| `--ssh-private-key-path` | Path to SSH private key | `~/.ssh/id_rsa` |
| `--api-server-port` | Port for the fake Kubernetes API server | `18080` |

tools/storm/aclagent/proxies/rp.go:63

• ScenarioStep supports assert-failure-reason (and Scenario.Validate allows it), but RPClient.runStep never handles AssertFailureReason. Any scenario YAML that uses assert-failure-reason will currently fail with "no recognized action" instead of performing the assertion.

Either implement assert-failure-reason handling in the runner, or remove the field (and its validation path) until it’s supported.

func (c *RPClient) runStep(ctx context.Context, index int, step ScenarioStep) (*StepReport, error) {
	switch {
	case step.Patch != nil:
		if err := c.patchNodeRequest(ctx, step.Patch); err != nil {
			return nil, err
		}
		return &StepReport{Index: index, Kind: "patch", Passed: true, Message: "patched fake Node request annotation"}, nil
	case step.Expect != nil:
		return c.expectStatus(ctx, index, step.Expect)
	default:
		return nil, fmt.Errorf("step %d had no recognized action", index)
	}

[Copilot]

Pull request overview

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

Suppressed comments (5)

tools/storm/aclagent/utils/config/config.go:4

• ArtifactsDir default is "." here, but other storm scenarios default it to "/tmp" (e.g. tools/storm/servicing/utils/config/config.go:4 and tools/storm/rollback/utils/config/config.go:4). Keeping this consistent avoids surprising behavior when running different scenarios locally and matches the docs’ stated default.

	ArtifactsDir          string `help:"Directory containing artifacts for the VM" default:"."`

tools/storm/utils/vm/qemu/qemu.go:24

• ImagePattern is compiled as a regex (tools/storm/utils/file/file.go:13) and the default pattern contains an unescaped . before qcow2, so it will also match names like ...testimageXqcow2 and can accidentally match unexpected files (or trigger the "multiple files found" error). Escaping the dot makes the default behave as intended.

	SecureBoot   bool   `help:"Enable secure boot for the VM" default:"false"`
	SerialLog    string `help:"Path to the serial log file" default:"/tmp/trident-vm-verity-test.log"`
	ImagePattern string `help:"Regex pattern used to find the base VM image (.qcow2) in the artifacts directory" default:"^trident-vm-.*-testimage.qcow2$"`

tools/storm/aclagent/README.md:46

• This describes prepareVmForAclAgent as using localhost:<port> via reverse-SSH tunnels and running enable --now, but the implementation writes host-IP endpoints (see tools/storm/aclagent/tests/update.go:262-277) and does systemctl enable + systemctl restart (update.go:360-366). Updating this keeps the doc aligned with the actual harness behavior (and the rationale about tunnels not surviving reboot).

`prepareVmForAclAgent` writes `/etc/trident/trident-acl-agent.conf` pointing
at the `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM,
then runs `systemctl enable --now trident-acl-agent.service`. Before that
runs, the service simply isn't started -- no crash-looping, no log noise.

tools/storm/aclagent/README.md:89

• This section still says the scenario uses shim-based reboot interception and depends on reverse SSH tunnels, but the tests wait for a real reboot by polling SSH reachability (tools/storm/aclagent/tests/update.go:403-429) and explicitly avoid tunnels for the fake endpoints (update.go:124-127). This mismatch will confuse anyone debugging reboot-related failures.

This scenario keeps the shim-based reboot interception from the old tester.
That is less realistic than a full VM reboot, but it keeps the test deterministic
and lets the storm runner hold the reverse SSH tunnels and in-process fake services
steady while the agent drives the finalize path.

docs/Development/Testing/TridentAclAgent-Tests.md:142

• The check-deployment description says the config points at localhost:<port> via reverse-SSH forwarding and that it runs enable --now, but the harness uses a host-reachable IP/port (tests/update.go:262-277) and performs systemctl enable + systemctl restart (update.go:360-366). Aligning this documentation matters because it affects how people reason about connectivity across the real reboot.

1. **deploy-vm** — Copies the base qcow2 image and creates a QEMU VM
2. **check-deployment** — Verifies the VM booted and is accessible via SSH;
   writes `/etc/trident/trident-acl-agent.conf` pointing at the
   `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM, then
   runs `systemctl enable --now trident-acl-agent.service`

[Copilot]

Pull request overview

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

Suppressed comments (10)

tools/storm/aclagent/proxies/rp.go:83

• Issue: decodeStatus() errors are ignored, so a malformed acl.azure.com/update-status annotation will be silently treated as “no status yet” and the step will just time out, hiding the real failure.

Evidence: status, _ := decodeStatus(node) drops the error.

Suggestion: handle the error explicitly and fail the step with the decode error + raw annotation value so the scenario report is self-diagnosing.

		node, err := c.getNode(ctx)
		if err != nil {
			return nil, err
		}
		status, _ := decodeStatus(node)

tools/storm/aclagent/tests/update.go:128

• Issue: The fake apiserver binds to 0.0.0.0, which exposes the unauthenticated test endpoint on all host interfaces. This is avoidable (and can be surprising on developer machines) since the VM only needs it reachable via HostEndpointIP.

Evidence: ListenAndServe(..., "0.0.0.0:<port>").

Suggestion: bind to testConfig.HostEndpointIP (or another explicit listen address) so the server is only exposed where needed.

	// Bind on all interfaces (not 127.0.0.1) so the VM can reach the fake
	// apiserver directly over the libvirt NAT network at testConfig.HostEndpointIP,
	// instead of relying on reverse SSH tunnels. Tunnels don't survive a real
	// VM reboot; a real host IP does.
	if _, err := apiServer.ListenAndServe(ctx, fmt.Sprintf("0.0.0.0:%d", testConfig.APIServerPort)); err != nil {

tools/storm/aclagent/tests/update.go:158

• Issue: The fake image server binds to 0.0.0.0, exposing a local artifact over unauthenticated HTTP on all host interfaces.

Evidence: imageServer.ListenAndServe(..., "0.0.0.0:<port>").

Suggestion: bind to testConfig.HostEndpointIP so it’s only reachable on the interface the VM uses.

			return fmt.Errorf("failed to hash image %s: %w", imagePath, err)
		}
		imageServer := &stormproxies.ImageServer{ImagePath: imagePath}
		if _, err := imageServer.ListenAndServe(ctx, fmt.Sprintf("0.0.0.0:%d", testConfig.ImageServerPort)); err != nil {
			return fmt.Errorf("failed to start fake image server: %w", err)

tools/storm/aclagent/tests/update.go:173

• Issue: The fake Nebraska endpoint binds to 0.0.0.0, exposing an unauthenticated update server mock on all host interfaces.

Evidence: nebraska.ListenAndServe(..., "0.0.0.0:<port>").

Suggestion: bind to testConfig.HostEndpointIP so it’s only reachable on the interface the VM uses.

		SHA384:      nebraskaSHA384,
		PackageName: nebraskaPackageName,
	}}
	if _, err := nebraska.ListenAndServe(ctx, fmt.Sprintf("0.0.0.0:%d", testConfig.NebraskaPort)); err != nil {
		return fmt.Errorf("failed to start fake Nebraska endpoint: %w", err)

tools/storm/aclagent/tests/rollback.go:42

• Issue: The fake apiserver in rollback binds to 0.0.0.0, exposing an unauthenticated test endpoint on all host interfaces.

Evidence: ListenAndServe(..., "0.0.0.0:<port>").

Suggestion: bind to testConfig.HostEndpointIP (or another explicit listen address) to limit exposure to the interface the VM uses.

	nodeStore := stormproxies.NewNodeStore(stormproxies.NewSeedNode(testConfig.NodeName, map[string]string{}))
	apiServer := stormproxies.NewAPIServer(testConfig.NodeName, nodeStore)
	if _, err := apiServer.ListenAndServe(ctx, fmt.Sprintf("0.0.0.0:%d", testConfig.APIServerPort)); err != nil {
		return fmt.Errorf("failed to start fake apiserver: %w", err)
	}

tools/storm/aclagent/trident.go:72

• Issue: On error paths, runTestCase calls tc.FailFromError(err) but then continues and unconditionally logs the test as “passed”. That makes logs misleading and can mask follow-on errors after a known failure.

Evidence: after tc.FailFromError(err) the function falls through to logrus.Infof("test case '%s' passed"...).

Suggestion: return immediately after FailFromError, and only log “passed” when there was no error.

	if testCaseSpecificConfig.OutputPath != "" {
		testCaseSpecificConfig.OutputPath = filepath.Join(testCaseSpecificConfig.OutputPath, tc.Name())
		if err := os.MkdirAll(testCaseSpecificConfig.OutputPath, 0o755); err != nil {
			tc.FailFromError(err)
		}

docs/Development/Testing/TridentAclAgent-Tests.md:142

• Issue: The doc says check-deployment writes config pointing at localhost:<port> via reverse SSH tunnels, but the harness now configures the VM to talk directly to the test runner at HostEndpointIP:<port> (and avoids tunnels specifically because they don’t survive a real reboot).

Evidence: RunABUpdate binds the fake apiserver on 0.0.0.0 and advertises http://<HostEndpointIP>:<port> to the VM.

Suggestion: update this step description to match the actual networking model (host-reachable IP/port, no reverse tunnels).

2. **check-deployment** — Verifies the VM booted and is accessible via SSH;
   writes `/etc/trident/trident-acl-agent.conf` pointing at the
   `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM, then
   runs `systemctl enable --now trident-acl-agent.service`

docs/Development/Testing/TridentAclAgent-Tests.md:157

• Issue: Documented default for --artifacts-dir doesn’t match the scenario’s flag default.

Evidence: docs list /tmp, but TestConfig.ArtifactsDir defaults to ..

Suggestion: update the doc table default (or the flag default) so they stay consistent.

| `--artifacts-dir` | Directory containing VM images | `/tmp` |

tools/storm/aclagent/README.md:46

• Issue: This README says the scenario uses localhost:<port> endpoints via reverse SSH forwarding, but the harness configures the VM to reach the fake services directly on HostEndpointIP:<port> (and intentionally avoids tunnels for reboot survival).

Evidence: prepareVmForAclAgent writes endpoints using HostEndpointIP.

Suggestion: update the README wording to match the actual networking model.

`prepareVmForAclAgent` writes `/etc/trident/trident-acl-agent.conf` pointing
at the `localhost:<port>` endpoints storm reverse-SSH-forwards into the VM,
then runs `systemctl enable --now trident-acl-agent.service`. Before that
runs, the service simply isn't started -- no crash-looping, no log noise.

tests/images/trident-vm-testimage/README.md:25

• Issue: This README says the scenario should write “localhost proxy endpoints”, but the harness configures the VM to talk to the test runner over a host-reachable IP (HostEndpointIP) rather than SSH-tunneled localhost.

Evidence: prepareVmForAclAgent uses HostEndpointIP for the apiserver/Nebraska endpoints.

Suggestion: update this text to describe writing <host-endpoint-ip>:<port> endpoints (reachable from the VM) instead of localhost.

into it there is no test harness left to `systemctl enable --now` it. Neither
image preseeds /etc/trident/trident-acl-agent.conf with runner-specific
tunnel ports; the test scenario should SSH in after boot and write the real
localhost proxy endpoints for Nebraska and the Kubernetes API server.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

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

Suppressed comments (1)

.pipelines/templates/stages/testing_acl_agent/trident-acl-agent-test.yml:131

• The verboseLogging template parameter is declared but ignored: the bash step hard-codes FLAGS="-a --verbose", forcing verbose output even when verboseLogging: false and making the parameter dead. Wire the parameter into the flag construction (or remove the parameter entirely).

          - bash: |
              set -eux

              FLAGS="-a --verbose"

[bfjelds (bfjelds)]

/azp run [GITHUB]-trident-pr-e2e

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

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

Suppressed comments (2)

.pipelines/templates/stages/testing_acl_agent/trident-acl-agent-test.yml:131

• The bash step hard-codes FLAGS="-a --verbose", so the verboseLogging parameter declared at the top of the template is unused. This makes verbose logs mandatory even when callers set verboseLogging: false and makes the parameter misleading/dead.

              set -eux

              FLAGS="-a --verbose"

tools/storm/aclagent/proxies/nebraska.go:173

• ListenAndServe sets NEBRASKA_DB_URL, but on the error paths after that (failed migrations init, seed failure, or listen failure) it returns without unsetting the env var. If the storm process runs additional scenarios/tests afterwards, they may unexpectedly pick up a stale DB URL pointing at a stopped container.

	a, err := api.NewWithMigrations(api.OptionInitDB)
	if err != nil {
		stopEphemeralPostgres(containerID)
		return nil, fmt.Errorf("failed to initialize Nebraska API against ephemeral Postgres: %w", err)
	}
	p.api = a
	p.handler = omaha.NewHandler(a)

	if err := p.seed(); err != nil {
		stopEphemeralPostgres(containerID)
		return nil, fmt.Errorf("failed to seed Nebraska scenario: %w", err)
	}

	listener, err := net.Listen("tcp", listenAddr)
	if err != nil {
		stopEphemeralPostgres(containerID)
		return nil, fmt.Errorf("failed to listen on %s: %w", listenAddr, err)
	}

[Copilot]

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated 4 comments.

Suppressed comments (2)

tools/storm/aclagent/tests/rollback.go:133

• Issue: This does not establish that the no-op rollback avoided a reboot. Evidence: GetVmIP only queries the VM provider for an address; QEMU uses the libvirt DHCP lease, which can remain present while the guest reboots, and its address helper retries until an IP exists rather than reporting temporary unreachability. A single check can also run before a delayed reboot starts. Suggestion: capture the guest boot ID before submitting the second rollback and verify it remains unchanged after a bounded no-reboot observation window (or continuously probe SSH and fail on any outage).

	if _, err := stormvm.GetVmIP(vmConfig); err != nil {

tools/storm/aclagent/tests/rollback.go:119

• Issue: This regression expectation does not prove that servicing_kind identified an empty rollback chain. Evidence: it accepts any OperationFailed result, including unrelated tridentd, connection, or agent failures; RPClient currently discards the status message and only matches operation/id/code. Suggestion: expose and assert the specific no-active-servicing reason (or another dedicated discriminator) for this step.

		{Expect: &stormproxies.ExpectStep{OperationID: "dddddddd-4444-4444-4444-444444444444", Operation: "rollback", Code: "OperationFailed", Timeout: 60 * time.Second}},

[Copilot]

Pull request overview

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

Suppressed comments (4)

tools/storm/aclagent/tests/rollback.go:87

• Issue: This post-reboot call can restart the agent while its automatically started instance is executing the pending commit. Evidence: the base root was enabled earlier in this same test, the fake API server remains the same across this reboot, and prepareVmForAclAgent unconditionally runs systemctl restart trident-acl-agent.service; that function’s own comment notes that interrupting a commit can leave the retry failing with “Servicing is active.” Suggestion: do not re-run preparation here; let the boot-started agent reconnect and complete, then poll the commit annotation.

	// Restart the agent so it reconnects to this test case's fresh fake
	// apiserver instance (see prepareVmForAclAgent's doc comment in
	// update.go). /var/lib/kubelet is its own dedicated ext4 partition, so
	// the fake kubeconfig itself already persists across the rollback
	// reboot's root swap and doesn't need to be re-delivered.
	if err := prepareVmForAclAgent(vmConfig.VMConfig, vmIP, testConfig); err != nil {
		return fmt.Errorf("failed to reconfigure ACL agent on post-rollback-reboot root: %w", err)

tools/storm/aclagent/tests/rollback.go:135

• Issue: This does not verify that the no-op rollback avoided a reboot. Evidence: GetVmIP only queries libvirt/Azure for the assigned address; a DHCP/public IP generally remains available while the guest is rebooting, and no SSH connection is attempted. Suggestion: poll SSH for a short grace period and fail if it becomes unreachable, so the regression test actually detects an unexpected reboot.

	// A no-op rollback must not trigger another reboot: the VM should
	// still be reachable immediately, with no reboot wait needed.
	if _, err := stormvm.GetVmIP(vmConfig); err != nil {
		collectAclArtifactsBestEffort(vmConfig.VMConfig, vmIP, testConfig.OutputPath)
		return fmt.Errorf("VM appears to have rebooted (or become unreachable) after a no-op rollback, which should not trigger a reboot: %w", err)

.pipelines/templates/stages/testing_acl_agent/trident-acl-agent-test.yml:135

• Issue: Failed runs can leave the libvirt VM behind on the shared runner. Evidence: the scenario explicitly skips cleanup-vm after a dependency failure, while Cleanup only destroys the VM when ForceCleanup is true; this pipeline does not pass that flag. Suggestion: enable forced cleanup as the existing servicing and rollback pipelines do.

              sudo ./bin/storm-trident run aclagent $FLAGS \
                --output-path $(ob_outputDirectory) \
                --artifacts-dir $(Build.ArtifactStagingDirectory) \
                --ssh-private-key-path ~/.ssh/id_rsa

tools/storm/aclagent/proxies/nebraska.go:151

• Issue: NEBRASKA_DB_URL is process-global but is cleared asynchronously by an older proxy’s shutdown goroutine. Evidence: the newly added test starts several proxies in sequential subtests; after one cleanup signals cancellation, its goroutine can run after the next proxy sets its URL and unset the new value. Startup failures after Setenv also leave the dead URL installed. Suggestion: save the previous value, set the URL only around api.NewWithMigrations, restore it immediately afterward on every path, and remove the delayed unconditional Unsetenv.

	// api.New only reads NEBRASKA_DB_URL from the environment - there is no
	// functional option to set a custom DSN - so this is the only way to
	// point it at our ephemeral container. Safe here because exactly one
	// NebraskaProxy is ever instantiated per storm test process.
	if err := os.Setenv("NEBRASKA_DB_URL", dbURL); err != nil {

[bfjelds (bfjelds)]

/azp run [GITHUB]-trident-pr-e2e

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

tools/storm/aclagent/tests/rollback.go:135

• Issue: This does not verify that the no-op rollback avoided a reboot. Evidence: GetVmIP only queries libvirt/Azure for an assigned address; for QEMU it reads the DHCP lease, which normally remains present while the guest is rebooting, so this check can succeed throughout an unintended reboot. Suggestion: observe SSH/boot identity for a bounded grace period (for example, read /proc/sys/kernel/random/boot_id before the request and assert the same ID after continuous SSH availability) instead of querying the lease.

	if _, err := stormvm.GetVmIP(vmConfig); err != nil {
		collectAclArtifactsBestEffort(vmConfig.VMConfig, vmIP, testConfig.OutputPath)
		return fmt.Errorf("VM appears to have rebooted (or become unreachable) after a no-op rollback, which should not trigger a reboot: %w", err)

[bfjelds (bfjelds)]

/azp run [GITHUB]-trident-pr-e2e

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[bfjelds (bfjelds)]

/azp run [GITHUB]-trident-pr-e2e

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

🟡 Changes recommended

The Nebraska preflight consumes the pending update, and the no-reboot rollback assertion cannot detect a reboot.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

tools/storm/aclagent/tests/rollback.go:135

• Issue: This does not verify that the no-op rollback avoided a reboot. Evidence: GetVmIP only reads the libvirt/Azure-assigned address; that address remains available while the guest reboots, and the QEMU implementation retries indefinitely until an address exists. Consequently, an erroneous reboot still passes this regression assertion. Suggestion: monitor SshCommandCombinedOutput(..., "true") for a bounded interval after the status and fail if SSH ever becomes unreachable.

	// A no-op rollback must not trigger another reboot: the VM should
	// still be reachable immediately, with no reboot wait needed.
	if _, err := stormvm.GetVmIP(vmConfig); err != nil {
		collectAclArtifactsBestEffort(vmConfig.VMConfig, vmIP, testConfig.OutputPath)
		return fmt.Errorf("VM appears to have rebooted (or become unreachable) after a no-op rollback, which should not trigger a reboot: %w", err)

• Files reviewed: 30/31 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The no-reboot assertion is ineffective, and Nebraska cleanup can leak resources or race subsequent proxies.

Review details

Suppressed comments (2)

tools/storm/aclagent/tests/rollback.go:135

• Issue: This does not verify that the VM stayed up after the no-op rollback. Evidence: GetVmIP only queries the assigned address; on QEMU it reads libvirt's DHCP lease (tools/storm/utils/vm/qemu/qemu.go:130-209), which normally remains present during a reboot and can therefore return the same IP after an erroneous reboot. Suggestion: capture the guest boot ID before the second request, then use bounded SSH polling to assert the VM remains reachable and the boot ID does not change throughout the no-reboot observation window.

	if _, err := stormvm.GetVmIP(vmConfig); err != nil {
		collectAclArtifactsBestEffort(vmConfig.VMConfig, vmIP, testConfig.OutputPath)
		return fmt.Errorf("VM appears to have rebooted (or become unreachable) after a no-op rollback, which should not trigger a reboot: %w", err)

tools/storm/aclagent/proxies/nebraska.go:181

• Issue: Cancellation does not synchronously clean up the external Postgres resource. Evidence: the context only wakes this background goroutine; callers have no way to wait for docker rm or environment restoration before the next scenario or process exit. A standalone run-ab-update can therefore leave the container running, and sequential proxy tests can race when an older goroutine unsets NEBRASKA_DB_URL after a newer proxy sets it. Suggestion: expose an awaited Close/Wait lifecycle for NebraskaProxy and have callers defer it; restore the environment only if it still contains this proxy's DB URL.

	go func() {
		<-ctx.Done()
		_ = server.Shutdown(context.Background())
		stopEphemeralPostgres(containerID)
		// Unset so it doesn't leak into other tests/scenarios running in
		// this same process after this proxy has shut down.
		_ = os.Unsetenv("NEBRASKA_DB_URL")

• Files reviewed: 30/31 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Five unresolved moderate issues affect pipeline configuration, image freshness, scenario sequencing, server teardown, and reboot validation.

Review details

Suppressed comments (5)

Previously missed (3) — in code that hasn't changed since the last review.

.pipelines/templates/e2e-template.yml:277

• Issue: The ACL image build ignores the parent pipeline's selected Image Customizer build. Evidence: this template exposes micBuildType/micVersion, but the inclusion passes neither, unlike the servicing stage immediately above; dev/preview and pinned-version runs therefore silently use the ACL template's release/*.*.* defaults. Suggestion: forward both parent parameters.
  tools/storm/aclagent/proxies/apiserver.go:238
• Issue: API-server shutdown races the next test case. Evidence: cancelling RunABUpdate only wakes this goroutine; it does not wait for Shutdown to close the listener, while RunRollback immediately binds the same APIServerPort. Scheduling can therefore make rollback fail intermittently with address already in use. Suggestion: expose/await synchronous server teardown (or explicitly close the returned listener) before the scenario returns and the next test starts.
  tools/storm/aclagent/proxies/rp.go:50
• Issue: Failed expectations do not stop this stateful scenario. Evidence: RunScenario records Passed=false but continues to later steps, so the update sequence still patches finalize after its preceding stage-success expectation times out or reports failure. Suggestion: stop processing steps after the first failed report so dependent operations are not executed with failed preconditions.

Makefile:1228

• Issue: The base-image target does not depend on the new systemd override that the YAML embeds. Evidence: changing files/trident-acl-agent-override.conf leaves an existing qcow2 newer than every listed prerequisite, so make artifacts/trident-vm-acl-agent-testimage.qcow2 reuses an image with stale agent settings. Suggestion: add the override file as an explicit prerequisite, as is done for other image-specific files in nearby targets.

	$(VM_IMAGE_PATH_PREFIX)/baseimg-acl-agent.yaml \
	$(VM_IMAGE_PATH_PREFIX)/files/id_rsa.pub \
	artifacts/rpm-overrides

tools/storm/aclagent/tests/rollback.go:133

• Issue: This does not verify that the VM stayed reachable. Evidence: GetVmIP only queries the assigned address; Azure retains it during reboot, and QEMU reads the persistent DHCP lease (and retries until an address appears), so an unintended second reboot can still pass this assertion. Suggestion: observe direct SSH connectivity for a bounded no-reboot window and fail if any outage occurs.

	if _, err := stormvm.GetVmIP(vmConfig); err != nil {

• Files reviewed: 30/31 changed files
• Comments generated: 0 new
• Review effort level: Balanced