Repository navigation
install: Add --var-mount-spec and DPS /var discovery (prototype on #2521) - #1
jmarrero-bot wants to merge 6 commits into
Conversation
|
@jmarrero questions on this prototype, for you or Colin:
|
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head 874c651f9d: 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.
Prototype of Colin's /var suggestions on top of bootc-dev#2521. Only the top three commits are new: a blockdev lookup for a DPS Variable Data Partition, --var-mount-spec (alias --var) plus config key and default discovery in install to-filesystem (bootc mounts /var, initializes it, unmounts it, records it in fstab or systemd.mount-extra), and moving the feature advertisement from container inspect to bootc --version. Risk is concentrated in the plan() decision table and in the mount/unmount lifecycle in var_mounts.rs and install_to_filesystem. Default discovery changes behavior for anyone with a DPS var partition and no flag. The composefs-with-UKI case cannot record the mount, so an explicit spec errors and discovery is skipped. The first three commits are the upstream PR's and were not changed. Reviewer re-run on a devspace: fmt, clippy, unit tests and doc build pass; plan-32 passes on ostree/grub (worker) and composefs systemd ext4 BLS, but fails on composefs systemd ext4 UKI unsealed because of the test script bug flagged below (the install behavior itself was as intended up to that point).
Hotspots
- look-closely · logic —
crates/lib/src/install/var_mounts.rs:55-103(59be652): Decision table for explicit, empty, caller-mounted, discovered and unrecordable cases. A non-empty existing DPS partition is silently adopted and recorded in fstab by default; only fstype is checked, not emptiness. - look-closely · error-handling —
crates/lib/src/install/var_mounts.rs:115-148(59be652): If a caller already mounted /var, the explicit spec is recorded without checking it matches what is mounted. Mountpoint creation and mount have separate flags; verify no leak on a failure between them. - look-closely · error-handling —
crates/lib/src/install/var_mounts.rs:152-177(59be652): Unmount via the umount binary plus remove_dir, repeated from Drop. Fails if something still holds the mount busy; Drop only warns, so an error path could leave /var mounted on the target. - look-closely · logic —
crates/lib/src/install.rs:2878-2923(59be652): Wires plan, mount and discover together. The host-root/alongside branch only rejects a spec from the CLI, not one from the config file, which is silently ignored there. Check that to-disk is intentionally not covered. - look-closely · logic —
crates/lib/src/install.rs:2262-2268(59be652): Unmount happens after sync and the final relabel but before finalize_filesystem; confirm nothing later needs /var and that the created mountpoint removal does not need SELinux labeling. - note · logic —
crates/lib/src/install.rs:1302-1323(59be652): fstab now holds /boot and /var lines. The var entry uses fstype auto and the PARTUUID or user-supplied source verbatim. - look-closely · logic —
crates/lib/src/bootc_composefs/boot.rs:761-783(59be652): Generalizes the /boot mount-extra karg to /var. Only the BLS path records it; UKI relies on plan() refusing, so any other composefs path that skips this loop would silently drop the mount. - note · logic —
crates/blockdev/src/blockdev.rs:236-244(ee7f0b7): Takes the first VAR partition across all backing roots; with several matches the choice is arbitrary. - look-closely · api —
crates/lib/src/cli.rs:975-989(874c651):bootc --versionis now multi-line. Parsers of its output (tmt bootc_testlib.nu uses parse "bootc {v}") should keep working on the first line; verify, since nu was not run. - note · api —
crates/lib/src/install/config.rs:118-122(59be652): Empty string is a meaningful value (disable everything); an empty config value overrides a non-empty one on merge, which the new test pins. - risky · test-gap —
tmt/tests/booted/test-install-to-filesystem-var-mount.sh:314-318(59be652): check_var_part_untouched mounts /var/mnt/varcheck, which only check_var_fs creates. On composefs UKI that function never runs, so the mount fails ('mount point does not exist') and plan-32 fails; reproduced on a devspace. Add mkdir -p.
Safe to skim
crates/lib/src/install/baseline.rs: One-line var: None field initializer
The test passed the EFI system partition's UUID as the boot mount spec, while /boot is a separate ext4 partition in its layout. Use the UUID of the boot partition, which is what the installed system needs to mount. This went unnoticed because the test does not boot the result. Generated-by: AI
A common setup places /var, or parts of it such as /var/log, on separate filesystems (for example LVM logical volumes) so that runaway writes cannot fill the root filesystem. With `install to-filesystem` these filesystems end up empty: the deployment backend seeds the image's /var into its own state directory (`ostree/deploy/<stateroot>/var`, or the composefs shared var), and at boot the separately mounted filesystems hide that content. On a fresh to-filesystem installation, discover filesystems mounted at the target's /var or below. Each empty top-level mount tree, including its nested mounts, is initialized from the seeded /var with `cp --archive`, then SELinux labeled and synced. A tree that already contains data is preserved as a whole, without merging. Alongside, host-root and existing-deployment installs are not affected, nor is `install to-disk`. Callers remain responsible for mounting these filesystems at boot. Note this changes behavior for callers that mount an empty /var filesystem and fill it themselves after installation: they now find the image's content there first. The seeded copy in the state directory is kept; on OSTree, emptying it would make the next deployment reseed it. Hardlinks cannot span filesystems, so trees with nested mounts are copied without preserving them. The roots of these filesystems are labeled unconditionally: without selinuxfs (e.g. in an osbuild buildroot), an inode with no label still reports the kernel's unlabeled context, which the regular relabel walk takes as already labeled. The separate /var TMT test now ships content in the image, adds a nested logical volume, and checks contents, ownership, symlinks and SELinux labels on the actual volumes. It no longer disables SELinux. Related: bootc-dev#1615 Related: bootc-dev#997 Generated-by: AI
Tools that prepare a target for `install to-filesystem`, such as disk image builders, need to know whether the bootc in an image initializes mounted /var filesystems before they mount them: older versions leave such filesystems empty, so they hide the image's /var content at boot, and versions before 1.12 reject mountpoints in the target entirely. Add an `install-features` list to `bootc container inspect` output, currently containing `initialize-var-mounts`. It describes the bootc binary rather than the image, but this is the command builders already run against the image. The human-readable output is unchanged. Generated-by: AI
The Discoverable Partitions Specification defines a type GUID for a partition holding /var. Let the installer find one on the disks backing the root, the same way it finds the ESP, so a later change can use it as the default /var without the caller passing anything. Prep for honoring DPS /var in install to-filesystem. Generated-by: AI
Having bootc mount the /var filesystem itself, like it does for the ESP, lets installers pass just a source instead of assembling the mount tree, and lets bootc record the mount for the installed system (fstab for ostree, systemd.mount-extra for composefs). Without the option, a Variable Data Partition on the root's disk is used by default, since DPS is how such layouts are meant to be described. An empty value turns that off and keeps the image's /var in the state directory, for installs that set up /var at first boot. --var is accepted as an alias. The mount is unmounted again once /var is initialized; the booted system does the mounting from then on. Generated-by: AI
Installer-visible capabilities describe the binary, not the image, and `--version` works wherever bootc does, including where there is no image to inspect. Drop install-features from `container inspect` again and list features after the version instead, keeping the first line the bare version for existing parsers. Generated-by: AI
874c651 to
d8cad10
Compare
|
Review guide for head Three bot commits on top of bootc-dev#2521: a blockdev lookup of the DPS Variable Data Partition, Look closely:
Notes: the default adopts a non-empty DPS var partition (decision for Colin); Skim: config.rs, baseline.rs, spec.rs, status.rs, man pages, |
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head d8cad1028e: 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.
Prototype of /var suggestions on top of bootc-dev#2521 (three operator commits, unchanged apart from a rebase). Bot commits add a blockdev lookup of the DPS Variable Data Partition, --var-mount-spec (alias --var) with default discovery (bootc mounts /var, initializes it from the image, unmounts it, and records it in fstab or systemd.mount-extra), and move the feature list from container inspect to bootc --version. Risk is in the mount lifecycle and cleanup on error, in plan selection (CLI over config, empty value disables everything, discovery adopts a non-empty DPS var partition by default), and in the changed --version output for existing parsers. Sealed composefs UKI cannot record the mount, so an explicit spec errors and discovery is skipped.
Hotspots
- look-closely · logic —
crates/lib/src/install/var_mounts.rs:55-114(71e2086): Plan selection: empty disables all, a spec on a UKI errors, caller mounts win over discovery, discovery adopts any DPS var partition with a known filesystem. Check this default is what the operator wants. - look-closely · error-handling —
crates/lib/src/install/var_mounts.rs:122-176(71e2086): mount() creates the mountpoint and mounts; every early return after create_dir must leave nothing behind, which the Mounted guard handles. Also the already-mounted-at-/var path compares sources via findfs/canonicalize. - look-closely · error-handling —
crates/lib/src/install/var_mounts.rs:200-250(71e2086): unmount and Drop: failure falls back to a lazy umount, then removes the mountpoint. Lazy detach could hide a still-busy mount; confirm warnings are acceptable. - look-closely · logic —
crates/lib/src/install.rs:2906-2956(71e2086): Plan wiring: CLI overrides config, host-root/existing-ostree/alongside installs reject an explicit spec but only warn on a config one, and a mounted /var is rediscovered after mount to drive populate. - note · logic —
crates/lib/src/install.rs:2279-2290(71e2086): The bootc-mounted /var is unmounted after sync and before finalization; the fstab/karg record is written earlier, so ordering matters if a later step fails. - note · logic —
crates/blockdev/src/blockdev.rs:236-245(0e123a7): The first VAR partition across all root devices wins; with several candidates the choice is arbitrary. - note · api —
crates/lib/src/cli.rs:976-990(d8cad10):bootc --versionnow has a second line and a Features list. Consumers that compare the whole output break; the in-tree .nu parsers were updated, external ones may not be.
Safe to skim
docs/src/man/bootc-install-config.5.md: docs for the new keydocs/src/man/bootc-install-to-filesystem.8.md: docs for the new option and feature flagcrates/lib/src/install/baseline.rs: one-line struct field defaultcrates/lib/src/install/config.rs: plain config key plus merge and parse teststmt/tests/booted/bootc_testlib.nu: one-line parser fix to use the first line of --versiontmt/tests/booted/test-49-composefs-1-16-bridge.nu: one-line parser fix to use the first line of --version
Prototype of cgwalters's review suggestions on bootc-dev#2521, rebased onto current main and stacked on that PR's three commits (the top three commits here are new):
--var-mount-spec(alias--var) and config keyvar-mount-spec; discovery of a DPS Variable Data Partition by default; an empty value disables discovery and all /var handling; and global features inbootc --versioninstead ofinstall-featuresincontainer inspect.Behavior: with a spec (or a discovered, mountable DPS var partition) bootc mounts it at the target's
/var, initializes it from the image, then unmounts it and records the mount (fstab for ostree,systemd.mount-extrafor composefs BLS). Caller-mounted /var trees keep the PR's behavior. Discovery errors only warn.Tested on a 16-core RHEL 10 devspace (jmarrero-devspace-36971106383) at head d8cad10: fmt, clippy (Makefile flags),
cargo test -p bootc-lib -p bootc-internal-blockdev(290 + 11 passed),cargo xtask update-generated direct --check, and tmt plan-32-install-to-filesystem-var-mount (caller-mounted, explicit spec, DPS and empty-spec scenarios) passed on ostree/grub, composefs systemd ext4 BLS and composefs systemd ext4 UKI (unsealed; an explicit spec errors there). Not run: sealed UKI, the#[ignore]real-mount unit tests, the.nuversion parsers.Open: whether
--varshould be the primary name; whether empty should also disable caller-mounted initialization; whether the default should adopt a non-empty DPS var partition (it does, and records it in fstab); sealed UKI composefs cannot record the mount. Commit 3 removes what bootc-dev#2521's third commit adds, so it should be squashed there.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.
bootc-dev/bootc, basemainPVTI_lADOFBIZ7c4BlZPrzg-DA9sTo review:
/promoteon a line of its own, to open it upstream, ready for review. Either covers only the commits pushed so far.Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>to the commits lacking it (the bot's and yours; anyone else's only if you ask), with you as committer./draftline (in the same comment or before) to open it upstream as a draft (/readyundoes that).