WIP: Add support for `--replace-mode=alongside` for ostree target #137

cgwalters · 2023-10-02T17:59:02Z

Ironically our support for --replace-mode=alongside breaks when we're targeting an already extant ostree host, because when we first blow away the /boot directory, this means the ostree stack loses its knowledge that we're in a booted deployment, and will attempt to GC it...

ostreedev/ostree-rs-ext@8fa019b is a key part of the fix for that.

However, a notable improvement we can do here is to grow this whole thing into a real "factory reset" mode, and this will be a compelling answer to
coreos/fedora-coreos-tracker#399

To implement this though we need to support configuring the stateroot and not just hardcode default.

openshift-ci · 2023-10-02T17:59:06Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

omertuc · 2024-09-18T08:01:35Z

Sorry @cgwalters , accidentally pushed the rebase to your fork instead of mine

~~EDIT: undid it, continuing my rebase efforts on https://github.com/omertuc/bootc/tree/137clone~~

EDIT2: Continuing here

cgwalters · 2024-09-18T12:26:53Z

I think you can just take over this PR too if you want, or open a new PR from your fork - either way.

omertuc · 2024-09-19T15:37:24Z

Rebased. Without any changes, I'm facing an issue where in an ostree system, the mounted / on the host system is an overlay (-v mounted into /:/target) and so the findmnt source for it is overlay rather than a /dev/... and so it trips up lsblk later on

I'll see how I can tweak it so that it finds the right device

omertuc · 2024-10-02T16:45:46Z

With additional -v /sysroot:/target -v /sysroot:/target/sysroot mounts instead of -v /:/target and --stateroot foo, this seems to work

omertuc · 2024-10-02T16:47:52Z

@cgwalters thoughts on the above mounts? Do we want to require them for install on ostree targets, or should I figure out a way to make this work without them, using just the already-documented install mounts (i.e. /:/target)?

cgwalters · 2024-10-02T18:23:57Z

and so the findmnt source for it is overlay rather than a /dev/... and so it trips up lsblk later on

We should learn how to peel that. This is really the same thing as https://bugzilla.redhat.com/show_bug.cgi?id=2308594 and ostreedev/ostree#3198 and containers/composefs#280

Short term the simplest is the same logic as the grub patch - detect overlayfs for / and check if /sysroot exists and is mounted, if so use that.

omertuc · 2024-10-08T20:30:55Z

and so the findmnt source for it is overlay rather than a /dev/... and so it trips up lsblk later on

We should learn how to peel that. This is really the same thing as bugzilla.redhat.com/show_bug.cgi?id=2308594 and ostreedev/ostree#3198 and containers/composefs#280

Short term the simplest is the same logic as the grub patch - detect overlayfs for / and check if /sysroot exists and is mounted, if so use that.

OK. Changed it so that when the target rootfs is an overlay, we'll implicitly try targetting <original_target>/sysroot instead.

It wasn't working at first and was a bit of a headache for me to debug because apparently if you mount /:/target then inside the container /target/sysroot is read-only by default, and so ensure_dir_labeled was failing, as opposed to when you mount /sysroot:/target directly, in which case it's not read-only. Took me a while to track that down chasing red herrings, and I'm still not sure who's responsible for this behavior (kernel? podman?), but after I realized it I simply moved ensure_dir_label to run only after your added let _ = crate::utils::open_dir_remount_rw... and then the rest just worked.

Current code might need a bit of touch-ups, but do you think the direction of the code in its current state is good? Should I clean it up and undraft?

cgwalters · 2024-10-11T18:02:06Z

It wasn't working at first and was a bit of a headache for me to debug because apparently if you mount /:/target then inside the container /target/sysroot is read-only by default, and so ensure_dir_labeled was failing, as opposed to when you mount /sysroot:/target directly, in which case it's not read-only.

I think that's possibly because it's bootc that's special casing mounting /sysroot read-write - that's how we do it outside of a container at least.

cgwalters

Thanks for picking this up!!

cgwalters · 2024-10-11T18:03:12Z

lib/src/install.rs

@@ -1336,6 +1352,19 @@ async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Re
        .ok_or_else(|| anyhow!("No uuid for boot/root"))?;
    tracing::debug!("boot uuid={boot_uuid}");

+    // If we're doing an alongside install, then the /dev bootupd sees needs to be the host's.


I think we can drop this code now and hard require -v /dev:/dev

I see we already require it documentation-wise.

And we also have it as an assertion here when installing to disk with --via-loopback

I've now also added it as an assertion at this code location as well. However since I don't fully understand the motivation for it, I'm not sure if the assertion should only occur in case of ReplaceMode::Alongside like the comment mentions, or generally

What I'd actually hoped to do is switch to doing dynamic mounts: #826

But in the short term yeah that's fine to have an assertion here.

lib/src/install.rs

lib/src/lsm.rs

lib/src/utils.rs

Ironically our support for `--replace-mode=alongside` breaks when we're targeting an already extant ostree host, because when we first blow away the `/boot` directory, this means the ostree stack loses its knowledge that we're in a booted deployment, and will attempt to GC it... ostreedev/ostree-rs-ext@8fa019b is a key part of the fix for that. However, a notable improvement we can do here is to grow this whole thing into a real "factory reset" mode, and this will be a compelling answer to coreos/fedora-coreos-tracker#399 To implement this though we need to support configuring the stateroot and not just hardcode `default`. Signed-off-by: Omer Tuchfeld <[email protected]>

cgwalters · 2024-10-15T17:08:54Z

lib/src/utils.rs

@@ -55,6 +58,21 @@ pub(crate) fn find_mount_option<'a>(
        .next()
 }

+/// Given a target directory, if it's a read-only mount, then remount it writable
+#[context("Opening {target} with writable mount")]
+pub(crate) fn open_dir_remount_rw(root: &Dir, target: &Utf8Path) -> Result<Dir> {


This needs a #[cfg(feature = install)] looks like

openshift-ci bot added the do-not-merge/work-in-progress label Oct 2, 2023

cgwalters force-pushed the install-existing-ostree branch from 86f6038 to 22671b5 Compare October 2, 2023 17:59

cgwalters mentioned this pull request Oct 2, 2023

Add factory reset capability coreos/fedora-coreos-tracker#399

Open

cgwalters mentioned this pull request Nov 9, 2023

Add support for bootc install --with-overlay quay.io/exampleuser/someoverlay:latest #190

Open

cgwalters mentioned this pull request Mar 19, 2024

install: Support reinstallation (factory reset) #404

Open

This was referenced Sep 13, 2024

bootc install to-filesystem does not check for mutable /usr before performing destructive operations #787

Open

bootc switch --stateroot flag #617

Open

omertuc force-pushed the install-existing-ostree branch from 22671b5 to e4f00af Compare September 18, 2024 08:00

github-actions bot added the area/install Issues related to `bootc install` label Sep 18, 2024

omertuc force-pushed the install-existing-ostree branch from e4f00af to 22671b5 Compare September 18, 2024 08:02

omertuc force-pushed the install-existing-ostree branch from 22671b5 to e4f00af Compare September 19, 2024 15:31

omertuc force-pushed the install-existing-ostree branch from e4f00af to 923b0ed Compare October 8, 2024 20:25

cgwalters commented Oct 11, 2024

View reviewed changes

omertuc force-pushed the install-existing-ostree branch 3 times, most recently from 5775a41 to 3561a64 Compare October 14, 2024 17:21

omertuc force-pushed the install-existing-ostree branch from 3561a64 to ed94f1e Compare October 14, 2024 17:35

cgwalters commented Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add support for `--replace-mode=alongside` for ostree target #137

WIP: Add support for `--replace-mode=alongside` for ostree target #137

cgwalters commented Oct 2, 2023

openshift-ci bot commented Oct 2, 2023

omertuc commented Sep 18, 2024 •

edited

Loading

cgwalters commented Sep 18, 2024

omertuc commented Sep 19, 2024 •

edited

Loading

omertuc commented Oct 2, 2024 •

edited

Loading

omertuc commented Oct 2, 2024

cgwalters commented Oct 2, 2024

omertuc commented Oct 8, 2024 •

edited

Loading

cgwalters commented Oct 11, 2024

cgwalters left a comment

cgwalters Oct 11, 2024

omertuc Oct 14, 2024 •

edited

Loading

cgwalters Oct 14, 2024

cgwalters Oct 15, 2024

WIP: Add support for --replace-mode=alongside for ostree target #137

Are you sure you want to change the base?

WIP: Add support for --replace-mode=alongside for ostree target #137

Conversation

cgwalters commented Oct 2, 2023

openshift-ci bot commented Oct 2, 2023

omertuc commented Sep 18, 2024 • edited Loading

cgwalters commented Sep 18, 2024

omertuc commented Sep 19, 2024 • edited Loading

omertuc commented Oct 2, 2024 • edited Loading

omertuc commented Oct 2, 2024

cgwalters commented Oct 2, 2024

omertuc commented Oct 8, 2024 • edited Loading

cgwalters commented Oct 11, 2024

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Oct 11, 2024

Choose a reason for hiding this comment

omertuc Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

cgwalters Oct 14, 2024

Choose a reason for hiding this comment

cgwalters Oct 15, 2024

Choose a reason for hiding this comment

WIP: Add support for `--replace-mode=alongside` for ostree target #137

WIP: Add support for `--replace-mode=alongside` for ostree target #137

omertuc commented Sep 18, 2024 •

edited

Loading

omertuc commented Sep 19, 2024 •

edited

Loading

omertuc commented Oct 2, 2024 •

edited

Loading

omertuc commented Oct 8, 2024 •

edited

Loading

omertuc Oct 14, 2024 •

edited

Loading