Repository navigation
install/migrate: Use the RPM database as the --merge-etc baseline, check before install - #12
jmarrero-bot wants to merge 5 commits into
Conversation
A plain 3-way comparison treats every difference between pristine and current /etc as intentional. The package-mode migration has better information about which host files are just package defaults, and needs to leave those out of the merge (and out of the unmergable check) without reimplementing the diff. Prep for filtering the --merge-etc diff by the host's RPM database. Generated-by: AI
e8f201f to
45cede0
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head 45cede0f6c: 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.
Reworks --merge-etc from bootc-dev#2384. The baseline is now the host's RPM database: host /etc entries that rpm -V would accept are dropped from the diff, so the image's copy wins. %ghost files and %config files whose content RPM does not verify stay the host's. The merge is dry-run against a copy of the installer container's /etc before anything is installed (skipped with --source-imgref). The image's users and groups are appended to the carried account databases. The dry run, the Diff filter API and the help fix are sound. The risk is in the classification. A %ghost file is always the host's, even when it is a cache generated from package content. A digest-unverified, non-%config file always comes from the image. Within /etc/selinux this mixes the two: policy.35 and file_contexts come from the image, but file_contexts*.bin (%ghost) are carried from the host with a fresh mtime, so libselinux would read the host's stale .bin. Entries the image wins keep the image's numeric owner (for example /etc/sssd root:sssd), while /etc/group is the host's, and dynamically allocated system ids often differ. Derived caches (ld.so.cache, ca-trust/extracted, hwdb.bin) are carried and stay pinned against later upgrades.
Hotspots
- risky · logic —
crates/lib/src/install/migrate/package_defaults.rs:218-222(abea6ce): Every %ghost file is carried from the host. That includes generated caches such as ld.so.cache, pki/ca-trust/extracted/, udev/hwdb.bin and selinux//contexts/files/*.bin. Once they differ from /usr/etc they stay pinned across later upgrades, and the SELinux .bin files no longer match the image's policy and file_contexts. - risky · logic —
crates/lib/src/install/migrate/package_defaults.rs:253-261(abea6ce): A digest-unverified, non-%config file means the image wins. For policy.35 this silently drops the admin's setsebool -P booleans, semodule modules and semanage port/login changes. Its file_contexts siblings come from the host (see above), so /etc/selinux ends up mixed. Treat /etc/selinux/<type> as one unit and warn about local customizations. - risky · logic —
crates/lib/src/install/migrate.rs:1382-1399(abea6ce): Entries left to the image keep the image's numeric uid/gid, but the host's passwd/group are carried. For dynamic system groups (sssd and chrony on RHEL 10) the ids often differ, so for example /etc/sssd root:sssd would point at the wrong group. Remap by name after the merge. - look-closely · logic —
crates/lib/src/install/migrate.rs:120-157(ca04e67): The dry run stands in for the real merge: the container's /etc in place of the deployment's /usr/etc and /etc, with podman's bind-mounted hosts/resolv.conf/hostname. Only dir-versus-file conflicts are unmergable, so that is close enough. With --source-imgref it is skipped, and a conflict still fails after /boot is wiped. - look-closely · logic —
crates/lib/src/install/migrate/accounts.rs:185-253(f283587): Only whole missing entries are appended. Members of groups both sides have (e.g. a Containerfile useradd -G wheel) are not merged. A same-name, different-id account is only reported, and files the image owns under its id keep that id. - note · error-handling —
crates/lib/src/install/migrate/accounts.rs:119-129(f283587): passwd and shadow are rewritten in place with truncate+write in what is by then the only bootable deployment. Writing a temporary file and renaming it, keeping mode/owner/label, would avoid a torn file. - note · test-gap —
crates/lib/src/install/migrate/package_defaults.rs:230-238(abea6ce): The ACL and security.capability checks have no unit test.
Safe to skim
crates/lib/src/cli.rs: Help text onlycrates/etc-merge/src/lib.rs: Small filter API with a testdocs/src/man/bootc-install-to-existing-root.8.md: Docs for the new behaviourtmt/plans/package-mode.fmf: Test image tweak
45cede0 to
4011430
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head 4011430d58: 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.
Reworks --merge-etc from bootc-dev#2384. The baseline is now the host's RPM database: host /etc entries that rpm -V accepts are left to the image. %ghost files and %config files whose content RPM does not verify stay the host's. A named list of generated content (ld.so.cache, udev/hwdb.bin, everything under selinux/<type>/ except file_contexts.local) is always left to the image. The merge is dry-run against a copy of the installer's /etc before the install (skipped with --source-imgref). The image's users, groups and group members are added to the carried account databases with atomic writes. This head fails the Makefile clippy gate (str::len at accounts.rs:192), so CI would fail. Smaller risks: the SELinux rule drops admin-edited %config files under selinux/<type>/, and an SELINUXTYPE the image lacks is not refused. Group members are matched by name only. The temporary shadow copy is written before its mode is set. Still open by design: image-won entries keep the image's numeric ids, ca-trust extracted bundles are kept from the host, and --source-imgref skips the dry run.
Hotspots
- risky · api —
crates/lib/src/install/migrate/accounts.rs:192(acbf8f3): merged.len() is a disallowed method under the Makefile clippy gate (-D clippy::disallowed-methods), so CI fails. Use as_bytes().len() or String::new(). - look-closely · security —
crates/lib/src/install/migrate/accounts.rs:138-168(acbf8f3): The temporary file is created with the default mode and filled before set_permissions runs. For shadow/gshadow the hashes sit for a moment in a file other users can read, inside the deployment directory of a still-running host. Create it with mode 0600 or set permissions before writing. - look-closely · security —
crates/lib/src/install/migrate/accounts.rs:174-216(acbf8f3): Image group members are added by name, even when that name on the host is a different, unrelated account (same name, different uid), or an image user that was skipped. An image wheel member would then give the host's same-named user wheel. Add only members appended in this run, or ones whose uid matches. - look-closely · logic —
crates/lib/src/install/migrate.rs:1409-1422(bb5145a): Everything under selinux/ except config, semanage.conf and file_contexts.local is left to the image, including admin-edited %config files with a verified digest (e.g. setrans.conf). Policy directories the image lacks are dropped too, while selinux/config is carried, so a host on SELINUXTYPE=mls with a targeted-only image would boot without a policy. Refuse that case in the preflight. - note · logic —
crates/lib/src/install/migrate.rs:120-157(aeb9ab3): The dry run uses the installer container's /etc in place of the deployment's, which is close enough for dir-versus-file conflicts. With --source-imgref it is skipped, and a conflict still fails after /boot is wiped (an open design item). - note · logic —
crates/lib/src/install/migrate/package_defaults.rs:218-266(bb5145a): Entries left to the image keep the image's numeric uid/gid, while the host's passwd/group are carried. For dynamically allocated groups such as sssd (/etc/sssd root:sssd) they can differ. This is open by design, and the accounts pass only reports it.
Safe to skim
crates/lib/src/cli.rs: Help text onlycrates/etc-merge/src/lib.rs: Small filter API with a testdocs/src/man/bootc-install-to-existing-root.8.md: Docs for the new behaviourtmt/plans/package-mode.fmf: Test image tweak
4e068e0 to
8c44994
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head 8c44994421: 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.
Reworks --merge-etc from bootc-dev#2384. The baseline is now the host's RPM database. Host /etc entries that rpm -V accepts are left to the image. Package files the host edited, unowned files, %ghost files and unverified %config files are carried. Generated content (ld.so.cache, hwdb.bin, the SELinux store under selinux/<type>/, except edited package files and file_contexts.local) is left to the image. The merge refuses an SELINUXTYPE the image lacks. It is dry-run against a copy of the installer's /etc before the install (skipped with --source-imgref). The image's accounts and the memberships of same-name, same-uid users are added, with atomic 0600-first writes. This round's fixes look correct and every commit passes the clippy gate. Remaining risk is the documented open items: account id drift (image-won entries keep image ids), fstab / and /boot entries, ca-trust bundles kept from the host, and --source-imgref skipping the dry run. In that last case the SELINUXTYPE check also runs only after merge() has written to the deployment's /etc.
Hotspots
- look-closely · logic —
crates/lib/src/install/migrate/package_defaults.rs:218-306(e5f92c4): This classification decides what the image overwrites. Read it with the RPM flag semantics in mind: %ghost is always the host's, a digest-unverified non-%config file is the image's, and an edited, digest-verified file is the host's. - note · error-handling —
crates/lib/src/install/migrate.rs:1366(e5f92c4): The SELINUXTYPE check runs after merge() has written to the new /etc. Normally the dry run catches it first, but with --source-imgref it fails after the wipe with /etc already half-merged. It could run before merge(), using the carried host config. - note · logic —
crates/lib/src/install/migrate.rs:1437-1455(e5f92c4): A hand-picked list of generated content left to the image. New generated %ghost files elsewhere would still be carried from the host. - note · security —
crates/lib/src/install/migrate/accounts.rs:138-176(9856f32): The temporary file is created 0600, then owner, label and mode are set before writing, then it is fsynced and renamed. That is the right order for shadow. - note · logic —
crates/lib/src/install/migrate/accounts.rs:360-400(9856f32): Image group memberships go only to users with the same name and uid on both sides. Account id drift between host and image is otherwise only reported (an open design item).
Safe to skim
crates/lib/src/cli.rs: Help text onlycrates/etc-merge/src/lib.rs: Small filter API with a testdocs/src/man/bootc-install-to-existing-root.8.md: Docs for the new behaviourtmt/plans/package-mode.fmf: Test image tweak
8c44994 to
fbb1d13
Compare
A package-mode host has no pristine /etc, and using the new image's /usr/etc in its place makes every difference between the host's package versions and the image's look like an admin edit. In a VM migration that carried the host's PAM and sshd defaults, crypto-policies and its compiled SELinux policy (so the image's SELinux module wasn't loaded), and left those files marked modified for every later update. It also failed on Fedora Cloud's /etc/ssl/certs symlink, which the tmt test deleted from its image to get past. The RPM database records what each package installed, which is the baseline the host actually has. Host entries that still match it (as rpm -V would check them) are dropped from the diff, so the image's version wins; edited package files, files no package owns and %ghost files (machine-id, SSH host keys, hostname) are still carried. Files whose content RPM doesn't verify count as package content too (the compiled policy), unless they are %config files, whose content is the admin's (fstab, passwd). Caches and the SELinux policy store are generated from what is installed, but RPM marks them %ghost or unverified, so they are named explicitly and left to the image: carrying the host's file_contexts.bin next to the image's policy would label files from the host's policy. SELinux customizations made with semanage are not carried; the man page says so. Files there that no package owns, or package files whose content or mode the admin changed (setrans.conf, say), are still carried, and a host selinux/config naming a policy type the image lacks is refused. The database is read before the install starts, and a host without one is refused, since the old behaviour is what we'd fall back to. Generated-by: AI
The merge ran after /boot was wiped and bootupd had installed the new bootloader, and never looked at the diff's unmergable paths. In a VM a host symlink where the image has a directory failed the merge partway, and the host rebooted into a half-merged /etc with no package-mode entry left to go back to. Now the merge is first run against a scratch copy of the image's /etc (the installer container's own, which is what gets deployed) before the install starts, and conflicts are reported and refused there. The real merge checks the unmergable paths again before changing anything, like the composefs upgrade path does. The scratch copy lives on the host's filesystem, which takes the host files' SELinux labels; the container's may not. With --source-imgref the installer's /etc isn't the image being installed, so the dry run is skipped and only the real merge's check remains. Generated-by: AI
fbb1d13 to
9a14fb4
Compare
useradd and package scriptlets change /etc/passwd and friends on every host, so the merge always carries the host's account databases, and the accounts the image defines are lost: in a VM migration the user and group the image's Containerfile added were gone after reboot. systemd-sysusers only recreates accounts declared in sysusers.d, and with whatever id is free, not the one the image's files use. So image entries the host lacks are appended to passwd and group (and their shadow entries), unless the host already uses their id or, for a user, its primary group id names a different group on the host; those are reported instead, as are names whose id differs from the image's. For groups both sides have, the members the image lists are added, so a user the image puts in wheel stays there, as long as the member is the same account (name and uid) on both sides; appended groups go through the same check. The files are replaced atomically (the temporary file starts out 0600), keeping their mode, owner and label, since by then this is the only deployment the host can boot. The host's nsswitch.conf (an authselect %ghost file) is carried too, and it doesn't list nss-altfiles, which fedora-bootc uses for its system accounts in /usr/lib/passwd; in the VM avahi-daemon then failed to find its user. So when the merged nsswitch.conf doesn't use altfiles, the image's /usr/lib accounts are added to /etc as well. Generated-by: AI
--preserve-var stopped writing a package-mode rollback entry when that code was removed, but the help still promised one, which is the thing a user would count on if the migration went wrong. Generated-by: AI
9a14fb4 to
d6c2893
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
Review guide for head d6c2893ed2: 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.
Reworks --merge-etc from bootc-dev#2384. The baseline is now the host's RPM database. Host /etc entries that rpm -V accepts (digest, mode, owner, ACL, caps, and size where verified) are left to the image. Edited package files, unowned files, %ghost files and unverified %config files are carried. Generated content (ld.so.cache, hwdb.bin, package-owned, unedited files under selinux/<type>/) is left to the image. An SELINUXTYPE the image lacks is refused before anything is written. The merge is dry-run against a copy of the installer's /etc before the install (skipped with --source-imgref). The image's accounts are appended with empty member and admin lists, then group members and gshadow admins are added only for users with the same name and uid on both sides, using atomic 0600-first writes. The reviewer found no remaining code issues. Open design items, documented in the PR: system-account id drift (entries the image wins keep the image's ids), the host fstab's / and /boot entries, ca-trust bundles kept from the host, and --source-imgref skipping the dry run.
Hotspots
- look-closely · logic —
crates/lib/src/install/migrate/package_defaults.rs:253-336(9800857): This classification decides what the image overwrites. %ghost is always the host's. A digest-unverified non-%config file is the image's unless its verified size differs. A changed digest or mode makes a file an edit, which is carried. - note · error-handling —
crates/lib/src/install/migrate.rs:1365-1375(9800857): The SELINUXTYPE check now runs before merge(). It reads the host's config when that is carried, so a refusal leaves the new /etc untouched, even with --source-imgref. - note · logic —
crates/lib/src/install/migrate.rs:1393-1469(9800857): A hand-picked list of generated content left to the image. Generated %ghost files outside that list are still carried from the host. - note · security —
crates/lib/src/install/migrate/accounts.rs:360-440(b99ba15): Appended groups start with empty members and admins. The member pass then adds only same-name, same-uid users, so an unrelated host account with an image member's name gets no privileges.
Safe to skim
crates/lib/src/cli.rs: Help text onlycrates/etc-merge/src/lib.rs: Small filter API with a testdocs/src/man/bootc-install-to-existing-root.8.md: Docs for the new behaviourtmt/plans/package-mode.fmf: Test image tweak
Fixes for the
--merge-etchalf of bootc-dev#2384, on top of ckyrouac'simage-to-packagehead 6ea15fe, which is pushed unchanged here as the baseckyrouac/image-to-package-base. Meant to be submitted to ckyrouac'simage-to-packagebranch once jmarrero has reviewed it; nothing here is for bootc-dev/bootc directly. The--preserve-varfixes target bootc-dev#2586, where that code now lives: #11./usr/etcas the baseline, every difference between the host's package versions and the image's was carried as an admin edit. Host entries that still match their package (asrpm -Vchecks them) are now left to the image, as are caches and the compiled SELinux policy store; edited package files, unowned files,%ghostfiles and unverified%configfiles (fstab, passwd) are still carried. A host without an RPM database is refused. The tmt image no longer needsrm -rf /etc/ssl/certs./etcbefore the install (skipped with--source-imgref), and unmergable paths are refused rather than failing halfway through the real merge./usr/lib/passwdwhen the carriednsswitch.confdoesn't use altfiles), unless their ids are taken.to-existing-roothelp; that code is gone. (The PR description still mentions it.)When this is rebased onto bootc-dev#2586,
require_ostree_backendthere should also cover--merge-etc, so a composefs-native image is refused before the install for this flag too.Testing, on a 16-core devspace (jmarrero-devspace-42195773969):
cargo test -p bootc-lib --lib -- install::(65 passed) andcargo test -p etc-merge(8 passed),cargo fmt --check, the Makefile's clippy gate andcargo xtask update-generated direct --checkpass; every commit builds./etcedits (sshd_config, an sshd drop-in, sysctl, motd, a local user and group), PostgreSQL and a custom service, migrated withpodman run ... bootc install to-existing-root --merge-etcto a fedora-bootc:44 image (plus postgresql-server and a user/group). With install: Add --preserve-var and --merge-etc to to-existing-root bootc-dev/bootc#2384's code the merge failed after the install onssl/certs, leaving a half-merged/etc; the image's user and group were gone and avahi-daemon failed; policy.35, semanage.conf and crypto-policies were the host's. With this branch the install succeeds, admin edits are kept, pam/sshd defaults, policy.35, semanage.conf and ld.so.cache are the image's, the image's user, group and the altfiles accounts resolve, postgres and the service run, andostree admin config-difflists 21 modified entries (34 before).just test-tmt-package-mode, test-50-to-existing-root-preserve-var-merge-etc, Fedora 44 guest, without the oldrm -rf /etc/ssl/certsand with a check for the image's user) passed at fbb1d13. The VM run above was at 4e068e0. Changes since (the SELinux policy-type check moved before the merge, appended groups go through the member check, unowned or edited SELinux files are carried, size and mode checks) are covered by unit tests only.Open points, not built here:
/usrtoo) map to other names;polkitdwasn't added because its group id is taken. This needs a decision on whose ids win./and/bootentries;systemd-remount-fsfails after migration.update-ca-trust.semanage/setsebool -Pcustomizations aren't carried (documented)./rootisn't migrated; non-RPM hosts would need a dpkg equivalent.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, baseimage-to-package(install: Add --preserve-var and --merge-etc to to-existing-root bootc-dev/bootc#2384); 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