Repository navigation
install/migrate: Fix --preserve-var failures after the install - #11
jmarrero-bot wants to merge 6 commits into
Conversation
92b7b95 to
537cde5
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head 537cde5d11: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.
Three fixes for --preserve-var (bootc-dev#2586) failing after /boot is wiped: the mountinfo parser now reads only the mountpoint field, a nested-mount placeholder may already hold image-seeded /var content, and the source /var is opened, its nested mounts listed and type-checked before the install starts (VarMigrationPlan). The parser and placeholder changes are small and look correct. The remaining risk is what the preflight cannot cover: the copy itself still runs after the wipe, and on a non-reflink filesystem it is a full second copy of /var on the same disk with no space check, so ENOSPC is now the most likely post-wipe failure. The code comment at install.rs:3201 overstates what is checked.
Hotspots
- look-closely · error-handling —
crates/lib/src/install.rs:3201-3214(537cde5): The comment says everything about the source /var that can fail is checked here, but the copy (ENOSPC on a full non-reflink copy, unreadable subdirectories) still fails after /boot is wiped. A free-space check when reflinks are unsupported would close the most likely gap. - note · test-gap —
crates/lib/src/install/migrate.rs:105-147(537cde5): The new per-mountpoint type check has no unit test, and it also checks mounts nested under another nested mount, which the copy never reaches. - note · logic —
crates/lib/src/install/migrate.rs:697-725(1e5c158): Accepting a non-empty destination is safe: the mount covers it at boot, and only the type is checked. Without --merge-etc the host fstab is not carried, so the image content shows instead of the nested filesystem, as the empty placeholder did before. - note · logic —
crates/lib/src/install/migrate/mountinfo.rs:73-124(50b2ec5): Splitting on single spaces is right because the five fields before the mountpoint are never empty. Decoding any three-digit octal escape is right too, since the kernel escapes a literal backslash as \134.
Safe to skim
docs/src/man/bootc-install-to-existing-root.8.md: One sentence on what a copy failure leaves behindcrates/lib/src/install/migrate/mountinfo.rs:206-239: Table-style parser tests
537cde5 to
fa61148
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head fa61148bf8: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.
Four fixes for --preserve-var (bootc-dev#2586) failing after /boot is wiped. The mountinfo parser now reads only the mountpoint field. A nested-mount placeholder may already hold image-seeded /var content. The source /var and its reachable nested mountpoints are opened and type-checked before the install starts (VarMigrationPlan). Restoring xattrs no longer removes the destination's security.selinux when the source has none, which a CentOS Stream 10 VM hit for an empty-source tmpfs below /var. The code looks correct. Running out of space during the copy after the wipe is acknowledged in the comment and left open. In the last commit the new const sits between apply_xattrs_exact and its doc comment, and the doc's 'exactly match' is no longer true.
Hotspots
- look-closely · logic —
crates/lib/src/install/migrate.rs:891-915(fa61148): Keeping the destination's label when the source has none is the right call. But the const was inserted between apply_xattrs_exact and its doc comment, which now documents the const and still says 'exactly match'. There is no unit test, so the change rests on the VM run. - note · error-handling —
crates/lib/src/install.rs:3201-3215(6430bbf): The copy can still fail after the wipe, including running out of space without reflinks. That is now stated, and listed as open rather than fixed. - note · test-gap —
crates/lib/src/install/migrate.rs:105-160(6430bbf): The type check now skips mounts below exclusions and below other nested mounts. It still has no unit test. - note · logic —
crates/lib/src/install/migrate/mountinfo.rs:73-124(50b2ec5): Splitting on single spaces is correct because the five fields before the mountpoint are never empty. Decoding any octal escape is safe because the kernel writes a literal backslash as \134.
Safe to skim
docs/src/man/bootc-install-to-existing-root.8.md: One sentence on what a copy failure leavescrates/lib/src/install/migrate/mountinfo.rs:206-239: Parser tests
fa61148 to
d00bf4f
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head d00bf4fd22: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.
Four fixes for --preserve-var (bootc-dev#2586) failing after /boot is wiped. The mountinfo parser now reads only the mountpoint field. A nested-mount placeholder may already hold image-seeded /var content. The source /var and its reachable nested mountpoints are opened and type-checked before the install starts (VarMigrationPlan). Restoring xattrs keeps the destination's SELinux label when the source has none (CentOS Stream 10 tmpfs case). The reviewer found no remaining issues in the code. Still open by design: the copy can fail after the wipe (e.g. running out of space without reflinks). With --preserve-var alone on a cloud-init host, sshd has no host keys after reboot (see the PR body). tmt results are still pending in the PR body.
Hotspots
- note · error-handling —
crates/lib/src/install.rs:3201-3215(6430bbf): Only the preflight moved before the wipe. The copy itself can still fail afterwards, as the comment now says. - note · logic —
crates/lib/src/install/migrate.rs:892-917(d00bf4f): This is the one deliberate exception to restoring xattrs exactly: a missing source label keeps the destination's. It is covered by the CentOS Stream 10 VM run, not by a unit test. - note · logic —
crates/lib/src/install/migrate/mountinfo.rs:73-124(50b2ec5): Splitting on single spaces is correct because the five fields before the mountpoint are never empty. Decoding any octal escape is safe because the kernel writes a literal backslash as \134.
Safe to skim
docs/src/man/bootc-install-to-existing-root.8.md: One sentence on what a copy failure leavescrates/lib/src/install/migrate/mountinfo.rs:206-239: Parser tests
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head db14a38beb: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.
Fixes for --preserve-var (bootc-dev#2586) failing after /boot is wiped. The mountinfo parser now reads only the mountpoint field. A nested-mount placeholder may already hold image-seeded /var content. The source /var and its reachable nested mountpoints are opened and type-checked before the install starts. Restoring xattrs keeps the destination's SELinux label when the source has none (CentOS Stream 10 tmpfs case). Two tmt commits: the plan pulls the bound images the image itself names, since main now pins ubi9/podman, and the preserve-var test mounts the new deployment under /run, because /mnt is a dangling symlink in fedora-bootc. The reviewer found no remaining code issues. Still open by design: the copy can fail after the wipe (e.g. running out of space without reflinks), and --preserve-var alone on a cloud-init host leaves sshd without host keys (see the PR body).
Hotspots
- note · error-handling —
crates/lib/src/install.rs:3201-3215(6430bbf): Only the preflight moved before the wipe. The copy itself can still fail afterwards, as the comment says. - note · logic —
crates/lib/src/install/migrate.rs:889-917(d00bf4f): This is the one deliberate exception to restoring xattrs exactly: a missing source label keeps the destination's. It is covered by the CentOS Stream 10 VM run, not by a unit test. - note · test-gap —
tmt/plans/package-mode.fmf:126-136(cbde03a): The bound-image references are read from the image's bound-images.d, so the plan follows whatever hack/lbi pins. It relies on Image= lines being unquoted.
Safe to skim
docs/src/man/bootc-install-to-existing-root.8.md: One sentence on what a copy failure leavescrates/lib/src/install/migrate/mountinfo.rs:206-239: Parser teststmt/tests/booted/package-mode/test-to-existing-root-preserve-var.sh: Mount path /mnt to /run
The parser validated every field of every record in the installer's mount namespace and failed the migration on any it didn't accept, and this runs after the install has finished. A tmpfs mounted with an empty source, which the kernel prints with two consecutive spaces, was enough (`Malformed mountinfo line N: invalid field count`), as were escapes other than the four the kernel uses for paths, anywhere on the system. Only the mountpoint, the fifth space-separated field, is needed, and the fields before it are never empty. Unknown escapes are kept as they are rather than rejected. Generated-by: AI
The first deploy seeds the new stateroot's /var from the image (ostree 2024.3 and later), so a mountpoint placeholder's destination can already exist with content, and that failed the migration after the install with "Mountpoint placeholder log is not empty". A fedora-bootc image that only ran `dnf install` ships /var/log/dnf5.log, so any host with /var/log on its own filesystem, a common hardened layout, hit it. The mount covers whatever is there at boot, so an existing entry of the right type is kept as the mountpoint. Only one of a different type is still an error. Generated-by: AI
The /var migration runs once /boot has been wiped and bootupd has installed the new bootloader, so anything that fails there leaves a host whose only boot entry is the new deployment with a partial /var. Opening the source /var, working out its nested mounts and checking that each can get a placeholder (only directories and files can) don't depend on the install, so do them first, when a failure still leaves the host as it was, and copy from that plan afterwards. The copy itself can't move earlier, since its destination only exists once the image is deployed; the man page now says what a failure there leaves behind. Generated-by: AI
Restoring a split directory's or placeholder's xattrs removes every destination xattr the source lacks. On CentOS Stream 10 the root of a tmpfs mounted below /var lists no security.selinux, so the placeholder for it lost its label, which SELinux refuses: the migration failed with "Removing destination xattr security.selinux: Permission denied" after the install, leaving a partial /var as the only system to boot. A missing source label now leaves the label the policy gave the new directory. Generated-by: AI
The plan pulled ubi9/podman:latest, but main now pins that bound image (hack/lbi/podman.image), so on a branch based on current main the install failed before touching the host: "resolving bound image registry.access.redhat.com/ubi9/podman:9.8-1793032728 ... does not resolve to an image ID". Reading the references from the image keeps the plan in step with whatever hack/lbi says. Generated-by: AI
In fedora-bootc images /mnt is a symlink to var/mnt, which doesn't exist in the installer container, so `mkdir -p /mnt/...` failed with "cannot create directory '/mnt': File exists" right after a successful migration, failing the test on a Fedora guest. /run always exists. Generated-by: AI
bb305fc to
86a65e0
Compare
db14a38 to
9992228
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head 9992228d36: where to look closely and what is safe to skim. It is advice from the bot's reviewer and doesn't replace reading the diff; the review app walks it.
Six fixes on top of ckyrouac's bootc-dev#2586 head 86a65e0 (the base branch), clean rebase from bb305fc with identical content (range-diff all '='). Four code fixes for --preserve-var failures that happened after /boot was already replaced: a lenient mountinfo parser (only the mountpoint field), keeping image-seeded content at nested mountpoints, a preflight of the source /var before install, and keeping the destination SELinux label when the source has none. Two tmt fixes: pull the bound images the image names, and mount the deployment under /run. Devspace VM results back it: F3/C3 pass where 86a65e0 failed (F1/C1/F2/C2). Risk is mostly in the preflight (the nested-mount list is now taken minutes before the copy) and in the xattr exception, which applies to every copied object, not only placeholders. Not fixed here: the test's printf quoting inside bash -c (Copilot's L313 on bootc-dev#2586) still writes a broken tmpfiles rule, visible as printf %sn in the run-003/004 traces, so the local-layout post-reboot check can't pass even once SSH works.
Hotspots
- look-closely · logic —
crates/lib/src/install.rs:3198-3217(637e8d1): The nested-mount list now comes from a mountinfo snapshot taken before the whole install (minutes earlier) instead of just before the copy. A mount added under /var in between isn't in the plan, and cp -a would copy through it. That's probably acceptable, but it widens the race the module docs describe. - note · error-handling —
crates/lib/src/install/migrate.rs:105-150(637e8d1): The preflight only checks that the source opens and that each reachable mountpoint is a dir or file. The F2/C2 failures (destination content, SELinux xattr) depend on the destination and are fixed separately, so this catches less than the commit subject suggests. - look-closely · logic —
crates/lib/src/install/migrate.rs:702-730(dc9eaf9): Image content at a mountpoint is kept on the assumption that the mount hides it at boot. Without /etc migration the host's fstab line for e.g. /var/log is gone, so the mount doesn't happen and the image's content is what the new system sees. That's probably fine, but it's worth a doc line. - look-closely · security —
crates/lib/src/install/migrate.rs:908-917(d50acf9): apply_xattrs_exact is shared by every copied file and directory, not just placeholders. Any source object without security.selinux now keeps the policy default label instead of failing. That's likely the right call, but the commit subject says placeholders only. - note · logic —
crates/lib/src/install/migrate/mountinfo.rs:73-124(4ff52e2): Splitting on single spaces and taking field 5 relies on the kernel escaping spaces in the root field. It does, so this is fine. Unknown escapes are now kept verbatim rather than rejected, and a mountpoint that is not under /var is ignored either way. - risky · test-gap —
tmt/tests/booted/package-mode/test-to-existing-root-preserve-var.sh:286-313(9992228): Inside the single-quoted bash -c script, printf '%s\n' on L312-313 closes the quote, so the guest runsprintf %sn(see the run-003/004 xtrace). The tmpfiles rule ends up as one line with literal n's, and the symlink is never recreated after reboot. Fix it here with double quotes.
Safe to skim
crates/lib/src/install/migrate/mountinfo.rs:206-239: unit testscrates/lib/src/install/migrate.rs:1142-1167: unit testcrates/lib/src/install/migrate.rs:1611-1661: unit test updated for kept contentcrates/lib/src/install.rs:2825-2832: mechanical extraction into mount_alongside_root_if_neededcrates/lib/src/install.rs:3090-3112: moved code, unchangeddocs/src/man/bootc-install-to-existing-root.8.md: one doc sentence
Fixes for bootc-dev#2586 (
--preserve-var), on top of ckyrouac'spreserve-varhead 86a65e0, which is pushed unchanged here as the baseckyrouac/preserve-var-base. (Rebased from bb305fc: 86a65e0 is that head rebased onto newer main with no changes to its commits, and none of these fixes are in it; the six commits rebased without conflicts and are unchanged.) Meant to be submitted to ckyrouac'spreserve-varbranch once jmarrero has reviewed it; nothing here is for bootc-dev/bootc directly. The--merge-etcfixes for bootc-dev#2384 are #12.Each of these failed the migration after
Installation complete!, when the host's /boot was already wiped:invalid field count)./varfrom the image, so a separate/var/logplus an image that randnffailed withMountpoint placeholder log is not empty(Fedora 44 VM at 86a65e0).ubi9/podman:latest, but main now pins it, so the install failed before touching the host (... 9.8-1793032728 does not resolve to an image ID); and the test's post-install step didmkdir -p /mnt/...in the installer container, where fedora-bootc's/mntis a dangling symlink.Removing destination xattr security.selinux: Permission denied.Testing at 9992228 (this head), on a 16-core devspace (jmarrero-devspace-42889470701), next to ckyrouac's 86a65e0:
cargo test -p bootc-lib --lib: 378 passed (376 at 86a65e0);cargo fmt --checkok. The Makefile's clippy gate on-p bootc-lib --no-depsreports onlydeploy.rs:1820(str::len), untouched here and also hit at 86a65e0: the devspace's clippy 1.97 is newer than CI's./varsubvolume) and CentOS Stream 10 GenericCloud (XFS) package-mode hosts with/var/logon its own XFS disk, an empty-source tmpfs under/var(plusrpc_pipefson CS10), PostgreSQL data, a service with state in/var/liband a podman volume, migrated with the documentedpodman run ... bootc install to-existing-root --preserve-varto images built from this branch withjust build(fedora-bootc:44 and centos-bootc:stream10 bases) pluspostgresql-server. Both installs succeed (reflink strategy, placeholders for/var/log, the tmpfs andrpc_pipefs), and after reboot PostgreSQL starts with its data and the app state is there. At 86a65e0 the same hosts fail afterInstallation complete!withMalformed mountinfo line 42: invalid field count(F44) /line 36(CS10); with a named tmpfs instead,Mountpoint placeholder log is not empty(F44) andRemoving destination xattr security.selinux: Permission denied(CS10). A host with nothing mounted under /var works at 86a65e0 too.sshdfails and the host is unreachable over SSH. Both images ship cloud-init (sosshd-keygen@stands down), the host's/var/lib/cloudcomes along so cloud-init sees the same instance, and the keys stayed in the old/etc. In the 86a65e0 run where nothing got copied, cloud-init generated keys. In the earlier round,--merge-etc(install: Add --preserve-var and --merge-etc to to-existing-root bootc-dev/bootc#2384) carried the keys. The package-mode Testing Farm jobs on 86a65e0 stop the same way (Failed to start sshd.serviceafter the reboot). Not addressed here.sshd.servicefails on the rebooted guest (host keys, above). On a CS10 guest the migration and pre-reboot checks pass, then tmt can't pull its workdir before rebooting (opendir "/var/tmp/tmt/run-003/tmt/plans/package-mode/data" failed: Too many levels of symbolic links). That comes from the test swapping/var/tmp/tmt/run-NNNfor a symlink, which happened at 86a65e0 too. Also, the test's L313printf '%s\n'sits inside a single-quotedbash -c, so it runs asprintf %sn(in the traces of both runs) and writes the tmpfiles rule as one line; Copilot flagged it on install: Preserve /var when installing to existing root bootc-dev/bootc#2586. That's ckyrouac's code and isn't fixed here. So the plan can't pass on a cloud-init guest yet.Open points for ckyrouac and cgwalters, not built here:
--preserve-varalone on a cloud-init host leaves no SSH host keys (above).tmp,cache,log/journal,lib/containers) can't be re-included, andlib/rpmisn't one of them./rootisn't migrated (it becomes/var/roothome)./varcontent is still skipped silently.Generated-by: https://github.com/jmarrero-bot#llms
Review draft in jmarrero-forge, not upstream yet. This section is removed when the PR is opened upstream.
ckyrouac/bootc, basepreserve-var(install: Preserve /var when installing to existing root bootc-dev/bootc#2586); not opened by the bot:bot-pr promotehas no policy record for ckyrouac/bootc and refusesPVTI_lADOFBIZ7c4BlZPrzhGhccsTo review:
Bot run: session 77cad178, agent a99f528d · claude-sonnet-5-5 4.9M in (97% cached) / 34k out · ~$1.17 inference (est., list prices) · 11m wall