Repository navigation
install: Initialize separately mounted /var filesystems from the image - #2
jmarrero-bot wants to merge 8 commits into
Conversation
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head 83fe4960e0: 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 stacked on bootc-dev#2521 (the first two commits are that PR and are not reviewed here). It replaces bootc-dev#2521's copy-from-the-seeded-stateroot-var with one cp -a per empty filesystem from the image's /var, run in a private mount namespace that covers other filesystems' mountpoints with tmpfs; adds --defer-var (ostree: write libostree's .ostree-selabeled stamp so prepare_deployment_var() skips seeding; composefs: skip the initial copy); adds bootc state initialize-var for booted systems; and makes composefs setup-root skip its /var bind mount when fstab has /var. The mount-namespace code is sound (allocation-free pre_exec, rprivate before mounting, fails closed). The main risks: the day-two command scans every mount under /var, including empty tmpfs mounts owned by podman or kubelet, and relabels their roots; using libostree's stamp also turns off libostree's relabel of the stateroot var root, which --defer-var alone never redoes; and the setup-root fstab change switches /var to the fstab volume on existing composefs systems that have run on the state directory's var until now. Unit tests, clippy, a VM matrix and tmt were run on a devspace. tmt covers install only; day two and boot are covered only by the gist's VM matrix.
Hotspots
- risky · security —
crates/lib/src/install/var_mounts.rs:481-487(cd8c3ae): On a booted host, discover("/") also returns pseudo and runtime mounts under /var, such as podman's per-container shm tmpfs (userdata/shm) or kubelet emptyDir and secret tmpfs. An empty one passes is_empty_fs, the image has no content for it, and label() then relabels its root or fails on a context= mount. Restrict this to block-backed filesystems, or require PATH to be a mountpoint. - look-closely · logic —
crates/lib/src/install/var_mounts.rs:394-397(b0d2f30): A mount the image has no content for is still added to prepared.mounts, so it gets relabeled and synced. That is fine at install time, but on day two it is how foreign empty mounts get relabeled (see above). - look-closely · security —
crates/lib/src/install/var_mounts.rs:290-322(b0d2f30): pre_exec hook: unshare(NEWNS), then rprivate /, then tmpfs mounts. It makes no allocations and fails closed before mounting anything. A failure surfaces as a bare errno with no hint of which path failed to mount. - note · logic —
crates/lib/src/install/var_mounts.rs:260-277(2ba316c): Hidden mountpoint directories in the stateroot var are copied from the tmpfs root, so they carry the tmpfs's SELinux xattr until label() relabels the stateroot var. That relabel is unconditional, so the result is correct, but only because of it. - look-closely · logic —
crates/lib/src/install/var_mounts.rs:448-467(2ba316c): libostree's stamp also suppresses selinux_relabel_var_if_needed(), which normally labels the stateroot var root as /var. With --defer-var alone, label() never runs, so the root keeps the physical-path label from the final walk (usr_t on ostree, default_t on composefs) until tmpfiles fixes it at boot. - note · logic —
crates/lib/src/install.rs:2233-2240(2ba316c): initialize_var holds only with --initialize-var-mounts, so --defer-var alone skips populate() and label() entirely. That is where the stateroot var root label gap above comes from. - look-closely · logic —
crates/lib/src/install/var_mounts.rs:104-147(b0d2f30): The emptiness rules decide what gets overwritten. If a cp fails partway, the filesystem is left non-empty, and the next run prints Preserving and skips it, leaving a half-initialized volume. That matters for the day-two command. - risky · api —
crates/initramfs/src/lib.rs:680-694(3a61f24): This changes behavior for existing composefs systems: one whose fstab has /var has been running on state/os/default/var, since the bind mount won. After an update to this initramfs it boots on the fstab volume instead, so its /var data appears to vanish. Call this out, or gate it. - note · logic —
crates/initramfs/src/lib.rs:522-534(3a61f24): openat follows symlinks, so an absolute symlink at the state's etc/fstab would resolve inside the initramfs rather than the deployment. This is an unlikely edge case. - note · test-gap —
tmt/tests/booted/test-install-to-filesystem-var-mount.sh:293-333(83fe496): The tmt test covers install only. The day-two command, the setup-root fstab change and booting a deferred /var are covered only by the gist's VM matrix, and the --defer-var-alone case never checks labels.
Safe to skim
docs/src/man/bootc-install-to-filesystem.8.md: Prose matching the code; the options section is generateddocs/src/man/bootc-setup-root-conf.5.md: One doc paragraphcrates/lib/src/bootc_composefs/boot.rs: Passes the new flag throughcrates/lib/src/bootc_composefs/update.rs: Passes false for the new flagcrates/lib/src/install/baseline.rs: New struct field defaulttmt/tests/tests.fmf: Duration bump, regeneratedcrates/lib/src/install.rs:3426-3480: Table test rewrite for check_var_options
jmarrero-bot
left a comment
There was a problem hiding this comment.
Needs rework at 83fe496 (reviewed only 5f31d97..83fe496).
- P2 — wrong fstab view:
crates/initramfs/src/lib.rs:685-695always prefers stateetc/fstab, including with[etc] mount = "none"or"transient", where that file is not the booted /etc. A stale state /var entry can suppress the bind mount while systemd sees no /var entry, leaving /var read-only; the reverse hides the intended volume. Mount/configure /etc first, then inspect its effective fstab throughvisible_root. Do not fall back to the image fstab when the effective bind-mounted /etc has deleted it. Add tests with conflicting state/image fstabs and these /etc modes. - P2 — lost mountpoint metadata:
crates/lib/src/install/var_mounts.rs:262-277,301-308reconstructs hidden source directories with only mode/uid/gid. The deferred stateroot placeholders consequently lose image xattrs/default ACLs (and timestamps); I reproduceduser.imageandsystem.posix_acl_defaultdisappearing on the devspace. Preserve the original directory metadata on the placeholders, or prepare fully attributed empty surrogate directories before spawning cp; add an xattr/default-ACL assertion to the namespace test.
The per-filesystem/hardlink deviations and empty-only policy are reasonable prototype choices; the stamp matches libostree’s current prepare_deployment_var() and relabel checks. The pre_exec hook uses preallocated C strings and isolates propagation. Reran fmt, diff/commit hygiene and shell syntax locally; on the existing devspace, clippy (Makefile flags), 303 bootc-lib tests, 11 initramfs tests and the ignored namespace test passed. Reviewed the linked VM/tmt evidence; existing tests miss the two cases above.
83fe496 to
859eb94
Compare
|
Review fixes pushed as 859eb94, squashed into the commits they belong to:
Left as open questions in the body: an interrupted copy, the |
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head 859eb94349: 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 stacked on bootc-dev#2521 (the first two commits are that PR and are not reviewed here). It replaces bootc-dev#2521's copy-from-the-seeded-stateroot-var with one cp -a per empty filesystem from the image's /var, run in a private mount namespace that covers other filesystems' mountpoints with tmpfs; adds --defer-var (ostree: write libostree's .ostree-selabeled stamp so prepare_deployment_var() skips seeding; composefs: skip the initial copy); adds bootc state initialize-var for booted systems, limited to block-device filesystems; and makes composefs setup-root skip its /var bind mount when the fstab /etc will show has /var. The namespace code is sound. This head fixes the earlier review's findings: the day-two scan skips tmpfs and network mounts, a deferred stateroot var root is labeled as /var, deferred mountpoint placeholders get the image directory's metadata, and the commit message documents the change for existing composefs systems. What remains is smaller. Bind mounts of block filesystems still count as block-backed, and the day-two command still labels empty mounts the image has no content for. copy_dir_metadata is stricter about xattrs than cp -a. setup-root's fstab read errors on an overlay whiteout. Unit tests, clippy, the VM matrix and tmt (ostree and composefs BLS) were run at this head, per the PR body and gist.
Hotspots
- look-closely · security —
crates/lib/src/install/var_mounts.rs:99-101(21f9da7): Bind mounts of block filesystems (source /dev/X[/path]) still count. An empty one, such as a kubelet subPath bind under /var/lib/kubelet/pods, would be treated as a fresh filesystem, and since the image has no content for it, its root would be relabeled. Consider requiring the filesystem root (no [..] in the source) for everything but /var itself. - note · logic —
crates/lib/src/install/var_mounts.rs:454-457(5b52260): A mount the image has no content for is still labeled. That is right at install time, but on day two it is the only reason a foreign empty mount gets touched. Skipping these in day-two mode would close the bind-mount case above as well. - look-closely · error-handling —
crates/lib/src/install/var_mounts.rs:339-377(351dc87): The ordering is fine (chown, then chmod, then xattrs, then times last). But any lsetxattr failure aborts the install, where cp -a silently skips xattrs it can't set, for example a security.selinux value the host policy rejects, or security.ima/evm. label() relabels the stateroot var anyway, so skipping security.selinux or tolerating ENOTSUP/EPERM would match cp. - note · logic —
crates/lib/src/install/var_mounts.rs:433-441(351dc87): Placeholder metadata is copied from the image's real directory, outside cp's namespace, after cp has created each placeholder from the hiding tmpfs. Only hidden paths that check_source_path accepted are included, so there are no symlinks to follow. - note · logic —
crates/lib/src/install/var_mounts.rs:479-505(351dc87): The stateroot var root is now relabeled as /var whenever --defer-var or --initialize-var-mounts is given, including when the backend seeded it. That case is harmless, since the root is already var_t. - note · security —
crates/lib/src/install/var_mounts.rs:290-334(5b52260): The pre_exec namespace hook makes no allocations, makes the mount tree private before mounting, and fails before mounting anything if a step errors. Errors now name the hidden paths. - look-closely · error-handling —
crates/initramfs/src/lib.rs:690-706(2a47cc0): With an overlay /etc, a deleted fstab is a whiteout (a 0/0 char device) in the state's upper directory. read_optional then fails to open it (ENXIO), and setup-root aborts the boot. Treat non-regular files as absent, and open with O_NOFOLLOW. - note · logic —
crates/initramfs/src/lib.rs:528-540(2a47cc0): This follows symlinks, so an absolute etc/fstab symlink in the state would resolve inside the initramfs. It is left as an open question in the PR. - note · test-gap —
tmt/tests/booted/test-install-to-filesystem-var-mount.sh:321-334(859eb94): The --defer-var-alone case now checks the stateroot var root's label. The day-two command and the fstab detection in setup-root are still covered only by the gist's VM matrix.
Safe to skim
docs/src/man/bootc-install-to-filesystem.8.md: Prose matching the code; the options section is generateddocs/src/man/bootc-setup-root-conf.5.md: One doc paragraphcrates/lib/src/bootc_composefs/boot.rs: Passes the new flag throughcrates/lib/src/bootc_composefs/update.rs: Passes false for the new flagcrates/lib/src/install/baseline.rs: New struct field defaulttmt/tests/tests.fmf: Duration bump, regeneratedcrates/lib/src/install.rs:3424-3478: Table test rewrite for check_var_optionscrates/initramfs/src/lib.rs:123-139: MountConfig::mount_type, extracted from mount_subdir unchanged
859eb94 to
afc7658
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head afc76580e9: 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 stacked on bootc-dev#2521 (the first two commits are that PR and are not reviewed here). It replaces bootc-dev#2521's copy-from-the-seeded-stateroot-var with one cp -a per empty filesystem from the image's /var, run in a private mount namespace that covers other filesystems' mountpoints with tmpfs; adds --defer-var (ostree: write libostree's .ostree-selabeled stamp so prepare_deployment_var() skips seeding; composefs: skip the initial copy); adds bootc state initialize-var for booted systems, limited to block filesystems mounted from their root (plus /var itself); and makes composefs setup-root skip its /var bind mount when the fstab /etc will show has /var. All findings from both review rounds are fixed or left as open questions in the PR body. What remains is design: the stamp versus a bootc marker or a libostree option, re-run semantics, the behavior change for existing composefs systems with a /var fstab entry, and day-two support for ZFS and btrfs subvolume mounts. Most of the risk is in var_mounts.rs.
Hotspots
- note · logic —
crates/lib/src/install/var_mounts.rs:101-103(8e42d4f): Excluding sources with [..] also excludes btrfs subvolume mounts (/dev/sda2[/@x]) and ZFS datasets, so those volumes can't be initialized on day two. That is conservative and acceptable, but worth documenting. - note · security —
crates/lib/src/install/var_mounts.rs:296-336(5b52260): The pre_exec namespace hook makes no allocations, makes the mount tree private before mounting, and fails before mounting anything if a step errors. Errors name the hidden paths. - look-closely · error-handling —
crates/lib/src/install/var_mounts.rs:341-389(338c2d2): Placeholder metadata is copied after cp. It skips security.selinux (the stateroot var is relabeled afterwards anyway) and tolerates ENOTSUP and EPERM like cp -a, while chown, chmod and utimensat stay strict. - note · logic —
crates/lib/src/install/var_mounts.rs:491-517(338c2d2): The stateroot var root is relabeled as /var whenever --defer-var or --initialize-var-mounts is given, since the stamp keeps libostree from doing it. - look-closely · api —
crates/initramfs/src/lib.rs:700-716(823fc4a): This changes behavior for existing composefs systems with a /var fstab entry: they move from the state directory's var to the fstab filesystem. The commit and the PR body say so; whether that is acceptable is a design question. - note · error-handling —
crates/initramfs/src/lib.rs:529-552(823fc4a): A symlinked etc/fstab now reads as absent, so such a system keeps the bind mount (the old behavior) rather than failing to boot. Whiteouts and FIFOs are also treated as absent.
Safe to skim
docs/src/man/bootc-install-to-filesystem.8.md: Prose matching the code; the options section is generateddocs/src/man/bootc-setup-root-conf.5.md: One doc paragraphcrates/lib/src/bootc_composefs/boot.rs: Passes the new flag throughcrates/lib/src/bootc_composefs/update.rs: Passes false for the new flagcrates/lib/src/install/baseline.rs: New struct field defaulttmt/tests/tests.fmf: Duration bump, regeneratedcrates/initramfs/src/lib.rs:123-139: MountConfig::mount_type, extracted from mount_subdir unchanged
jmarrero-bot
left a comment
There was a problem hiding this comment.
Needs rework at afc7658: mountpoint metadata is fixed; the fstab-view finding remains for persistent /etc overlays.
- P2 — wrong overlay fstab view:
crates/initramfs/src/lib.rs:704-708readsstate/etc/fstab, butbuild_overlay_fd()usesstate/etc/upperas the upperdir (lines 295-302). An upper-layer fstab adding /var is missed, and a whiteout deleting the image fstab still falls back to that image file, suppressing the /var bind mount despite no effective /var entry. Mount /etc first, then read its effective fstab throughvisible_root; add conflicting upper/lower and whiteout cases.
The bind/none/transient selection and directory metadata restoration look correct by inspection, as do the additional bind-mount filter, xattr error handling and deferred-root labeling. Reviewed the range-diff; diff and commit hygiene checks pass. Read-only follow-up: no tests rerun or devspace started; the PR records testing at 859eb94.
afc7658 to
dd929f0
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head dd929f0330: 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 stacked on bootc-dev#2521 (the first two commits are that PR and are not reviewed here). It replaces bootc-dev#2521's copy-from-the-seeded-stateroot-var with one cp -a per empty filesystem from the image's /var, run in a private mount namespace that covers other filesystems' mountpoints with tmpfs; adds --defer-var (ostree: write libostree's .ostree-selabeled stamp so prepare_deployment_var() skips seeding; composefs: skip the initial copy); adds bootc state initialize-var for booted systems, limited to block filesystems mounted from their root (plus /var itself); and makes composefs setup-root skip its /var bind mount when the fstab /etc will show has /var. All review findings are fixed or left as open questions in the PR body. What remains is design: the stamp versus a bootc marker or a libostree option, re-run semantics, the behavior change for existing composefs systems with a /var fstab entry, and day-two support for ZFS and btrfs subvolume mounts. Most of the risk is in var_mounts.rs.
Hotspots
- note · logic —
crates/lib/src/install/var_mounts.rs:101-103(4e0db6d): Excluding sources with [..] also excludes btrfs subvolume mounts (/dev/sda2[/@x]) and ZFS datasets, so those volumes can't be initialized on day two. That is conservative and acceptable, but worth documenting. - note · security —
crates/lib/src/install/var_mounts.rs:296-336(5b52260): The pre_exec namespace hook makes no allocations, makes the mount tree private before mounting, and fails before mounting anything if a step errors. Errors name the hidden paths. - look-closely · error-handling —
crates/lib/src/install/var_mounts.rs:341-389(338c2d2): Placeholder metadata is copied after cp. It skips security.selinux (the stateroot var is relabeled afterwards anyway) and tolerates ENOTSUP and EPERM like cp -a, while chown, chmod and utimensat stay strict. - note · logic —
crates/lib/src/install/var_mounts.rs:491-517(338c2d2): The stateroot var root is relabeled as /var whenever --defer-var or --initialize-var-mounts is given, since the stamp keeps libostree from doing it. - look-closely · api —
crates/initramfs/src/lib.rs:702-727(a35622b): This changes behavior for existing composefs systems with a /var fstab entry; the commit and the body say so. With an overlay /etc it reads state/etc/upper/fstab (a whiteout reads as absent) and otherwise falls back to the image's. - note · error-handling —
crates/initramfs/src/lib.rs:531-551(a35622b): A symlinked etc/fstab now reads as absent, so such a system keeps the bind mount (the old behavior) rather than failing to boot. Whiteouts and FIFOs are also treated as absent.
Safe to skim
docs/src/man/bootc-install-to-filesystem.8.md: Prose matching the code; the options section is generateddocs/src/man/bootc-setup-root-conf.5.md: One doc paragraphcrates/lib/src/bootc_composefs/boot.rs: Passes the new flag throughcrates/lib/src/bootc_composefs/update.rs: Passes false for the new flagcrates/lib/src/install/baseline.rs: New struct field defaulttmt/tests/tests.fmf: Duration bump, regeneratedcrates/initramfs/src/lib.rs:123-139: MountConfig::mount_type, extracted from mount_subdir unchanged
|
Second round of review fixes, now at dd929f0 (squashed into the cli, defer and composefs commits):
At dd929f0: fmt, clippy, doc, |
jmarrero-bot
left a comment
There was a problem hiding this comment.
opencode review of dd929f0: changes needed.
P2 — raw overlay upper is not the effective fstab (crates/initramfs/src/lib.rs:710-719). With OverlayFS metacopy enabled (a kernel/module default this mount does not override), chmod/chown of a lower /etc/fstab can leave an empty metadata-only upper file. The merged fstab still contains the lower /var entry, but this code reads the empty raw upper file and binds the state var over the intended volume. Mount/configure /etc first, then read etc/fstab through visible_root; this also delegates opaque-directory and whiteout semantics to OverlayFS. Add effective-view regression cases for conflicting upper/lower files, deletion and metadata-only copy-up. The recorded VM cases explicitly use no overlay /etc, and the added unit tests only exercise parsing and mount-type selection.
The host-directory bind-mount filter and prior placeholder-metadata/deferred-root-label fixes look correct. Reviewed only 5f31d979..dd929f03, the range-diff from afc7658, prior reviews/fix summaries and linked test evidence. Locally reran cargo fmt --check, shell syntax, git diff --check and bot-git check (all passed). Static review; no build or devspace run.
Review guide for head dd929f0330: 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.
Independent static recheck of the five prototype commits above bootc-dev#2521. The day-two host-directory bind-mount filter and earlier metadata and labeling fixes look correct. The remaining correctness issue is setup-root reading the raw persistent /etc overlay upper layer rather than the merged fstab: a metadata-only copy-up can make these contents differ. The recorded VM matrix explicitly does not use overlay /etc; the unit tests cover parsing and mount-type selection, not effective file lookup. The stamp, interrupted-copy semantics and behavior change for existing composefs systems remain documented prototype design questions.
Hotspots
- risky · logic —
crates/initramfs/src/lib.rs:710-719(a35622b): With OverlayFS metacopy enabled, chmod of a lower fstab can create an empty upper metadata-only file while the merged fstab still reads lower data. Reading the raw upper file then misses /var and binds the state directory over the intended volume. Mount /etc first and inspect its effective fstab through visible_root. - look-closely · test-gap —
crates/initramfs/src/lib.rs:756-792(a35622b): The tests verify mount-type selection and parsing but never exercise conflicting upper/lower fstabs, whiteouts or metadata-only copy-up. The linked VM matrix uses no overlay /etc, so it cannot detect the remaining lookup bug. - note · logic —
crates/lib/src/install/var_mounts.rs:96-103(4e0db6d): The filter rejects host-directory bind sources while retaining the intended state-directory bind at /var. The fixture covers kubelet binds, and btrfs subvolume and ZFS exclusions are documented prototype limitations. - look-closely · error-handling —
crates/lib/src/install/var_mounts.rs:341-389(338c2d2): Placeholder metadata is restored after cp leaves its private namespace. SELinux labels are left to the final logical-path relabel; forbidden and unsupported xattr writes are tolerated. The ordering matters to the earlier ACL and metadata-loss fix.
Safe to skim
crates/lib/src/bootc_composefs/boot.rs: Passes the defer flag through.crates/lib/src/bootc_composefs/update.rs: Passes false for the initial-install flag.crates/lib/src/install/baseline.rs: New field default.tmt/tests/tests.fmf: Generated duration change.
|
Reviewers disagree at dd929f0: the Claude reviewer said ship; opencode (review) says changes needed: with an overlay /etc, setup-root reads the raw upper-layer fstab, which can be wrong under metadata-only copy-up (an empty upper file whose content is in the lower layer) or an opaque /etc, so /var may be bound or not on the wrong fstab; the tests don't cover an overlay /etc. Its suggested fix: mount /etc first and read the effective fstab, plus overlay-view tests. The stricter verdict stands until resolved. |
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head 0438277436: 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.
Next revision of bootc-dev#2521, stacked on #5. --initialize-var-mounts discovers the filesystems mounted at or below the target's /var and fills each empty one from the image's /var (the ostree deployment checkout, or the mounted composefs image), one cp -a per filesystem in a private mount namespace where nested mountpoints are covered by tmpfs on both sides. With --defer-var, the stateroot var gets only what no mounted filesystem holds. bootc state initialize-var runs the same code on a booted system for block-device filesystems. A setup-root commit skips the composefs /var bind mount when the effective fstab (read through the mounted /etc) has a /var entry. The copy logic is careful and well covered by unit tests, tmt and VM runs. The riskiest piece is the setup-root change: it runs on every composefs boot, not only for users of the new flags, and any read error there (including a non-UTF-8 fstab) fails the boot. Also look at the pre_exec mount-namespace hook, the label pass, and the commit message in jmarrero's commit, which still says --version 'now prints' the Features list.
Hotspots
- risky · error-handling —
crates/initramfs/src/lib.rs:526-548(6ab7f0a): This runs in the initramfs on every composefs boot. read_to_string fails on a non-UTF-8 fstab, and other open or read errors propagate, so setup-root fails. Reproduced: one Latin-1 byte in a comment sends a UEFI composefs VM to emergency mode ('stream did not contain valid UTF-8'). Read the bytes and decode lossily, and on errors warn and keep the bind mount. - look-closely · logic —
crates/initramfs/src/lib.rs:699-716(6ab7f0a): This changes boot for existing composefs systems that already have a /var fstab entry: they move from the state directory to the fstab filesystem (the commit message says so). The read goes through visible_root after /etc is mounted, which is right. Only the pre-6.15 path is exercised by the default build; the floating-tree path is untested. - look-closely · security —
crates/lib/src/install/var_mounts.rs:252-318(f671436): pre_exec runs unshare(NEWNS) and makes / rprivate before the tmpfs mounts, so nothing leaks to the host namespace. It only allocates beforehand, which is correct after fork in a multithreaded process. The default view skips the namespace so unit tests can run unprivileged. - look-closely · logic —
crates/lib/src/install/var_mounts.rs:114-150(34fe90e): The emptiness check decides between filling and leaving data alone. It allows only an empty root lost+found and directories that lead to nested mountpoints; anything else, including a non-UTF-8 name or a symlink, counts as data. It is conservative in the right direction. - look-closely · logic —
crates/lib/src/install/var_mounts.rs:411-430(e9742e9): Deferred stateroot var fill. The source mountpoints are hidden by tmpfs, so cp gives the stateroot var's mountpoint directories tmpfs metadata, which copy_dir_metadata then restores from the unhidden source (SELinux label excluded; label() relabels the whole stateroot var as /var later). - note · error-handling —
crates/lib/src/install/var_mounts.rs:499-527(f671436): Failure path: the error names the mountpoint and source and says the filesystem may be partly filled. cp's stderr is attached by run_capture_stderr. Journal events are a no-op inside the install container (no systemd), as the man page says. - note · logic —
crates/lib/src/install/var_mounts.rs:91-93(159ee0f): The day-two filter: /dev/ sources without a [subpath], plus /var itself. This excludes btrfs subvolumes, ZFS datasets, NFS, tmpfs and bind mounts, as bootc-dev#2599 promises. - look-closely · logic —
crates/lib/src/install.rs:2432-2473(f671436): Ordering: populate runs after the deployment and before the physical-root relabel. Copied files keep the source labels, so the walk skips them, and label() then labels the mounts and the deferred stateroot var by their /var paths. The ImageVar (and the composefs mount) is dropped at the end of the if-let, before labeling. - note · test-gap —
tmt/tests/booted/test-install-to-filesystem-var-mount.sh:303-330(0438277): --defer-var --initialize-var-mounts is tested only without a /var volume. The STIG-shaped case (a /var volume with nested volumes, and --defer-var) was booted neither here nor in the gist's VM cases.
Safe to skim
crates/lib/src/install/var_mounts.rs:627-1128: unit testscrates/initramfs/src/lib.rs:734-792: unit testsdocs/src/man/bootc-setup-root-conf.5.md: doc paragraph
jmarrero-bot
left a comment
There was a problem hiding this comment.
Reviewer verdict at 0438277: fix first. The copy logic, the mount-namespace handling, labels, hardlinks, nested volumes and the failure and journal path are sound. Two things need fixing before promote.
crates/initramfs/src/lib.rs:526-548(risky). setup-root now reads/etc/fstabon every composefs boot.read_to_stringfails on non-UTF-8 content, and any other open or read error propagates, so the boot fails. Reproduced on a UEFI composefs VM: one Latin-1 byte in an fstab comment goes to emergency mode (Reading etc/fstab: stream did not contain valid UTF-8). Fix: read the bytes and decode them lossily (/varis ASCII). On any other error, warn and keep the bind mount (the old behavior). Add a case with a0xffbyte.- 34fe90e's message (jmarrero's) still says the
Features:list "bootc --versionnow prints", which #5 adds now. Reword it to "It is listed asinitialize-var-mountsin theFeatures:list ofbootc --version", and drop or move thecontainer inspectrationale to #5. Keep his authorship and say in the reply that the message changed.
Minor:
- The tmt hunk in 34fe90e also drops #5's
mkfs.vfat -iUUID preservation. That's harmless after b99826c, but the fork note doesn't mention it. The other conflict resolutions matchrange-diffagainst 5f31d97/c3949f7f. - Test gap:
--defer-var --initialize-var-mountswith a/varvolume plus nested ones (the STIG layout) wasn't booted in tmt or in the gist. On composefs it depends on 6ab7f0a. Add it to the e2e matrix. - 6ab7f0a's fstab read through the mounted
/etcis right for pre-6.15. The floating-tree path (non-default) is untested. bot-git checkflags b99826c and 34fe90e forGenerated-byunder jmarrero's authorship. He already carried that trailer in bootc-dev#2521, so this is fine.- bootc-dev#2599 says install-time failures reach the journal. Inside the install container
journal_sendis a no-op. The man page's "when it is available" is accurate.
Pre-existing on main, not caused by this PR: the overlay /etc comes up unlabeled_t. build_overlay_fd creates etc/upper and work in the initramfs before the policy loads (crates/initramfs/src/lib.rs:284-285), and the overlay root takes the upper dir's missing label. I confirmed this by reading the code, not by booting main.
Re-run on a 16-core devspace: fmt, clippy, doc, --no-default-features, update-generated (no diff), 350 + 12 unit tests, and mounted_var_filesystems under unshare -Urnm, all passing. VM: a composefs layout-D run with --defer-var --initialize-var-mounts on UEFI (OVMF) passed every check through boot, bootc status, switch to v2 and rollback, enforcing. That covers the composefs switch and rollback the body lists as untested. The body's other claims match the round-4 logs.
e2e (tracker#55): nothing here blocks it, since osbuild writes an ASCII fstab, but fix item 1 before promote. Run composefs e2e on UEFI, because BIOS hits main's SystemNotUEFI bug.
Generated-by: https://github.com/jmarrero-bot#llms
a20a461 to
3452994
Compare
0438277 to
d72d38a
Compare
|
Review fixes pushed: 0438277 is now d72d38a, on top of #5 at 3452994.
Results are in the body, logs in the gist (round 5). |
3452994 to
9506ed3
Compare
d72d38a to
4c65307
Compare
|
Restacked unchanged onto #5 at 9506ed3: d72d38a is now 4c65307. Static checks, unit tests (351 + 15), the namespace test and |
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. With the new --initialize-var-mounts option, a fresh to-filesystem installation discovers 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. The option is rejected for alongside and existing-deployment installs, before anything on the target is changed. Callers remain responsible for mounting these filesystems at boot. Without the option nothing changes, so callers that mount an empty /var filesystem and fill it themselves after installation are not affected. Doing this by default was considered, but would have changed behavior for them. Image builders can tell whether to pass the option from its `initialize-var-mounts` entry in the `Features:` list of `bootc --version`. 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 passes the option, checks contents, ownership, symlinks and SELinux labels on the actual volumes, including a nested one, and checks that without the option they stay empty. Related: bootc-dev#1615 Related: bootc-dev#997 Generated-by: AI
9506ed3 to
ff41dcc
Compare
4c65307 to
55fd0e5
Compare
|
Re-review fixes pushed: 4c65307 is now 55fd0e5, restacked on #5 at ff41dcc. jmarrero's two commits are unchanged.
Tested at the pushed commit (package |
55fd0e5 to
5875cfc
Compare
|
Second-review items and the e2e
Tested at the pushed commit: static checks, unit tests, |
5875cfc to
2139e25
Compare
|
Third-review fixes pushed: 5875cfc is now 2139e25. jmarrero's two commits are unchanged, and each fix is squashed into the commit it belongs to (lsm prep, per-filesystem, deferred /var, tmt).
One mutation still survives: not checking that the parent has a label before copying it. On an SELinux host an unlabeled parent is Logs: https://gist.github.com/jmarrero-bot/cb0ed128e38cab8cc220d7b21e63de0b (round 9). |
relabel() failed on any path the policy has no label for. In the targeted policy those are the <<none>> entries, such as /var/tmp/.*, which mean "don't relabel": setfiles and restorecon skip them. Relabeling a /var that holds image content there, as --initialize-var-mounts does, then failed the installation with "No label found in policy". Skip them too, keeping whatever label they have. A file without any label, for example one copied onto a fresh filesystem before its root was labeled, gets its directory's label instead, as it would if it had been created there; left unlabeled_t, confined services such as systemd-tmpfiles couldn't touch it. This applies to every caller of relabel(), all of which would otherwise have failed outright. With no policy loaded at all, every lookup comes back empty the same way: that now skips labeling too, saying so at debug level, like the <<none>> paths. The test writes a policy of its own from a file_contexts, so it doesn't depend on the host's; it skips where this process can't set and remove labels. Loading a policy sets libselinux's process-wide policy root, so new_sepolicy_at() now loads one at a time: with the test's policy and another test's loading in parallel threads, either could get the other's file contexts. The few direct SePolicy constructors left (the SELinux re-exec, the overlay relabel service, the internal relabel command) run alone in single-threaded command paths, and taking them through new_sepolicy_at() would change how they handle a missing policy, so they stay as they are. Generated-by: AI
--initialize-var-mounts copied the deployment backend's seeded /var into each empty mount tree, and skipped a whole tree when any of its filesystems held data. That made the stateroot copy the source of truth for the volumes, and a single volume restored from backup under an empty /var volume meant the system booted with none of the image's /var, and with /var unlabeled. Copy from the image's /var instead: the deployment's checkout on ostree, the mounted composefs image on composefs. Each empty filesystem gets its own `cp --archive`, run in a private mount namespace where the mountpoints of the filesystems nested in it are covered by a tmpfs, on both the source and the target side. So each file lands on the filesystem that holds it at boot, a filesystem with data is left alone without stopping its empty parent or children from being filled, and hardlinks are kept within a filesystem. This is also what lets a later change fill the stateroot var itself without duplicating what the volumes hold. An ostree commit doesn't record hardlinks: identical files share one object, so a deployment checkout links files that are separate in the image. Copying it with links preserved would make writes to one show up in the other, so on ostree every file is copied on its own, like libostree's own seeding does. composefs images record them, so there they are kept. The namespace is only entered when there is something to cover, so single-filesystem copies still work unprivileged in unit tests. A failed copy leaves its filesystem partly filled, which later runs take as holding data, so it is never repaired behind the user's back. The error says that, naming the filesystem's mountpoint and source, and later runs say which filesystems they skip because they hold data. The filesystems completed before a failure are still labeled and synced before the error is returned, since a retry leaves them alone too; each is tried on its own, so one failing to label or sync doesn't stop the others, and their errors are reported after the copy's. At install this happens right after the copies, before the physical root's relabel can fail. A volume that is still empty because the image has nothing for it counts as not initialized, so a later run copies and labels it again; the man page says so. Marking such volumes would put state on them. Labeling a filesystem's root and lost+found unconditionally skips a lost+found that something is mounted on, as the walk below them skips mountpoints: that belongs to another filesystem. The namespace test, which needs a private user and mount namespace, now runs with the others through unshare -Urnm where that is available, and says so when it skips. Fills, skips and failures are also logged to the journal under one message ID, like other bootc events. Generated-by: AI
With --initialize-var-mounts alone the deployment backend still seeds its state directory with the whole image /var, so everything that ends up on a mounted filesystem is also stored on the root filesystem, hidden under the mount at boot. Combined with --defer-var, which leaves the state directory with only its .bootc-defer-var marker, bootc fills it itself: it gets only what isn't on a mounted filesystem, and nothing at all when a filesystem is mounted on /var itself. The copy is the same per-filesystem one as for the mounts, with the mountpoints hidden, so it only leaves empty mountpoint directories (with the image's metadata) where the volumes go. It is labeled like libostree labels the /var it seeds. Whether to fill the state directory follows from --defer-var, rather than from the directory being empty, since on ostree it also holds libostree's own stamp by then. Messages and journal entries name it as /var, the deployment's state directory, and a failed copy there says to install again onto a fresh root, since it is filled only at installation. The two options are now checked together. Generated-by: AI
--initialize-var-mounts only helps at installation. A volume added to /var later, say a new LV for /var/opt/app, starts empty and hides the image's content for that path just like an uninitialized one did at install time. `bootc state initialize-var [PATH]` runs the same per-filesystem copy on a booted system: each empty block-device filesystem mounted at or below PATH (/var by default) gets its part of the booted deployment's /var, taken from the deployment's checkout on ostree and from its composefs image on composefs, and is then labeled and synced. Filesystems with data are left alone, so running it on /var as a whole only touches empty volumes. Other mounts there, like container tmpfs mounts, network filesystems or bind mounts of subdirectories, aren't ours to fill or label, but they stay boundaries: an empty volume with a tmpfs mounted below it is still filled, around the tmpfs. When only such mounts are there, the error names them. Filling a /var whose installation was deferred is not something this covers on its own: by the time anything can run it on the booted system, systemd-tmpfiles has already written into /var, so it isn't empty anymore. That needs either the image builder doing it at install time, or an early first-boot unit. It is advertised as "state-initialize-var" in `bootc --version`. Generated-by: AI
setup-root leaves /var to the fstab only on a system whose stateroot var has the --defer-var marker. On composefs, --initialize-var-mounts without --defer-var onto a filesystem mounted on /var then fills that filesystem, and setup-root bind-mounts the seeded state directory over it at boot, so the volume bootc just filled goes unused. So when a filesystem is mounted on the target's /var itself, whether bootc fills it or leaves it alone because it holds data, write a second marker, .bootc-var-mounts, into the stateroot var, labeled like /var's files, and have setup-root accept either marker. It is written after the first deployment: on ostree, anything in the stateroot var before it would stop libostree from seeding it, and the marker only matters to composefs anyway. Reusing .bootc-defer-var isn't possible, since it would also keep composefs from seeding the state directory that --initialize-var-mounts alone still seeds. The name is defined once, next to the other marker, in the initramfs crate. Existing systems, and installs with neither option, still have no marker, and keep the bind mount. Generated-by: AI
The point of combining the two is that a file under a mounted filesystem is stored only there. Check that on a target without a /var filesystem, where the state directory is /var at boot: it gets the image's /var minus the nested volume's content, which lands only on the volume, and both are labeled. Hardlinks within the volume are kept on composefs and split on ostree, whose commits don't record them. The --defer-var alone case now also runs with the nested volume, and checks that the state directory is labeled as /var. The installs also check that .bootc-var-mounts is written exactly when a filesystem is mounted on /var itself. The image also has content under /var/tmp, which the policy has no label for: it must not stop the labeling of the filesystems or of the state directory, and must not end up unlabeled. Generated-by: AI
2139e25 to
ce2a1e1
Compare
|
opencode's fix-first item is fixed: 2139e25 is now ce2a1e1. Only the lsm commit (784be9f) and the per-filesystem commit (598adef) changed, and only in tests and the lsm commit message.
Static checks and unit tests (358 + 16) pass, and |
A common setup puts
/var, or parts of it such as/var/log, on separate filesystems, for example LVM logical volumes. Hardening profiles such as DISA STIG and CIS ask for this. Withbootc install to-filesystem, these filesystems currently end up empty. The deployment backend seeds the image's/varinto its own state directory, and at boot the separately mounted filesystems hide that content. This was reported in osbuild/bootc-image-builder#1222. This is the second part of bootc-dev#2599. It is stacked on the--defer-varPR (#5 until that is opened upstream), which also has the composefs setup-root change that keeps a deferred/varvolume in fstab from being hidden.This series:
to-filesysteminstall, bootc discovers the filesystems mounted at the target's/varor below and fills the empty ones from the image, then SELinux-labels and syncs them. The option is opt-in, and it is listed in theFeatures:list ofbootc --version.<<none>>as they are. A prep fix. Labeling failed on paths the policy has no label for, such as/var/tmp/.*, so image content there failed the install. Like setfiles and restorecon, such a path now keeps its label. A file with no label at all gets its directory's, as a file created there would. So on ostree such a file keeps the label of the deployment checkout's file (system_conf_tfor/var/tmpcontent), and on composefs it usually gets its directory's (tmp_t). With no policy loaded, labeling is skipped with a debug message, and policies are loaded one at a time, since loading sets libselinux's process-wide policy root./var(the deployment checkout on ostree, the mounted image on composefs) instead of from the seeded copy. Each empty filesystem gets its owncp -a, run in a private mount namespace where the mountpoints of the filesystems nested in it are hidden, so each file lands on the filesystem that holds it at boot. A filesystem that already has data is left alone, but its empty parent is still filled. Hardlinks are kept within a filesystem on composefs. On ostree they become separate files, because a commit doesn't record them. A failed copy fails the command with an error that names the filesystem and says it may be partly filled and won't be filled again on its own. Filesystems completed before the failure are still labeled and synced, each on its own, and their errors are reported after the copy's. Alost+foundthat something is mounted on is left to that filesystem. Later runs say which filesystems they skip. Fills, skips and failures are logged to the journal.--defer-var, the state directory gets only what isn't on a mounted filesystem, so nothing is stored twice. Errors for it name it as/var, the deployment's state directory, and a failed copy there says to install again onto a fresh root.bootc state initialize-var. This runs the same routine on a booted system, for example for a volume added later. It only fills block-device filesystems mounted from their root. Other mounts, like a container's tmpfs, still count as boundaries, so an empty volume with a tmpfs below it is filled around it./var, bootc writes a second marker,.bootc-var-mounts, into the staterootvar, and setup-root accepts it like install: Add --defer-var to leave the stateroot /var unseeded #5's.bootc-defer-var. Without it, on composefs--initialize-var-mountsalone fills the/varvolume, and setup-root then bind-mounts the seeded state directory over it. Reusing.bootc-defer-varwould also stop composefs from seeding the state directory. An initramfs older than this ignores the marker after a switch or rollback, like.bootc-defer-var.Only empty filesystems are filled, and nothing is merged into existing data. A volume left empty because the image has nothing for it still counts as not initialized, so a later run copies the image's empty directory onto it again and relabels its root. The man page says so. Skipping it would need a record of the first run on the volume or in the state directory, and putting state on a caller's volume seemed worse than an idempotent re-copy. Subsequent image updates don't update
/var. Callers remain responsible for mounting these filesystems at boot.Tested at 2139e25 on a fresh 16-core RHEL 10 devspace (jmarrero-devspace-40915942997). The images report package
bootc-202610291722.g2139e251c3-1.el10. Since then only tests changed: static checks, unit tests and mutations ran again at ce2a1e1 (jmarrero-devspace-40932828423). Logs: https://gist.github.com/jmarrero-bot/cb0ed128e38cab8cc220d7b21e63de0b (rounds 9 and 10).cargo fmt --check, clippy (Makefile flags),cargo docwith-D warnings,cargo check --no-default-features(lib),cargo xtask update-generated direct --check, andupdate-generated from-codewith no diff.cargo test -p bootc-lib -p bootc-initramfs-setup: 358 + 16 passed, and the defaultbootc-unitscontainer run passed.cargo check --testspasses at each commit.relabel_none,finish_labels_and_syncs) build a policy from afile_contextsfile, so they need no host policy. They skip where this process can't both set and remove labels, judged from the raw extended attribute only, which includes the defaultjust unit-testscontainer on SELinux hosts. Otherwise they assert that bootc's own classification sees the result as unlabeled, so a regression there fails rather than skips them. So upstream CI can't be counted on to run them, and there only tmt covers<<none>>. In the container withSYS_ADMINandspc_t, they now skip, where 5875cfc failed.finish()doing nothing, stopping at the first error, dropping later errors, syncing only after a successful label, not labeling the stateroot var, the real finisher not labeling, not syncing, or labeling the stateroot var by its own path, the stateroot var's failure text, labeling a mounted-onlost+found, day two filtering the boundaries, the scope message,hide_target, writing or reading the second marker, andrelabel()failing on a<<none>>path, overwriting its label, or not inheriting one, and the unlabeled type never being looked up. Not copying an unlabeled parent's label is only observable where labels can be removed (not on an SELinux host).plan-32-install-to-filesystem-var-mountcovers--initialize-var-mountswith a nested volume, the same install without it,--defer-var --initialize-var-mountswithout a/varvolume, and--defer-varalone. It checks that.bootc-var-mountsis written exactly when a volume is mounted on/var, and the test image has a file under/var/tmp, which the policy labels<<none>>. Its directory must get/var/tmp's label on both backends. It PASSED on ostree/grub and composefs grub ext4 BLS. It fails on composefs without the label inheritance, and in round 8 it failed without the install-time marker call (composefs) and without the<<none>>fix (both backends, with and without a/varvolume). composefs systemd-boot xfs UKI (unsealed) last ran at 0438277 and passed.<<none>>paths and of a mounted-onlost+foundchanged, which tmt and the unit tests cover:--defer-var --initialize-var-mountswith nested volumes and no/varvolume: content, owners, modes, labels, and no duplicate in the staterootvar. Thenbootc state initialize-varon a new volume. A forced failure (immutable target) gives the error, and a journal entry withMESSAGE_ID=a3dbd7d2…. A re-run names each filesystem it skips.bootc switchto v2 androllbackkeep the volumes' data on both backends./varvolume with nested volumes and--defer-var --initialize-var-mounts, on ostree and composefs: all checks pass through switch and rollback. On composefs/varis the volume at boot.--initialize-var-mountsalone, with an overlay/etcwhose image fstab mounts/var: the staterootvarhas.bootc-var-mounts, setup-root leaves/varto fstab, and/varis the volume at first boot. It stays the volume after a metadata-only copy-up offstab(overlay.metacopy=Y). An upperfstabwithout the/varline, and a whiteout, both give the state directory's bind mount. This case ran withenforcing=0, because an overlay/etccomes upunlabeled_tunder enforcing SELinux, also on main (draft upstream issue to follow).Open questions:
bootc statestay hidden?initialize-varis meant for admins.--varand DPS are out of scope here (install: Defer the /var copy and initialize separately mounted /var filesystems from the image bootc-dev/bootc#2599, future work; tracked in design: DPS Variable Data Partition support for install to-filesystem tracker#54).Closes bootc-dev#997, closes bootc-dev#1615. Related: bootc-dev#336, bootc-dev#1727, osbuild/image-builder#2733
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_lADOFBIZ7c4BlZPrzhAZed0To 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).Bot run: session e7bb5c51, agents a819c411 +2 · claude-opus-5-5 102.5M in (97% cached) / 249k out · ~$41.99 inference + ~$4.99 compute (est., list prices) · 2h 5m wall · 31.7 core-h, Actions run 40714907801
Bot run: session e7bb5c51, agents a819c411 +2 · claude-opus-5-5 223.6M in (95% cached) / 378k out · ~$108.13 inference + ~$11.52 compute (est., list prices) · 17h 57m wall · 73.1 core-h, Actions runs 40832841988, 40714907801
Bot run: session e7bb5c51, agents a6341f71 +1 · claude-opus-5-5 30.0M in (97% cached) / 137k out · ~$12.66 inference + ~$2.58 compute (est., list prices) · 15h 54m wall · 16.4 core-h, Actions runs 40848700131, 40739864348