Repository navigation
Conversation
| // "a path led outside of the filesystem". Warn and skip rather than aborting the | ||
| // whole merge; the image's existing content wins for that path. | ||
| if let Err(ref e) = copy_result { | ||
| if e.to_string().contains("a path led outside of the filesystem") { |
There was a problem hiding this comment.
Let's avoid string matching errors, we can use downcasts
But perhaps better we should be using https://docs.rs/cap-std-ext/5.1.2/cap_std_ext/struct.RootDir.html ? And yes we'd need a copy api there.
Perhaps simplest to open the parent dir rooted, then do a copy that way
There was a problem hiding this comment.
Status at e04ef8a: the string match is gone, but merge_leaf still uses cap-std Dir::copy and not RootDir, so a host path through an absolute symlink now fails the merge (after install) instead of being skipped. The /etc/ssl/certs failure the test works around is a different case (host symlink, image directory).
Generated-by: https://github.com/jmarrero-bot#llms
| //! When `--preserve-var` is passed, the running kernel and initramfs are saved | ||
| //! to `<root_path>/var/lib/pkgmode-rollback/` **before** the install wipes | ||
| //! `/boot`. After the install a third BLS entry (`pkgmode-rollback.conf`) is | ||
| //! written into the active `loader.N/entries/` directory so GRUB presents a | ||
| //! "Previous OS" option. The entry is invisible to ostree (which only reads | ||
| //! `ostree-*.conf` files) and to bootupd (which does not touch `loader/entries/`). |
There was a problem hiding this comment.
This is kind of hacky, I think it'd be cleaner to just not delete the entries (and kernel/initramfs) at all which should be a separate option.
| .context("Locating new ostree deployment directory")?; | ||
| println!(" Deployment directory: {}", deploy_dir.display()); | ||
|
|
||
| // The deployment's var/ is two levels up from the deploy dir: |
There was a problem hiding this comment.
Partly addressed: the deployment path now comes from install_with_sysroot. The physical root is still guessed with root_path.join("sysroot/ostree").exists() (install.rs:3075, 3092), and deployment_var_path() walks two parents up from the deploy dir instead of using the deployment's stateroot.
Generated-by: https://github.com/jmarrero-bot#llms
| /// We enumerate `<root_path>/ostree/deploy/` and return the most recently | ||
| /// modified entry under each stateroot's `deploy/` subdirectory. | ||
| #[context("Locating new ostree deployment directory")] | ||
| fn find_deploy_dir(root_path: &Path) -> Result<PathBuf> { |
There was a problem hiding this comment.
There's APIs for htis too
| /// `root_path` is the host root as seen from inside the install container. | ||
| /// `kver` is the running kernel version string returned by `save_pkgmode_kernel`. | ||
| #[context("Running post-install /var preservation and rollback entry")] | ||
| pub(crate) fn preserve_var_and_write_rollback(root_path: &Path, kver: &str) -> Result<()> { |
There was a problem hiding this comment.
In general let's try to use cap-std
| /// Returns true if the filesystem hosting `new_var` supports reflinks. | ||
| /// | ||
| /// Probes by attempting a zero-byte reflink from `src_var` into `new_var`. | ||
| fn reflinks_supported(src_var: &Path, new_var: &Path) -> bool { |
There was a problem hiding this comment.
Our storage code already probes this I believe
| let src_entry = src_var.join(name_str.as_ref()); | ||
| let dst_entry = new_var.join(name_str.as_ref()); | ||
|
|
||
| // For `log/`, skip only the `journal` subdirectory (large; regenerated by journald). |
There was a problem hiding this comment.
Why is this special cased?
| continue; | ||
| } | ||
|
|
||
| // For `lib/`, skip `containers` (podman/docker storage — overlay mounts cannot |
There was a problem hiding this comment.
Ditto, I don't think we should have special cases buried in this code. We should make it configurable of course in general, via e.g. --preserve-var-skip=log/,lib/containers or something
There was a problem hiding this comment.
Still open at e04ef8a: --preserve-var-skip only adds exclusions, and tmp, cache, log/journal and lib/containers stay hardcoded in preserve_var() (migrate.rs:108) with no way to turn them off. lib/containers includes podman named volumes: in a VM run the copied /var had no lib/containers, so a volume created before migrating was missing from the copy (it only showed up after reboot because the carried fstab remounted the original /var).
Generated-by: https://github.com/jmarrero-bot#llms
|
In a live chat about this the problem that We should support a config file for this functionality (or reuse existing bootc install config) that supports exclusions e.g. |
881a22a to
c0c54ca
Compare
c0c54ca to
c0d54ce
Compare
c0d54ce to
054c395
Compare
|
@ckyrouac Do you think this'll be ready for 1.18? |
yea it should be. I'll add it to 1.18. |
054c395 to
fd026db
Compare
Preserve existing-root data while providing a clear, reliable /etc merge path for migration. Assisted-by: AI Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Use directory-capability checks to preserve /var efficiently across filesystems and document migration exclusions. Assisted-by: AI Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Exercise package-mode migration flags through a focused TMT path with predictable Fedora defaults. Assisted-by: AI Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Make package-mode TMT runs work locally and in Packit by consistently resolving artifacts, playbooks, and workdirs. Assisted-by: AI Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Stream package-mode image transfers and make staging, cleanup, and version validation reliable across test execution. Assisted-by: AI Signed-off-by: ckyrouac <ckyrouac@redhat.com>
Prepare the target's boot filesystem and image state required by the package-mode boot test. Assisted-by: AI Signed-off-by: ckyrouac <ckyrouac@redhat.com>
fd026db to
e04ef8a
Compare
jmarrero-bot
left a comment
jmarrero-bot
left a comment
There was a problem hiding this comment.
These are findings from @jmarrero's review that aren't already raised in the existing threads.
Reproduced in VMs: Fedora Cloud 44 package-mode hosts (btrfs, UEFI, SELinux enforcing) with a local user and some /etc edits, migrated with --preserve-var --merge-etc to a fedora-bootc:44 image built from e04ef8a. The image adds a user and group, installs an SELinux module, and edits /etc/pam.d/login and /etc/ssh/sshd_config. Points marked (code) come from reading the code; the rest were reproduced.
Blockers
-
--merge-etcuses the new image's /usr/etc as the pristine baseline, so every difference between the host and the image is applied as if it were an admin edit (inline comments below). After reboot:- the image's user and group are gone from /etc/passwd and /etc/group (
getentfinds neither); - /etc/pam.d/login, /etc/ssh/sshd_config, crypto-policies/config, selinux/semanage.conf and selinux/targeted/policy/policy.35 are the host's copies. The image's SELinux module isn't in the loaded policy (
chcon -twith its type fails); ostree admin config-difflists 257 entries, includingMfor passwd, group, shadow, pam.d/login, ssh/sshd_config and the compiled policy, so later image updates to those files should no longer apply (from ostree's merge rules; no upgrade was run).
The test's
rm -rf /etc/ssl/certs(package-mode.fmf:111) hides one such case. Without it the merge fails on ssl/certs (a symlink on the host, a directory in the image); see 2. - the image's user and group are gone from /etc/passwd and /etc/group (
-
Both migrations run after the install has finished. The log goes bootupd,
Installation complete!, thenPreserving /var.... By then /boot has only the new ostree entry, so a failure leaves no package-mode entry to go back to. In the VMs:- Without the ssl/certs workaround, the merge failed partway. passwd, group, pam, sshd and the policy were already the host's, but subuid/subgid were still the image's (the local user is in passwd, not in subuid). The VM then rebooted into that half-merged deployment.
- With /var/log on a separate XFS filesystem, /var preservation failed mid-copy (nis, opt and spool weren't copied, and /etc wasn't merged).
merge_etcnever checksdiff.unmergable_paths. The diff printedUnmergable File 'ssl/certs' now defaults to a directory in new etcand the merge went ahead anyway. The composefs update path checks it first (update.rs:326).- With a composefs-native image (setup-root-conf.toml, no prepare-root.conf) and no
--composefs-backend, install picked composefs itself (install.rs:1866), wiped /boot, printedInstallation complete!and then failed withInstall did not produce an ostree deployment. - Not from this PR's code, but the same ordering: on an earlier attempt, with the image's logically bound images missing from host storage, to-existing-root wiped /boot (only efi/ was left) before failing on that check, leaving no boot entry. Every check that can fail should run before /boot is touched.
-
(code) The GRUB rollback entry was removed, but the PR description and cli.rs:351 still say
--preserve-varwrites one.
Should fix
- The reflink probe only checks the top-level /var pair. With /var/log on XFS,
cp --reflink=alwaysfailed withInvalid cross-device link. In the same VM,cp -a --reflink=autocopied that /var/log and still cloned on btrfs (btrfs filesystem dushows 0 exclusive). That would remove reflink.rs and one of the two copy loops. Plaincp -aalready defaults to reflink=auto in coreutils 9. - A separate /var gets copied but not used. Fedora Cloud mounts /var as a btrfs subvolume. With
--merge-etcthe host fstab comes along (A fstab), so after reboot /var is the original subvolume mounted over the deployment's var, and the copy is hidden under /sysroot/ostree/deploy/default/var.cp -aalso descends into other mounts under /var (it tried to copy the XFS /var/log above). - The
copy_dir_skip_subdirpath recreates /var/lib and /var/log without their metadata. In the copy, /var/lib was 0755var_t(host: 0711var_lib_t) and /var/log wasvar_t(host:var_log_t). - /root and /home aren't migrated. A file in /root was gone after reboot (/root is /var/roothome). /home only came back because the carried fstab mounts the home subvolume; on a host without a separate /home nothing would copy it (code: only /var is copied). The test works around /root by copying authorized_keys into /var/roothome by hand.
Tests and PR structure
- About 930 lines of test infrastructure, 507 of them in five Ansible playbooks, move one image archive from a harness VM to the target. Building the image on the target VM would remove most of that.
- Commit 2 rewrites about a third of commit 1's migrate.rs (122 lines removed, 420 added) and adds reflink.rs. Commits 4 to 6 rework the plan from commit 3 (for example, copy-test-image.yml is added in 3 and deleted in 5). Suggest splitting: etc-merge prep, /var preservation, /etc merge (after a design discussion), and the test infrastructure as its own PR.
The 7 etc-merge and 13 migrate/reflink unit tests pass at e04ef8a (SELinux-enabled build); none of the above is covered by them.
Generated-by: https://github.com/jmarrero-bot#llms
| /// C (new deployment) = `<deploy>/etc` — deploy target | ||
| /// | ||
| /// The diff A→B captures every file the admin changed relative to the image | ||
| /// defaults and applies those changes onto C. This is the same algorithm |
There was a problem hiding this comment.
This isn't what bootc upgrade does on ostree: there libostree does the merge, and etc-merge is only used by the composefs backend, whose baseline is the booted deployment's /etc. Here the new image's /usr/etc stands in for a baseline the host never had (Blocker 1).
| } | ||
| } | ||
| let preserve_var_skip = migrate::validate_exclusions(&preserve_var_skip)?; | ||
| if (preserve_var || merge_etc) && opts.composefs_opts.composefs_backend { |
There was a problem hiding this comment.
This only sees the CLI flag. install.rs:1866 can switch to the composefs backend later (UKI or composefs-native image). In the VM that wiped /boot, printed Installation complete! and then failed with Install did not produce an ostree deployment.
| }; | ||
|
|
||
| install_to_filesystem(opts, true, cleanup).await | ||
| let deployment_path = install_to_filesystem(fs_opts, true, cleanup).await?; |
There was a problem hiding this comment.
Both migrations run after this returns, so after /boot is wiped and bootupd has run. Reproduced a half-merged /etc and a partial /var, each left as the only boot entry (Blocker 2).
|
|
||
| let new_tree = new_tree_opt.unwrap_or_else(|| FileSystem::new(Stat::uninitialized())); | ||
|
|
||
| let diff = compute_diff_without_deletions(&pristine_tree, ¤t_tree, &new_tree) |
There was a problem hiding this comment.
diff.unmergable_paths isn't checked before merge(). In the VM it held ssl/certs and the merge failed partway. update.rs:326 checks it first.
| .collect::<Vec<_>>(); | ||
| effective_exclusions.extend(exclusions.iter().cloned()); | ||
|
|
||
| if supports_reflink(&src_var, &new_var)? { |
There was a problem hiding this comment.
The probe covers only the top-level /var pair. With /var/log on XFS this failed with Invalid cross-device link; cp -a --reflink=auto handled the same tree and still cloned on btrfs.
| ) -> Result<()> { | ||
| let src = open_child_dir_nofollow(src_root, dir_name) | ||
| .with_context(|| format!("Opening /var/{}", dir_name.to_string_lossy()))?; | ||
| let dst = open_or_create_child_dir(dst_root, dir_name)?; |
There was a problem hiding this comment.
The directory is created fresh, so its mode and label are lost: in the VM copy /var/lib was 0755 var_t (host 0711 var_lib_t), /var/log var_t (host var_log_t).
| /// need to be cleaned up if desired when rebooted into the new root. | ||
| /// | ||
| /// When migrating from a package-mode system, use `--preserve-var` to copy `/var` data | ||
| /// into the new deployment and write a GRUB rollback entry, and `--merge-etc` to carry |
There was a problem hiding this comment.
No GRUB rollback entry is written anymore; this line and the PR description still say so.
| cat > "$workdir/Containerfile" <<'EOF' | ||
| FROM localhost/bootc | ||
| # Fedora Cloud and fedora-bootc have different /etc/ssl/certs baselines. | ||
| RUN rm -rf /etc/ssl/certs && \ |
There was a problem hiding this comment.
Without this the merge fails on Fedora Cloud 44: the host's /etc/ssl/certs is a symlink to /etc/pki/tls/certs and the image has a directory. A real migration hits it too.
| # TMT connects as root. Package-mode hosts keep root's home at /root, | ||
| # while bootc images link /root to /var/roothome, so preserve its key | ||
| # under /var for TMT to reconnect after the migration reboot. | ||
| install -d -m 0700 /var/roothome/.ssh |
There was a problem hiding this comment.
This works around /root not being migrated: without it root's keys and files in /root are gone after reboot (reproduced with a marker file).
This branch has not been deployed
4 participants
This is AI generated with lots of guidance.
Add two flags to
bootc install to-existing-rootthat are most usefulwhen migrating a package-mode system to a bootc image built externally:
--preserve-var Copy the running system's /var data into the new
deployment, and write a package-mode GRUB rollback
boot entry.
--merge-etc Apply the running system's /etc customisations onto
the new deployment via a 3-way merge.
Typical usage:
podman run --rm --privileged --pid=host --user=root:root
-v /dev:/dev --security-opt label=type:unconfined_t
-v /:/target -v /var/lib/containers:/var/lib/containers
bootc install to-existing-root
--preserve-var
--merge-etc
--acknowledge-destructive