Skip to content

fix: deflake //rs/tests/consensus/upgrade:upgrade_downgrade_old_nns_subnet_test_head_nns_colocate - #10569

Closed
basvandijk wants to merge 3 commits into
masterfrom
ai/deflake-upgrade_downgrade_old_nns_subnet_test-2026-06-25
Closed

fix: deflake //rs/tests/consensus/upgrade:upgrade_downgrade_old_nns_subnet_test_head_nns_colocate#10569
basvandijk wants to merge 3 commits into
masterfrom
ai/deflake-upgrade_downgrade_old_nns_subnet_test-2026-06-25

Conversation

@basvandijk

@basvandijk basvandijk commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Deflakes the upgrade system tests that assert the orchestrator logs
Orchestrator shut down gracefully before an upgrade reboot (read back from the
previous boot via journalctl --boot=-1).

The orchestrator was not at fault — it logs the message every time. The
message was being lost from the persistent journal before the test could
read it.

Root cause (confirmed from flaky-run logs)

GuestOS uses A/B slots with separate encrypted /var partitions, and the
target /var is wiped on upgrade. The previous boot's journal is carried across
the reboot by transfer_log_state() in
ic-os/components/guestos/init/setup-encryption/setup-var-encryption.sh, which
copies the newest .journal files from the old /var into the freshly-created
new /var on first boot.

That copy mounts /dev/mapper/var_crypt at /mnt/var_new, which races with
the fstab-generated var.mount unit
that mounts the same device at /var
(the device binding was loosened in #9984 via x-systemd.device-bound=false,
letting var.mount fire as soon as var_crypt appears — before the script
reaches its copy step).

When var.mount wins, the script's mount fails:

Successfully mounted old /var partition
mount: /mnt/var_new: /dev/mapper/var_crypt already mounted or mount point busy.

Because the script runs under set -e, it aborts before transfer_log_state
runs
, so the previous boot's journal — including the graceful-shutdown line —
is never preserved. journalctl --boot=-1 then can't find the message and the
test fails.

This was confirmed across all 4 recent flaky runs: the failing node always
logged the already mounted or mount point busy error, while passing
boots copied system.journal successfully.

Fix

Make the journal transfer resilient to the race instead of aborting:

  • transfer_log_state now takes the destination /var mountpoint as an argument.
  • The caller tries to mount var_crypt at /mnt/var_new; if that fails because
    var.mount already mounted it at /var, it copies the logs into /var
    instead. It only unmounts /mnt/var_new when it actually created that mount,
    and never aborts the boot on a failed mount.

This PR was created following the steps in .claude/skills/fix-flaky-tests/SKILL.md.

…ubnet_test

The upgrade system tests assert that, after an upgrade, the previous boot's
journal (`journalctl --boot=-1`) contains the orchestrator's
"Orchestrator shut down gracefully" message.

GuestOS uses A/B slots with separate encrypted /var partitions, and the target
/var is wiped on upgrade. The previous boot's journal is carried across the
reboot by `transfer_log_state()` in setup-var-encryption.sh, which copies the
newest journal files from the old /var into the freshly-created new /var.

That copy raced with the fstab-generated `var.mount` unit: both mount
/dev/mapper/var_crypt. When var.mount won, the script's
`mount /dev/mapper/var_crypt /mnt/var_new` failed with "already mounted or
mount point busy", and because the script runs under `set -e` it aborted
before `transfer_log_state` ran. The previous boot's journal (including the
graceful-shutdown line) was therefore not preserved, and `journalctl --boot=-1`
could not find it, failing the test. This was confirmed in the logs of all
recent flaky runs (the failing node always logged the "already mounted" error).

Make the journal transfer resilient to the race: try to mount var_crypt at
/mnt/var_new, and if it is already mounted at /var (var.mount won), copy the
logs there instead. Only unmount /mnt/var_new when we actually created that
mount, and never abort the boot on a failed mount.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR deflakes a GuestOS upgrade system test by making the “previous boot” journal transfer resilient to a race between the setup script and the fstab-generated var.mount unit, ensuring the prior boot’s journal (read via journalctl --boot=-1) is preserved across A/B-slot upgrades.

Changes:

  • Updates transfer_log_state to accept the destination /var mountpoint as an argument.
  • Makes mounting /dev/mapper/var_crypt at /mnt/var_new best-effort; if it’s already mounted at /var, copies logs there instead.
  • Only unmounts /mnt/var_new when this script mounted it.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if mount /dev/mapper/var_crypt /mnt/var_new; then
var_new=/mnt/var_new
var_new_mounted_by_us=1
elif awk '$2 == "/var" { found = 1 } END { exit !found }' /proc/mounts; then

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — addressed in 61ccd55. I now confirm the device too, but compare the canonical device via readlink -f rather than matching the literal /dev/mapper/var_crypt string: the flaky-run logs show the system refers to it as var_crypt (dm-1) and the dm number varies across boots, so /proc/mounts may spell the source as either /dev/mapper/var_crypt or /dev/dm-N. Comparing canonical paths handles both spellings and avoids silently skipping the transfer (which would re-introduce the flake).

basvandijk and others added 2 commits June 25, 2026 18:47
Address review feedback: the fallback that copies the previous boot's journal
into the already-mounted /var only checked that *something* was mounted at /var,
not that it was var_crypt. Verify the device too, comparing canonical device
paths via `readlink -f` since /proc/mounts may name it /dev/mapper/var_crypt or
/dev/dm-N.
@basvandijk
basvandijk marked this pull request as ready for review June 25, 2026 20:18
@basvandijk
basvandijk requested a review from a team as a code owner June 25, 2026 20:18
@github-actions github-actions Bot added the @node label Jun 25, 2026
@basvandijk

Copy link
Copy Markdown
Collaborator Author

Closed in favour of #10572.

@basvandijk basvandijk closed this Jun 26, 2026
@basvandijk
basvandijk deleted the ai/deflake-upgrade_downgrade_old_nns_subnet_test-2026-06-25 branch June 26, 2026 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants