Repository navigation
cleanup: Mount the previous system's /var before removing its packages - #45
cgwalters-bot wants to merge 1 commit into
Conversation
|
Verification run on a devspace (16 cores, Intel Xeon Platinum 8573C, RHEL 10, KVM, bcvk 0.19.0, tmt 1.79.0 with testcloud; host set up as in Result: the fix works on the case it was written for. Both paths pass, exit 0, and test 014 passes on the reinstall path. The cleanup unit mounts the previous What ran
The reinstalled guest (Fedora 44 cloud image, btrfs, booted image version
|
VERDICT: APPROVE (safe to merge as is; the items below can follow). This review is LLM-generated, by Claude Opus 5.5 ( Scope: Findings (ranked; none block the merge)
Claims in the PR description checked against the code
Security postureThe change runs only inside the existing root oneshot unit, after the install. It adds no new input surface beyond the previous OS's Checks run here (rustc 1.99.0)
Not checked
Generated-by: https://github.com/cgwalters/#llms |
After an alongside install, fedora-bootc-destructive-cleanup runs `rpm -e --root=/sysroot` on every package of the previous OS. rpm runs the erase scriptlets chrooted in /sysroot, so they need the previous OS's /var. But /sysroot is only the previous root filesystem; on Fedora 44 cloud images /var is a separate btrfs subvolume that nothing mounts after the reinstall. netcat's %preun (`alternatives --remove`) then fails with "failed to create admindir", xargs exits 123 and bootc-destructive-cleanup.service fails. So mount the previous /var at /sysroot/var from the previous system's fstab when its source is a block device. The unit has PrivateMounts=true so the mount stays private to the service. Anything that looks wrong (already a mount point, not a block device, the mount fails, or the result is the same filesystem as /sysroot or the booted /var) gets a warning and the old behaviour, since deleting from the wrong /var would be much worse than a failed unit. Each of those, and a skipped container prune, prints a line saying why, so the journal shows what was not cleaned up. Add a readonly test for the in-place reinstall path that checks the unit ran to completion and left no packages of the previous system: "no failed units" alone also passes when the cleanup never ran. The alternative is `rpm -e --noscripts`, which is smaller but skips every scriptlet on every system and leaves the old /var unreachable for the podman prune. bootc-dev#2559 Generated-by: AI Agent-run: https://github.com/bootc-dev/cgwalters-devspace-sandbox/actions/runs/38040104249/attempts/1
ce3cd6d to
47b1a70
Compare
|
Addressed review findings 1, 2 and 4 from the review above, squashed into the one commit. New head: 47b1a70 (was ce3cd6d); still one commit on top of step 2. LLM-written and LLM-run; nothing here was checked by a human. What changedFinding 1 (silent skips). Finding 4 (unresolvable source). An fstab entry whose source Finding 2 (tests check intent). New The man page says that these lines are printed and where to find them, and the commit message has two more sentences for the warnings and the test. Finding 3 (the old fstab's mount options, e.g. VerificationOn a devspace (16 cores, AMD EPYC 9V45, RHEL 10,
The new test, from each guest's bcvk path: Test 014 passed on both paths too. Not runThe new warning lines themselves: the run only took the happy path (a btrfs Generated-by: https://github.com/cgwalters/#llms |
Step 3 of the plan in bootc-dev#2559, stacked on #44 (which is stacked on #43): the fix for
bootc-destructive-cleanup.servicefailing after an in-place reinstall.Root cause
fedora-bootc-destructive-cleanuprunsrpm -e --root=/sysrooton every package of the previous OS. rpm runs the erase scriptlets chrooted in/sysroot, so they need the previous OS's/var. But/sysrootis only the previous root filesystem (btrfssubvol=/root); on Fedora 44 cloud images/varis a separatevarsubvolume that nothing mounts after the reinstall. netcat's%preun(alternatives --remove) then fails withfailed to create admindir,xargsexits 123 and the unit fails.The script is unchanged since f567f7e (January 2026), so this is not a regression. It went unseen because nothing checked the unit until test 014 was added in 41049cc (2026-09-18), and the bcvk flows never boot a reinstalled system.
The fix
In
contrib/scripts/fedora-bootc-destructive-cleanup(plus its man page): if/sysroot/etc/fstabhas a/varentry whose source resolves to a block device, mount it at/sysroot/varwithmount --fstab /sysroot/etc/fstab --target-prefix /sysroot /varbefore removing the packages. The unit already hasPrivateMounts=true, so the mount stays private to the service.The mount is skipped, and the script behaves as before, when:
/sysroot/etc/fstabdoes not exist or has no/varentry;/sysroot/varis already a mount point;/varturns out to be on the same filesystem as/sysrootor as the booted/var: it is unmounted again (warning).The podman prune now runs only if
/sysroot/var/lib/containersexists.Not handled: nested mounts under
/var, a/vardefined by a.mountunit, and LVM or LUKS devices that are not active in the new system. Those give a warning and then the old behaviour. The script only uses/sysrootand/var, so it is the same for ostree and composefs; but on composefs--cleanupnever writes the stamp file (onlyostree_installdoes), so the service never runs there. That is a separate gap and is not fixed here.Alternatives the run listed
The run called this "not a clear-cut choice":
rpm -e --noscripts,bootc-dev/bootc#2561. Smaller, but it skips every scriptlet on every system (cgwalters called it a big hammer on that PR), and the old/varstays unreachable for the podman prune.It chose the targeted mount because it matches cgwalters' stated preference ("special case mount preservation").
Questions for the maintainer (from the run)
rpm -e --noscripts(bootc-dev/bootc#2561), which is smaller but skips all scriptlets. I recommend keeping the mount and closing that PR or rebasing it onto this; a maintainer should decide.just test-tmt-readonly. Test 014 must now pass on the reinstall path, and the cleanup unit's journal should showmount --fstab /sysroot/etc/fstab --target-prefix /sysroot /varsucceeding.install to-existing-root --cleanupwith the composefs backend write the cleanup stamp? Today it is silently ignored. I left it alone as a separate change.bootloader-update.servicefailure after the reinstall on a BIOS-booted guest: file a separate issue (bootc removesEFI/whilebootupd --autoinstalls only BIOS)? I recommend yes, after confirming the guest's boot mode. (The run inferred this cause; it did not verify it.)What the run did and did not run
Ran:
shellcheckandbash -non the script;cargo xtask update-generated direct --check; and the new function in an unprivileged user and mount namespace (unshare -rm) against a scratch fstab with/sysrootand/varrelocated, 8 scenarios (a tmpfs/varentry mounts; no/varentry, no fstab, an already-mounted/sysroot/var, a non-block source and a bad mount option are each skipped; a/varon the same filesystem as the old root or as the live/varis unmounted again). The block-device check was stubbed for the cases that need a mount.Not run: a real boot, a btrfs subvolume mount,
just test-tmt-readonly, and the fulljust validatecontainer build. There are no unit tests because the logic is bash. So as opened, this change has never executed on a reinstalled system; a devspace verification will follow as a comment.Run: https://github.com/bootc-dev/cgwalters-devspace-sandbox/actions/runs/38040104249
Base PR: #44 (on #43)
Generated-by: https://github.com/cgwalters/#llms