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.

[Copilot]

Pull request overview

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

Suppressed comments (3)

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

• Before overwriting NEBRASKA_DB_URL, capture any existing value so the shutdown hook can restore it (instead of always unsetting). This prevents tests/tools that already rely on NEBRASKA_DB_URL in the same process from breaking after this proxy runs.

	if err := os.Setenv("NEBRASKA_DB_URL", dbURL); err != nil {
		stopEphemeralPostgres(containerID)
		return nil, fmt.Errorf("failed to set NEBRASKA_DB_URL: %w", err)
	}

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

• The shutdown hook currently unsets NEBRASKA_DB_URL unconditionally, which can delete a caller-provided value. Restore the previous value when one existed; otherwise unset.

		// 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")
	}()

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

• ListenAndServe temporarily overrides NEBRASKA_DB_URL for the embedded Nebraska API, but the shutdown path currently unsets it unconditionally. To avoid clobbering a caller-provided NEBRASKA_DB_URL, store the prior value on the proxy so it can be restored on shutdown.

	containerID string
	dbURL       string
	api         *api.API
	handler     *omaha.Handler
	appID       string

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

• In NebraskaProxy.ListenAndServe, NEBRASKA_DB_URL is set globally but not unset on the early-return error paths after Setenv succeeds (e.g., api.NewWithMigrations/seed/net.Listen failures). That environment leak can affect subsequent tests/scenarios running in the same process, because later Nebraska instances will read the stale DB URL.

Unset NEBRASKA_DB_URL alongside container teardown on those error paths (the ctx-cancel goroutine only runs on the success path).

	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)
	}

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

• NEBRASKA_DB_URL is set globally, but on error paths after os.Setenv (e.g. DB init/seed/listen failures) it is never unset. That can leak into later tests/scenarios running in the same process and point them at a stale/removed container, making failures flaky and hard to diagnose.

Unset NEBRASKA_DB_URL before returning on these error paths (it is already unset on the happy-path shutdown goroutine).

	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)
	}

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

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

• The shutdown goroutine always unsets NEBRASKA_DB_URL, which will wipe any pre-existing value a developer might have had in their environment (and it won’t match the restore logic needed for the failure paths). Prefer restoring the prior value captured before os.Setenv (or unsetting only if it wasn’t previously set).

	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")
	}()

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

• waitForServiceActive only tracks lastErr. If systemctl is-active returns a non-"active" state with exit code 0 (common), lastErr ends up nil and the final error message becomes misleading. Track and report the last observed is-active output as well (e.g. "activating", "failed"), so timeouts are diagnosable without reading the embedded journal.

func waitForServiceActive(cfg stormvmconfig.VMConfig, vmIP, service string, timeout time.Duration) error {
	deadline := time.Now().Add(timeout)
	var lastErr error
	for time.Now().Before(deadline) {
		out, err := stormssh.SshCommandCombinedOutput(cfg, vmIP, fmt.Sprintf("sudo systemctl is-active %s", service))
		if err == nil && strings.TrimSpace(out) == "active" {
			return nil
		}
		lastErr = err
		time.Sleep(2 * time.Second)

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

• The template defines a verboseLogging parameter, but the bash step hard-codes --verbose unconditionally (FLAGS="-a --verbose"), so the parameter currently has no effect. Wire --verbose to the parameter (matching the pattern used by other storm pipeline stages) so callers can disable noisy logs when desired.

          - bash: |
              set -eux

              FLAGS="-a --verbose"

              sudo ./bin/storm-trident run aclagent $FLAGS \

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

• ListenAndServe sets NEBRASKA_DB_URL but several early-return error paths after that (api.NewWithMigrations, p.seed, net.Listen) don’t restore/unset it, so a failed proxy startup can leak a stale DB URL into later tests/scenarios running in the same process (and it can also clobber an existing NEBRASKA_DB_URL). Capture the prior value and restore it on every failure after the env var is modified.

	// 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 {
		stopEphemeralPostgres(containerID)
		return nil, fmt.Errorf("failed to set NEBRASKA_DB_URL: %w", err)
	}

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

• ListenAndServe sets NEBRASKA_DB_URL before initializing/seeding Nebraska, but on the early error paths (api.NewWithMigrations, seed, net.Listen) it returns without unsetting the env var. That leaks process-global state into subsequent storm scenarios/tests running in the same process and can cause very confusing cross-test failures.

	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)
	}

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

• ListenAndServe sets NEBRASKA_DB_URL globally, but on early-return error paths after the Setenv succeeds it never unsets it. That can leak a stale DSN into later storm scenarios/tests running in the same process (and make subsequent NebraskaProxy initializations behave unpredictably). Unset the env var before returning on these error paths (you can keep the existing unset in the ctx-cancel shutdown goroutine for the normal success path).

	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)
	}

[Copilot]

Pull request overview

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

Suppressed comments (2)

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

• NEBRASKA_DB_URL is set to point Nebraska's API at the ephemeral Postgres container, but on later failure paths (api.NewWithMigrations, seed, or net.Listen) the function returns without unsetting the env var. That can leak the ephemeral DSN into subsequent tests/scenarios running in the same process and cause confusing cross-test behavior.

	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)
	}

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

• The --image-path default description says the runner picks the first *.cosi under --artifacts-dir, but the implementation uses stormfile.FindFile(...) which errors if there are multiple matches. Also, QemuConfig now supports --image-pattern for selecting the base qcow2 image, but this flag isn’t listed here, making it harder to resolve the common "multiple files found" error when an artifacts directory contains more than one *-testimage.qcow2.

| `--image-path` | Real `.cosi` update image to serve during staging | first `*.cosi` found under `--artifacts-dir` |
| `--verbose` | Enable verbose logging | `false` |
| `--test-case-to-run` | Run a specific test case only | `all` |

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

• In NebraskaProxy.ListenAndServe, if api.NewWithMigrations fails after setting NEBRASKA_DB_URL, the function returns without unsetting the env var. This can leak a stale DB URL into later tests/scenarios running in the same process and cause confusing follow-on failures.

	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)
	}

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

• In NebraskaProxy.ListenAndServe, if seeding fails after NEBRASKA_DB_URL is set, the env var is left set even though the ephemeral Postgres container is torn down. Unset NEBRASKA_DB_URL on this error path to avoid leaking a dead DSN into later scenarios.

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

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

• The template defines a verboseLogging parameter, but the script always forces "--verbose" via FLAGS. This makes the parameter ineffective and prevents running the stage with non-verbose output.

          - bash: |
              set -eux

              FLAGS="-a --verbose"

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

• In NebraskaProxy.ListenAndServe, if net.Listen fails after NEBRASKA_DB_URL is set, the env var remains set even though the proxy is aborting. Unset NEBRASKA_DB_URL on this error path to avoid cross-test contamination.

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

[bfjelds (bfjelds)]

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

[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)