Speed up agent integration tests - #457
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
WalkthroughThe integration environment now uses persistent SSH-based agent access and centralized baseline restoration. Remote commands, device handling, cleanup, and file transfer are batched. Integration specs wait for agent readiness directly and remove repeated tunnel, configuration, and device teardown steps. Firewall checks use consistent nftables snapshots. Filesystem checks retry permission and ownership assertions. CI setup uses a dedicated blobstore assets directory, SSH proxying, and ShellCheck validation. A local integration-testing guide was added. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the BOSH Agent integration test harness to reduce SSH round-trips, centralize VM setup/teardown into a single “clean baseline” helper, and make the suite easier to run from a workstation against a BOSH-deployed VM—resulting in substantially faster and less flaky integration runs.
Changes:
- Centralize and optimize suite setup/teardown via
RestoreCleanBaseline, batched remote command execution, and reduced per-spec redundant cleanup. - Replace per-spec tunnel startup with a persistent SSH client +
WaitForAgent, and start fake-blobstore once at suite level with a dedicated assets directory. - Update multiple integration specs to rely on suite-level baseline restore and to avoid TOCTOU-style flake patterns by asserting against a single polled snapshot.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| integration/v1_apply_test.go | Removes redundant per-test cleanup/tunnel usage; improves permission assertions to avoid TOCTOU flakes. |
| integration/user_permissions_test.go | Simplifies setup to rely on suite baseline; switches to WaitForAgent. |
| integration/update_settings_test.go | Removes redundant cleanup/detach; switches to WaitForAgent. |
| integration/test_environment.go | Major harness refactor: batched SSH scripts, baseline restore, device teardown changes, persistent SSH streaming uploads, WaitForAgent. |
| integration/system_mounts_test.go | Removes per-test device detach; keeps targeted unmount cleanup. |
| integration/sync_dns_test.go | Removes redundant cleanup/blobstore start; switches to WaitForAgent. |
| integration/run_script_test.go | Removes per-test agent config restore and device detach; switches to WaitForAgent. |
| integration/remove_file_test.go | Removes per-test agent config restore and device detach; switches to WaitForAgent. |
| integration/raw_ephemeral_disk_test.go | Removes redundant cleanup and per-test device detaches; relies on suite baseline. |
| integration/prepare_test.go | Removes per-test agent config restore/blobstore start and device detach; switches to WaitForAgent. |
| integration/nats_firewall_test.go | Removes redundant agent config updates/detaches; fixes TOCTOU by polling a single nft snapshot with SatisfyAll. |
| integration/integration_suite_test.go | Establishes suite-level baseline restore and suite-level fake-blobstore startup; simplifies AfterEach teardown path. |
| integration/instance_info_test.go | Removes redundant cleanup/detach; switches to WaitForAgent. |
| integration/file_settings_test.go | Removes redundant agent config restore and per-test detach; relies on suite baseline. |
| integration/fetch_logs_with_signed_url_test.go | Removes per-test agent config restore/blobstore start/detach; switches to WaitForAgent. |
| integration/fetch_logs_test.go | Removes per-test agent config restore/detach; switches to WaitForAgent. |
| integration/ephemeral_disk_test.go | Removes redundant cleanup/detach; relies on suite baseline. |
| integration/delete_arp_entries_test.go | Removes redundant cleanup/detach; switches to WaitForAgent. |
| integration/compile_package_test.go | Removes redundant cleanup/blobstore start/detach; keeps fixture upload via new streaming copy. |
| integration/bundle_logs_test.go | Removes redundant agent config restore/detach; switches to WaitForAgent. |
| integration/apply_test.go | Removes per-test agent config restore/detach; improves permission assertions to avoid TOCTOU flakes. |
| docs/running_bosh_agent_integration_tests_from_local_machine.md | Adds a local-workstation guide for running the integration suite against a remote BOSH VM. |
| ci/tasks/test-integration.sh | Updates CI provisioning: safer optional scp flag handling, new blobstore assets dir, updated release assets path, adds ProxyJump config. |
Suppressed comments (1)
integration/test_environment.go:796
WaitForAgentno longer creates an SSH tunnel, but the log message still says "via ssh tunnel", which makes debugging harder when reading CI output.
for i := 1; i < 90; i++ {
t.writerPrinter.Printf("Trying to contact agent via ssh tunnel...")
time.Sleep(1 * time.Second)
_, err = t.AgentClient.Ping()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/tasks/test-integration.sh`:
- Around line 86-88: Update the copy_to_remote_host invocation to use the
existing release_folder variable as its upload destination instead of repeating
the literal release path, keeping the cleanup and upload targets synchronized.
In `@docs/running_bosh_agent_integration_tests_from_local_machine.md`:
- Around line 33-34: Update the account setup command for agent_test_user to be
idempotent, ensuring an existing user does not stop the subsequent group, shell,
SSH directory, copy, and ownership configuration. Guard user creation or
otherwise allow the remaining commands to execute independently on repeated
runs.
- Around line 42-43: Update the SSH key installation command in the documented
bosh invocation to append the local public key to agent_test_user’s existing
authorized_keys instead of overwriting it; replace the truncating redirection
while preserving the cleanup of /tmp/id_rsa.pub.
- Line 9: Update the BOSH CLI prerequisite in the guide to require version
7.10.4 or newer, and add the provided bosh --version verification command with
guidance to continue only when that minimum version is met.
In `@integration/test_environment.go`:
- Around line 113-127: Update the endpoint URL passed to
NewIntegrationAgentClient to derive its port from mbusPort instead of the
hardcoded 16868 literal, while retaining mbusUser, mbusPass, and localhost. Keep
DialContext and the existing transport behavior unchanged.
- Around line 916-917: Promote the hard-coded blobstore path to a package-level
constant, then update TestEnvironment.BlobstoreDir, StartBlobstore’s assets
argument, and CleanupDataDir’s cleanup script to use that shared constant so all
three operations target the same directory.
- Around line 437-441: Update the loop-device setup in the test environment flow
to replace the separate `losetup -f` discovery and attachment commands with a
single atomic `losetup --find --show` invocation against the created virtual
filesystem. Assign its printed device path to `loop`, then retain the existing
`devnum` lookup using that value.
- Around line 222-239: Update RestoreCleanBaseline to execute StopAgent,
configuration restoration, CleanupDataDir, CleanupLogFile, and ResetDeviceMap
unconditionally, while recording and returning only the first encountered error
after all cleanup stages complete. Preserve configDirty handling and ensure
later cleanup errors do not replace an earlier error.
- Around line 323-326: Update the cleanup loop for /virtualfs-* to compare the
exact backing-file field from losetup output rather than using substring
matching with grep -qF "$f". Ensure /virtualfs-2 is considered unattached when
only /virtualfs-21 is present, while preserving the existing sudo rm cleanup
behavior.
- Around line 290-291: Update the blobstore cleanup command in CleanupDataDir to
ensure /home/agent_test_user/blobstore exists before running find, or make the
sweep tolerate a missing directory so -e cannot abort cleanup. Also align the
doc comment near CleanupDataDir with the implemented find flag order: -maxdepth
1 -type f.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 33cc4073-ac52-488f-8cd2-59de0a1f3d6f
📒 Files selected for processing (23)
ci/tasks/test-integration.shdocs/running_bosh_agent_integration_tests_from_local_machine.mdintegration/apply_test.gointegration/bundle_logs_test.gointegration/compile_package_test.gointegration/delete_arp_entries_test.gointegration/ephemeral_disk_test.gointegration/fetch_logs_test.gointegration/fetch_logs_with_signed_url_test.gointegration/file_settings_test.gointegration/instance_info_test.gointegration/integration_suite_test.gointegration/nats_firewall_test.gointegration/prepare_test.gointegration/raw_ephemeral_disk_test.gointegration/remove_file_test.gointegration/run_script_test.gointegration/sync_dns_test.gointegration/system_mounts_test.gointegration/test_environment.gointegration/update_settings_test.gointegration/user_permissions_test.gointegration/v1_apply_test.go
💤 Files with no reviewable changes (3)
- integration/raw_ephemeral_disk_test.go
- integration/file_settings_test.go
- integration/ephemeral_disk_test.go
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
KauzClay
reviewed
Aug 3, 2026
|
My only musing revolves around the shift from isolated test environments to a shared global state. Is |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration/test_environment.go`:
- Line 651: Update the command construction around CombinedOutput to avoid
unsafe interpolation of remotePath: prefer invoking sudo and tee without a
shell, passing remotePath as a separate argument and using tee’s option
terminator where supported. If a shell command is required, apply shell-specific
quoting that prevents metacharacter interpretation and command substitution,
while preserving support for valid remote paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7b3f0c6-f426-43f4-a273-55d365219780
📒 Files selected for processing (1)
integration/test_environment.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (2)
integration/test_environment.go:794
- WaitForAgent no longer uses an SSH tunnel/port-forward, but this log line still says “via ssh tunnel”. Updating it will make failures easier to interpret when debugging local/CI runs.
t.writerPrinter.Printf("Trying to contact agent via ssh tunnel...")
docs/running_bosh_agent_integration_tests_from_local_machine.md:90
- The tests no longer shell out to system
ssh/scpfor their own operations (they use the Go SSH client), so this sentence is a bit misleading. Clarify that the integration tests readintegration/ssh-configfor the Go SSH client connection.
All test commands (both the Go native SSH client and the system `scp`/`ssh` calls) read from `src/bosh-agent/integration/ssh-config`. Create or update that file to point to the local tunnel. The Go code hardcodes the host name `agent_vm` for this file:
- add docs/running_bosh_agent_integration_tests_from_local_machine.md which outlines how to run the tests locally while targetting a remotely deployed bosh vm - update all ssh actions to use the integration/ssh-config file - refactor tunnel to agent to reuse existing ssh connection. Splits old method into two, one which runs in suite setup and a new helper which just waits for the agent to be ready that individual tests use
Every spec ran cleanup twice. The suite AfterEach already returned the VM to a
clean baseline (agent stopped, default agent.json, empty data dir, truncated
logs, no ephemeral devices), yet each file's BeforeEach repeated CleanupDataDir,
CleanupLogFile, and the default UpdateAgentConfig. CleanupDataDir alone is ~25
SSH round-trips, so running it twice dominated per-test setup.
Establish that baseline as an invariant through a single shared
RestoreCleanBaseline helper, so both the first spec (SynchronizedBeforeSuite)
and every subsequent teardown (AfterEach) go through the same code and can't
drift apart. Delete the duplicated cleanup from each file's BeforeEach, keeping
only test-specific config, CreateSettingsFile, and AttachDevice. Drop the
now-redundant AfterEach{ DetachDevice } blocks and make ResetDeviceMap the
single device-teardown path.
RestoreCleanBaseline stops the agent before cleaning: otherwise CleanupDataDir's
`fuser -km /var/vcap/data` kills the agent, systemd restarts it, and the restart
races the umount/rm of /var/vcap/data.
Each RunCommand opens a fresh SSH session through the jumpbox, so a helper built from N shell commands with Go-side control flow costs N latency round-trips. The cleanup and device helpers are deterministic shell logic, so push that logic server-side: each now fires one RunCommand carrying a `sudo bash -euo pipefail` heredoc batch (via the new runBatch helper) instead of N trips. - CleanupDataDir: fold the 5 detach calls, the monit-stopped wait (formerly ensureMonitStopped), the /tmp unmount, and directory recreation into one batch. - AttachDevice: loop over partitions server-side, read the loop device's major:minor straight from sysfs, and echo a parseable device map back; drops the per-partition `ls -al` debug trips. - ResetDeviceMap: move the whole udevadm-settle / retry / sleep loop (formerly forceDetachLoopDevice) server-side, preserving the Noble loop-device semantics verbatim. - DetachDevice: batch the mount|grep -> fuser -> umount -> rm sequence through a shared detach_mount bash function. - CleanupLogFile: run the log truncate and the systemd journal rotate in one batch instead of two RunCommands. The now server-side helpers AttachLoopDevice, forceDetachLoopDevice, and ensureMonitStopped are removed; the Go wrappers stay thin (invoke + parse). Batching removed the per-command SSH latency that used to space out systemd restarts, which exposed a crash-loop: a StopAgent -> CleanupDataDir -> StartAgent sequence would SIGKILL auditd (its log lives under the `fuser -km /var/log` target) and storm systemd past auditd's StartLimitBurst before the agent's bootstrap starts it. detach_mount now stops auditd cleanly before the fuser (as it already did for rsyslog), and StartAgent runs `systemctl reset-failed` on auditd/rsyslog/bosh-agent as a timing-independent safety net.
Contributor
they weren't isolated test envs even before this. They all share the same bosh deployment. But also, each spec kinda had their own cleanup, which was maybe worse? These changes just try to unify the cleanup to return the shared env to the same state after each run |
mkocher
force-pushed
the
resolute-integ-test
branch
from
August 3, 2026 20:41
2d02baf to
878db67
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
integration/test_environment.go:806
WaitForAgentstill logs "via ssh tunnel" even though the tunnel code was removed, and it sleeps before the first ping attempt. This makes logs confusing and slows fast-starting cases by an extra second per spec.
t.writerPrinter.Printf("Trying to contact agent via ssh tunnel...")
time.Sleep(1 * time.Second)
_, err = t.AgentClient.Ping()
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/tasks/test-integration.sh`:
- Line 88: Quote the release_folder argument in the release upload command,
preserving it as the intended single path argument and correcting the
surrounding command’s accidental string termination.
In `@docs/running_bosh_agent_integration_tests_from_local_machine.md`:
- Line 40: Synchronize the SSH key paths in the instructions around the bosh scp
command and the key configuration at line 86. Use the same configurable key-path
placeholder for both the private key and its corresponding .pub file, or
explicitly instruct users to update both paths when changing the default key.
- Line 43: Update the SSH key installation command in the documented bosh
integration-test setup to append the local public key to agent_test_user’s
authorized_keys instead of overwriting existing entries. Preserve the keys
copied earlier, using append or an equivalent merge-and-deduplicate approach.
In `@integration/compile_package_test.go`:
- Around line 61-63: Move the dummy_package.tgz fixture copy from the nested
BeforeEach into a nested JustBeforeEach that runs after the outer JustBeforeEach
and WaitForAgent() startup flow. Preserve the existing
testEnvironment.CopyFileToPath source and blobstore destination and its error
assertion.
In `@integration/nats_firewall_test.go`:
- Around line 79-82: Update the Eventually polling closures in
integration/nats_firewall_test.go at lines 79-82, 165-168, and 224-227 to return
both output and the error from testEnvironment.RunCommand, matching the existing
Windows closure pattern; stop discarding command failures so polling receives
the non-nil error.
In `@integration/v1_apply_test.go`:
- Around line 187-201: Extract the duplicated restarted-directory permission
loops into one shared integration helper, such as expectJobDirsRestored,
preserving the existing directory list, retry policy, stat command, and
mode/ownership regexp. Replace the loops at integration/v1_apply_test.go lines
187-201 and 309-323, and integration/apply_test.go lines 149-163 with calls to
that helper, passing each site’s existing environment and directories; remove
the unnecessary loop-variable rebinding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a8dfe332-66a0-45bf-ad0d-fc81df7a8c79
📒 Files selected for processing (23)
ci/tasks/test-integration.shdocs/running_bosh_agent_integration_tests_from_local_machine.mdintegration/apply_test.gointegration/bundle_logs_test.gointegration/compile_package_test.gointegration/delete_arp_entries_test.gointegration/ephemeral_disk_test.gointegration/fetch_logs_test.gointegration/fetch_logs_with_signed_url_test.gointegration/file_settings_test.gointegration/instance_info_test.gointegration/integration_suite_test.gointegration/nats_firewall_test.gointegration/prepare_test.gointegration/raw_ephemeral_disk_test.gointegration/remove_file_test.gointegration/run_script_test.gointegration/sync_dns_test.gointegration/system_mounts_test.gointegration/test_environment.gointegration/update_settings_test.gointegration/user_permissions_test.gointegration/v1_apply_test.go
💤 Files with no reviewable changes (3)
- integration/raw_ephemeral_disk_test.go
- integration/ephemeral_disk_test.go
- integration/file_settings_test.go
Eliminate setup work repeated on every spec. - Dirty-track the default agent.json. Only 3 of 49 specs override it, yet the suite AfterEach rewrote it (rm + scp + mv) after every spec. UpdateAgentConfig now sets a configDirty flag and RestoreCleanBaseline restores the default only when a spec actually changed it, so the 46 clean specs skip the restore. - Batch CreateSettingsFile's three settings-file removals into one round-trip; it runs in every spec's BeforeEach. - Hoist StartBlobstore to SynchronizedBeforeSuite. fake-blobstore is stable, stateless infrastructure (HTTP over the filesystem), so start it once rather than restarting it in ~15 specs' BeforeEach. Move its assets dir out of the home dir to a dedicated /home/agent_test_user/blobstore so the SSH keys and the binary are no longer web-servable, and have CleanupDataDir sweep transient top-level uploads from it each teardown (preserving the read-only release/ fixtures). Provisioning (ci/tasks/test-integration.sh) and the manual-setup doc create the dir owned by agent_test_user and stage release/ under it. Two profiling-driven speedups: - CopyFileToPath streams the local file over the persistent SSH tunnel (piping into `sudo tee`) instead of opening a fresh scp connection through the jumpbox plus a follow-up mv. A new scp handshake costs ~4s; reusing the tunnel makes it a single ~0.3s round-trip. Every spec uploads settings this way. - AttachDevice creates loop-device backing files sparsely with `truncate` rather than writing zeros with `dd` (~5.5s of 3x128MB writes per device spec). Loop devices back sparse files fine and size probes report the file size regardless of allocation.
Narrower timing windows exposed several latent races. - ephemeral_disk (root-disk-as-ephemeral): the spec fakes the root disk by swapping in a loop device; at bootstrap the agent runs blkid on the resolved root partition and exits with "Cannot get filesystem type for root file system" if the swapped-in device state hasn't settled. The old `dd` backing-file write incidentally let udev settle; sparse truncate removed that cushion. Add an explicit `udevadm settle` at the end of AttachPartitionedRootDevice. - DetachPartitionedRootDevice restored the real root node only at the very end, so any early return left /dev/sdb2 dangling and every later spec's agent hit the same root-fs error -- one flaky spec cascaded into a suite-wide timeout. Defer the restore (named return preserves the original error) so a failure stays contained to its spec. - nats_firewall (multi-url, ipv4, ipv6): the specs gated on an Eventually that rules existed, then asserted regexes against a separate `nft list` read. The agent manages the nats_access chain dynamically (flush + re-add on refresh), so a rule can transiently vanish between the two reads. Fold all assertions into one Eventually(SatisfyAll(...)) over a single polled snapshot. - apply and v1_apply (x2): the same gate-then-separate-read TOCTOU on directory permissions after a restart. Assert the perms inside Eventually so it polls the actual asserted state.
- Remove the emptyReader type and its Read method; nothing references it after
the setup refactor.
- Quote the optional ${scp_flag} in copy_to_remote_host (shellcheck SC2086) so
the empty-flag call still works.
mkocher
force-pushed
the
resolute-integ-test
branch
from
August 3, 2026 21:21
878db67 to
6847028
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (3)
integration/test_environment.go:804
- WaitForAgent no longer uses an SSH tunnel (it pings the agent over the existing SSH client dialer), but the progress log still says "via ssh tunnel", which is misleading when debugging failures.
t.writerPrinter.Printf("Trying to contact agent via ssh tunnel...")
integration/test_environment.go:704
- StartAgent stops/starts the unit named "agent", but the new systemd
reset-failedcall only resetsbosh-agent.service. If the start-limit is tripped onagent.service, this won't clear it and the subsequentsystemctl start agentcan still fail.
_, err = t.RunCommand("sudo systemctl reset-failed auditd.service rsyslog.service bosh-agent.service 2>/dev/null || true")
integration/test_environment.go:476
t.deviceMapis no longer read anywhere (ResetDeviceMap now querieslosetupdirectly), so this assignment is dead state and can mislead future readers into thinking the map is still meaningful.
t.deviceMap[deviceNum] = fields[2]
t.writerPrinter.Printf("AttachDevice[%s]: loop=%s node=%s (b %s)\n", fields[3], fields[2], fields[3], fields[4])
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (1)
integration/test_environment.go:213
- RestoreCleanBaseline currently records only the first cleanup error and silently drops any subsequent failures. When the suite is flaking, this can hide the real failing teardown step and make debugging much harder (e.g., StopAgent fails, but the later CleanupDataDir/ResetDeviceMap errors are lost). Consider aggregating and returning all errors (bosh-utils/errors has NewMultiError) so CI shows the full failure set.
func (t *TestEnvironment) RestoreCleanBaseline() error {
var firstErr error
record := func(err error) {
if err != nil && firstErr == nil {
firstErr = err
mkocher
force-pushed
the
resolute-integ-test
branch
from
August 3, 2026 22:30
b7c9911 to
87b900d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (2)
integration/test_environment.go:689
- WaitForAgent still logs "Trying to contact agent via ssh tunnel...", but the SSH tunnel process was removed and the agent client now dials the MBUS over the existing SSH connection. This message is misleading when debugging failures.
for i := 1; i < 90; i++ {
t.writerPrinter.Printf("Trying to contact agent via ssh tunnel...")
time.Sleep(1 * time.Second)
_, err = t.AgentClient.Ping()
integration/test_environment.go:706
- StartBlobstore now serves from /home/agent_test_user/blobstore, but the helper does not ensure this directory exists (or is owned by agent_test_user) before starting fake-blobstore. That makes the suite more brittle when run outside the CI task/docs steps (and can fail if the dir was removed during debugging).
func (t *TestEnvironment) StartBlobstore() error {
_, ignoredErr := t.RunCommand("sudo killall -9 fake-blobstore")
if ignoredErr != nil {
t.writerPrinter.Printf("StartBlobstore: %s", ignoredErr)
}
_, err :=
t.RunCommand(fmt.Sprintf("nohup /home/agent_test_user/fake-blobstore -host 127.0.0.1 -port 9091 -assets %s &> /dev/null &", blobstoreDir))
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
integration/test_environment.go (1)
653-662: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate the detected service manager before storing it.
t.serviceManageris now interpolated into batch scripts asSM=%s\n(Lines 191, 238, 258). If the remote command output contains anything besides the single token (for example shell or sudo noise), the value flows into the script verbatim. A newline inside the value injects extra lines into the batch script, and any other noise makes every[ "$SM" = systemd ]check fall to the non-systemd branch silently.Accept only the two known values.
🛡️ Proposed fix
- t.serviceManager = strings.TrimSpace(out) + sm := strings.TrimSpace(out) + if sm != SERVICE_MANAGER_SYSTEMD && sm != "sv" { + return fmt.Errorf("unexpected service manager detected: %q", out) + } + t.serviceManager = sm🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration/test_environment.go` around lines 653 - 662, Update DetectServiceManager to accept only the known service-manager tokens, SERVICE_MANAGER_SYSTEMD or “sv”, after trimming command output; return an error for any other value and assign t.serviceManager only after validation.integration/nats_firewall_test.go (1)
64-71: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTolerate a missing nftables chain in the hooks.
These hooks assert that
sudo nft flush chain inet bosh_agent nats_accesssucceeds. The chain exists only after the agent has programmed the firewall. If a spec fails before that point, or the agent never creates the chain,nftexits non-zero and theAfterEachfails. The teardown failure then replaces the original spec failure in the report. TheBeforeEachat Line 51 already uses the tolerant form for the same command, so the hooks are inconsistent.Use the tolerant form in the hooks.
🛡️ Proposed fix
- _, err = testEnvironment.RunCommand("sudo nft flush chain inet bosh_agent nats_access") - Expect(err).To(BeNil()) + _, _ = testEnvironment.RunCommand("sudo nft flush chain inet bosh_agent nats_access") //nolint:errcheckAlso applies to: 152-155, 183-187, 209-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration/nats_firewall_test.go` around lines 64 - 71, Update the nftables cleanup commands in the AfterEach hooks and the other teardown locations to use the same tolerant form as the BeforeEach command, allowing a missing bosh_agent/nats_access chain without failing teardown. Preserve the existing cleanup and assertions for the other commands.ci/tasks/test-integration.sh (1)
99-99: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse the generated SSH config for release uploads.
scpandssh_commandboth target${agent_ip}without-F ~/.ssh/config, so upload attempts bypass theProxyJump ${JUMPBOX_IP}entry in that file. Feed the same SSH configuration intoscpandSSHfor these operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/tasks/test-integration.sh` at line 99, Update the release upload commands that invoke scp and ssh_command with ${agent_ip} to explicitly use the generated ~/.ssh/config via the SSH configuration option, ensuring both operations apply the ProxyJump jumpbox entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration/scripts/cleanup_data_dir.sh`:
- Around line 19-28: Increase the retry-attempt count in the monit-stop polling
loop while keeping the existing one-second sleep and immediate success break
unchanged. Preserve the current stopped-state check and failure handling, but
provide a substantially larger maximum wait for slow VMs.
---
Outside diff comments:
In `@ci/tasks/test-integration.sh`:
- Line 99: Update the release upload commands that invoke scp and ssh_command
with ${agent_ip} to explicitly use the generated ~/.ssh/config via the SSH
configuration option, ensuring both operations apply the ProxyJump jumpbox
entry.
In `@integration/nats_firewall_test.go`:
- Around line 64-71: Update the nftables cleanup commands in the AfterEach hooks
and the other teardown locations to use the same tolerant form as the BeforeEach
command, allowing a missing bosh_agent/nats_access chain without failing
teardown. Preserve the existing cleanup and assertions for the other commands.
In `@integration/test_environment.go`:
- Around line 653-662: Update DetectServiceManager to accept only the known
service-manager tokens, SERVICE_MANAGER_SYSTEMD or “sv”, after trimming command
output; return an error for any other value and assign t.serviceManager only
after validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f320a4a4-8cf1-4f1d-a577-ea220d8d65f8
📒 Files selected for processing (19)
bin/lintci/tasks/test-integration-windows.shci/tasks/test-integration.shdocs/running_bosh_agent_integration_tests_from_local_machine.mdintegration/apply_test.gointegration/compile_package_test.gointegration/fetch_logs_with_signed_url_test.gointegration/integration_suite_test.gointegration/nats_firewall_test.gointegration/prepare_test.gointegration/scripts/attach_device.shintegration/scripts/cleanup_data_dir.shintegration/scripts/cleanup_log_file.shintegration/scripts/detach_device.shintegration/scripts/detach_mount.shintegration/scripts/reset_device_map.shintegration/sync_dns_test.gointegration/test_environment.gointegration/v1_apply_test.go
💤 Files with no reviewable changes (3)
- integration/sync_dns_test.go
- integration/fetch_logs_with_signed_url_test.go
- integration/prepare_test.go
After adding copies of the agent config that used systemd, it seemed execessive to have 6 json config files used by tests. Instead, we can just define a default config in code, let the test harnesss determine if sysetmd is being used, and edit the agent config as needed per test. This cleans up the tests to make it clearer what is different about the config being tested.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (3)
integration/test_environment.go:535
- CreateAgentConfigFile writes a generated JSON file into integration/assets (agent-config.json). This leaves an untracked file behind in working copies and also couples test execution to a writable source tree path. It’s safer to write the generated config to a temp file and delete it after streaming it to the VM.
integration/test_environment.go:687 - WaitForAgent still logs "via ssh tunnel", but StartAgentTunnel/StopAgentTunnel were removed and the agent client now connects by dialing through the persistent SSH client. Updating the log message will avoid confusion when debugging failures.
infrastructure/settings_source_factory.go:173 - The PR description says "No production agent code changes", but this adds a MarshalJSON implementation in the production infrastructure package. Because this can change how configs/settings are serialized anywhere SourceOptionsSlice is marshaled, please either (a) update the PR description to reflect the production-code change, or (b) constrain the behavior to the integration harness (e.g., avoid relying on json.Marshal for SourceOptionsSlice in integration config generation).
// MarshalJSON is the inverse of UnmarshalJSON: it re-injects the "Type" discriminator that the
// concrete SourceOptions structs (FileSourceOptions, HTTPSourceOptions, ...) don't carry as a field,
// so a marshaled slice round-trips back through UnmarshalJSON. Without it, json.Marshal emits source
// objects with no "Type" and reloading fails with "Missing source type".
func (s SourceOptionsSlice) MarshalJSON() ([]byte, error) {
Alphasite
reviewed
Aug 4, 2026
Alphasite
left a comment
Alphasite
left a comment
Contributor
There was a problem hiding this comment.
didnt have time to finish the review but left some comments
| // concrete SourceOptions structs (FileSourceOptions, HTTPSourceOptions, ...) don't carry as a field, | ||
| // so a marshaled slice round-trips back through UnmarshalJSON. Without it, json.Marshal emits source | ||
| // objects with no "Type" and reloading fails with "Missing source type". | ||
| func (s SourceOptionsSlice) MarshalJSON() ([]byte, error) { |
Contributor
There was a problem hiding this comment.
nice! very rare to see someone making good use of this feature
| for wantType, opts := range cases { | ||
| data, err := json.Marshal(SourceOptionsSlice{opts}) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(string(data)).To(ContainSubstring(`"Type":"` + wantType + `"`)) |
Contributor
There was a problem hiding this comment.
this is fine, but iirc gomega has a nice json matcher for this exact use case which is even better
Contributor
There was a problem hiding this comment.
cool, I didn't know about MatchJSON, if that is what you were referring to.
Based on reading how MatchJSON works here, I'm inclined to say we just keep this explicit check for the type field though.
I also think MatchJSON doesn't do partial matching, so we'd need to write out a full string each time, something like: {"Headers":null,"InstanceIDPath":"","SSHKeysPath":"","TokenPath":"","Type":"HTTP","URI":"http://example.com","UserDataPath":""}
|
|
||
| // ServiceManager is intentionally left zero here; CreateAgentConfigFile stamps it per target | ||
| // (sv vs systemd) so this value stays service-manager-agnostic. | ||
| var DefaultAgentConfig = app.Config{ |
Contributor
There was a problem hiding this comment.
i cant remember the rules here but can you make this const?
Contributor
There was a problem hiding this comment.
no I don't think you can make structs consts
|
|
||
| // StartAgentTunnel also acts as a wait condition for the agent to have fully started. Copying over the blobs before it fully starts, will result in issues because the agent cleans up dirs on start. | ||
| err := testEnvironment.StartAgentTunnel() | ||
| // WaitForAgent blocks until the agent is reachable. Copying over the blobs before it fully starts will result in issues because the agent cleans up dirs on start. |
Comment on lines
+159
to
+162
| Eventually(func() string { | ||
| out, _ := testEnvironment.RunCommand("sudo stat " + dir) //nolint:errcheck | ||
| return out | ||
| }, 2*time.Minute, 1*time.Second).Should(MatchRegexp("Access: \\(0770/drwxrwx---\\) Uid: \\( 0/ root\\) Gid: \\( 100[0-9]/ vcap\\)")) |
Contributor
There was a problem hiding this comment.
The looooong regex just makes the assertion hard to read (heres my possibly bad suggestion)
Suggested change
| Eventually(func() string { | |
| out, _ := testEnvironment.RunCommand("sudo stat " + dir) //nolint:errcheck | |
| return out | |
| }, 2*time.Minute, 1*time.Second).Should(MatchRegexp("Access: \\(0770/drwxrwx---\\) Uid: \\( 0/ root\\) Gid: \\( 100[0-9]/ vcap\\)")) | |
| Eventually( | |
| func() string { | |
| out, _ := testEnvironment.RunCommand("sudo stat " + dir) //nolint:errcheck | |
| return out | |
| }, | |
| 2*time.Minute, | |
| 1*time.Second, | |
| ).Should( | |
| MatchRegexp("Access: \\(0770/drwxrwx---\\) Uid: \\( 0/ root\\) Gid: \\( 100[0-9]/ vcap\\)"), | |
| ) |
or something like that (not sure what the patch its going to suggest looks like exactly.
| _, err := testEnvironment.RunCommand("sudo userdel -rf username || true") //nolint:errcheck | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| err = testEnvironment.DetachDevice("/dev/sdh") |
Contributor
There was a problem hiding this comment.
im missing some context but who calls the detach scripts now?
Member
Author
There was a problem hiding this comment.
they happen in a suite level AfterEach that cleans up everything
Contributor
There was a problem hiding this comment.
the cleanup got folded into a function called RestoreCleanBaseline() that gets called in the suite-level AfterEach (hopefully this link works)
| ) | ||
|
|
||
| BeforeEach(func() { | ||
| err := testEnvironment.CleanupDataDir() |
Contributor
There was a problem hiding this comment.
q: as above who calls these scripts now? WaitForAgent or something?
Member
Author
There was a problem hiding this comment.
there's an AfterEach in integration_suite_test.go which always calls RestoreCleanBaseline() which cleans up everything
Contributor
There was a problem hiding this comment.
the cleanup got folded into a function called RestoreCleanBaseline() that gets called in the suite-level AfterEach (hopefully this link works)
| Expect(output).To(ContainSubstring("ct state established,related accept")) | ||
| Expect(output).To(MatchRegexp(`meta skuid 0 ip daddr %s tcp dport 4222 accept`, directorIP)) | ||
| Expect(output).To(MatchRegexp(`ip daddr %s tcp dport 4222 drop`, directorIP)) | ||
| // Assert every rule against a single snapshot, polled until all conditions hold at once, to |
KauzClay
approved these changes
Aug 4, 2026
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this change about?
The agent tests were slow and brittle. @KauzClay and I spent a bunch of time adding Noble and Resolute builds of them, and in the process saw a bunch of stuff we wanted to improve.
These changes clean up the setup and teardown, and optimize the runtime:
(all times include the fixed ~4 minute cost of deploying a new VM)
This PR also includes documentation and changes to make it possible to run the agent suite from a workstation targeting a bosh deployed VM.
This PR is multiple commits. Reviewing by commit-by-commit may be easier. Please do not squash them if you're merging this PR.
What tests have you run against this PR?
These changes have gone green 4x in a row in a reproduction of the CI pipeline running builds for Jammy, Noble and Resolute. They've also gone green when run locally against a bosh deployed Resolute VM.
How should this change be described in bosh-agent release notes?
Does this PR introduce a breaking change?
No
Tag your pair, your PM, and/or team!
@aramprice @KauzClay
AI Slop Description of Changes
Summary
This PR overhauls integration-test setup/teardown to cut the number of SSH round-trips each test makes. A single spec previously issued ~148 SSH round-trips — every helper command opened a fresh SSH session through the jumpbox, so each mount, fuser, umount, losetup, etc. was a full network round-trip, and much of the cleanup ran twice per test. The suite now does far less redundant work and ships deterministic command sequences server-side, roughly halving wall-clock time on the full run.
No production agent code changes — this is entirely test-harness and CI-provisioning work.
What changed
Remove redundant per-test cleanup. The suite AfterEach already returns the VM to a clean baseline, but each file's BeforeEach repeated the same CleanupDataDir / CleanupLogFile / config restore. Setup/teardown now share a single RestoreCleanBaseline helper (used by both the first spec and every teardown, so the two paths can't drift), and per-file BeforeEach blocks keep only test-specific config. ResetDeviceMap becomes the single device-teardown path.
Batch deterministic command sequences into single SSH calls. CleanupDataDir, AttachDevice, ResetDeviceMap, DetachDevice, and CleanupLogFile each now fire one sudo bash -euo pipefail batch instead of N per-command round-trips, with the Go wrappers reduced to invoke-and-parse. The Noble loop-device semantics (partition auto-scan, autoclear-pending detaches, udevadm settle/retry) are preserved verbatim in the server-side script.
Hoist stable per-test setup and stream file uploads.
Fix flakes exposed by the faster suite. Tighter timing surfaced several latent races, all fixed: a root-device settle race in the ephemeral-disk spec (plus a defered restore so one failure can't cascade into a suite-wide timeout), and gate-then-separate-read TOCTOU patterns in the nats_firewall, apply, and v1_apply specs (assertions now poll a single snapshot).
Lint/format cleanup. Removed an unused type and quoted an optional scp flag (shellcheck SC2086).