Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libostree/deploy: enable composefs by default #3353

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Dec 11, 2024

The composefs libostree integration has been supported for a while now
and is actively in use in various ostree/bootc-based systems. Let's
turn it on by default.

This has no effect if composefs support is not compiled in. Note also
that this does not change the default value of the composefs.enabled
tristate to true. The default is still maybe, but the deploy API
will now also create composefs images for maybe.

The reason for doing it this way is so that systems upgrading from
old libostree versions (which may either not have composefs support or
may have composefs-related bugs) will still be able to upgrade and not
trip ostree-prepare-root in the new deployment (which allows missing
composefs images for maybe).

We may in the future change the default value to true.

See also: #2867

@jlebon
Copy link
Member Author

jlebon commented Dec 11, 2024

Haven't tested this yet.

@jlebon
Copy link
Member Author

jlebon commented Dec 12, 2024

OK so after looking at this a bit more, this doesn't quite do what I thought. I was planning to use enabled = maybe in the config file, but maybe is already the default value if it's missing. So this has a much larger impact; i.e. all libostree clients with composefs compiled in and no config specified would now start creating and mounting composefs images. Which... maybe it has baked enough now? But we'd need to raise awareness.

If we don't think we're ready for that, then we might need another config knob to express what we want here.

@jlebon
Copy link
Member Author

jlebon commented Dec 12, 2024

all libostree clients with composefs compiled in and no config specified would now start creating and mounting composefs images. Which... maybe it has baked enough now?

I think one thing that helps here is the "with composefs" part. If you're compiling libostree today with composefs enabled, then you're already tracking this tech and possibly using it already. Everyone else will not really be affected.

@cgwalters
Copy link
Member

If you're compiling libostree today with composefs enabled, then you're already tracking this tech and possibly using it already.

Maybe. I think it's probably fine to do that change and release note it heavily, but it is a semantic change.

Without this, it's hard to have new nodes use composefs while still allowing old nodes with ancient libostree to upgrade to the same image (without composefs).

That said, how does patching the current ostree help at all for this problem? Unless we back port this patch to y-stream branches...but I dunno, that seems like a big task to do.

I think we need to investigate instead fixing this up post-deploy pre-reboot via code execution which we're already tracking in other venues (e.g. the discussions of using podman run <target image> bootc install) which already give us a way to ensure we're running the new OS image.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as requested changes just to signal it's reviewed

@cgwalters
Copy link
Member

If we were to try to cherry pick this patch to older releases, it'd definitely also want a6d07b6 for example

@cgwalters cgwalters added the area/composefs Issues related to composefs label Dec 12, 2024
@jlebon
Copy link
Member Author

jlebon commented Dec 13, 2024

If you're compiling libostree today with composefs enabled, then you're already tracking this tech and possibly using it already.

Maybe. I think it's probably fine to do that change and release note it heavily, but it is a semantic change.

OK cool, I'll test it in earnest. Definitely at least some tests need to be adjusted it seems.

Without this, it's hard to have new nodes use composefs while still allowing old nodes with ancient libostree to upgrade to the same image (without composefs).

That said, how does patching the current ostree help at all for this problem? Unless we back port this patch to y-stream branches...but I dunno, that seems like a big task to do.

Backporting is not necessary.

With this patch, the node eventually converges to composefs enabled. The first upgrade to an image which has this patch will not have composefs. The second upgrade starting from this patch and onward will.

I think we need to investigate instead fixing this up post-deploy pre-reboot via code execution

For the OCP/OKD case, we clearly need to eventually be able to just assume that composefs is on to unblock other feature work that rely on it. There's indeed workarounds we could do (though actually with this patch, one workaround is just redeploy the same commit after updating) though we're also close I think to be able to addressing the old bootimages problem which would just solve... so much. At least until then, we can get the hardening benefits of composefs.

@jlebon jlebon force-pushed the pr/composefs-maybe branch from debd2f4 to 9a9eaf7 Compare December 13, 2024 02:29
@jlebon jlebon changed the title libostree/deploy: create composefs image even if maybe libostree/deploy: enable composefs by default Dec 13, 2024
@jlebon
Copy link
Member Author

jlebon commented Dec 13, 2024

Tweaked the tests and added more colour to the commit message! Still want to sanity-check this in e.g. c9s.

@jlebon jlebon force-pushed the pr/composefs-maybe branch from 9a9eaf7 to 7daeaa2 Compare December 13, 2024 02:55
@travier
Copy link
Member

travier commented Dec 13, 2024

With this patch, the node eventually converges to composefs enabled. The first upgrade to an image which has this patch will not have composefs. The second upgrade starting from this patch and onward will.

If I understand correctly, this means that we do not explicitly enable composefs on RHCOS yet but any system that updates using an ostree with this code will start using composefs?

@travier
Copy link
Member

travier commented Dec 13, 2024

Hum, I'm just realizing that we might need this for the Atomic Desktops as well and now I don't understand anymore how we could make it work in Fedora CoreOS without this 😕

@cgwalters
Copy link
Member

The more I think about this the more I lean towards trying to cleanly split "generate" vs "consume".

Today composefs = maybe is not really different from composefs = no right? So maybe the cleanest is the default for composefs is set to no in the code, but if composefs is compiled in we default to generating a composefs unconditionally.

@jlebon
Copy link
Member Author

jlebon commented Dec 13, 2024

If I understand correctly, this means that we do not explicitly enable composefs on RHCOS yet but any system that updates using an ostree with this code will start using composefs?

Yeah, exactly. RHCOS would ship with enabled = maybe for now.

Hum, I'm just realizing that we might need this for the Atomic Desktops as well and now I don't understand anymore how we could make it work in Fedora CoreOS without this 😕

It worked in FCOS because the previous version has new enough libostree. FCOS has periodic barriers. Upgrade tests would've hit this if it weren't covered by one of the existing barriers. It should equally work in the Atomic Desktops given that we require that nodes go through every major at least once.

So maybe the cleanest is the default for composefs is set to no in the code, but if composefs is compiled in we default to generating a composefs unconditionally.

I'm OK with that. If we do that, then we should also make the deploy logic match the ostree-prepare-root logic where we always parse the config and correctly error out if composefs is not compiled in but is enforced.

@jlebon
Copy link
Member Author

jlebon commented Dec 13, 2024

If we do that, then we should also make the deploy logic match the ostree-prepare-root logic where we always parse the config and correctly error out if composefs is not compiled in but is enforced.

Hmm, based on

// We always parse the composefs config, because we want to detect and error
// out if it's enabled, but not supported at compile time.
I think that was an unintended gap in the current code. Will fix!

@jlebon jlebon force-pushed the pr/composefs-maybe branch from 7daeaa2 to 0643544 Compare December 13, 2024 16:50
@jlebon
Copy link
Member Author

jlebon commented Dec 13, 2024

Updated!

@cgwalters
Copy link
Member

But won't this cause the opposite problem where we will just bomb out on upgrades because the new prepare-root won't find a composefs for systems that didn't have it configured on previously?

@jlebon
Copy link
Member Author

jlebon commented Dec 13, 2024

But won't this cause the opposite problem where we will just bomb out on upgrades because the new prepare-root won't find a composefs for systems that didn't have it configured on previously?

I think you're right.

So... I think we either have to leave the default on maybe (but still change the deploy threshold down to maybe), or add a new knob to control the deploy and ostree-prepare-root behaviours separately?

@cgwalters
Copy link
Member

cgwalters commented Dec 13, 2024

Wouldn't my previous suggestion work?

@jlebon
Copy link
Member Author

jlebon commented Dec 13, 2024

Wouldn't my previous suggestion work?

Gotcha, I had misunderstood your suggestion.

Hmm... it's confusing though that we'd still be generating composefs images even if it's been explicitly turned off. I feel like this is quite close to the maybe suggestion, but goes one step further than it needs to and lowers the creation threshold to no?

Another way to look at the maybe approach I think is that it makes the deploy and mount logic consistent. The mount logic is "if the composefs image is available, then mount it" and the deploy logic becomes "if composefs is available, then generate it". This is also closer to the usual semantic of maybe.

The composefs libostree integration has been supported for a while now
and is actively in use in various ostree/bootc-based systems. Let's
turn it on by default.

This has no effect if composefs support is not compiled in. Note also
that this does not change the default value of the `composefs.enabled`
tristate to `true`. The default is still `maybe`, but the deploy API
will now also create composefs images for `maybe`.

The reason for doing it this way is so that systems upgrading from
old libostree versions (which may either not have composefs support or
may have composefs-related bugs) will still be able to upgrade and not
trip `ostree-prepare-root` in the new deployment (which allows missing
composefs images for `maybe`).

We may in the future change the default value to `true`.

See also: ostreedev#2867
If composefs was explicitly requested (`enabled = true`) but libostree
was not compiled with composefs support, error out at deploy time. This
matches the logic in `ostree-prepare-root`.
@jlebon jlebon force-pushed the pr/composefs-maybe branch from 0643544 to 41a7f36 Compare December 17, 2024 18:33
@jlebon
Copy link
Member Author

jlebon commented Dec 17, 2024

Updated this now but haven't tested it yet!

@jlebon
Copy link
Member Author

jlebon commented Dec 17, 2024

OK yup, can confirm this allows ratcheting from older libostree versions. ✔️

$ cosa run --qemu-image /srv/imgs/rhcos-412.86.202412161907-0-qemu.x86_64.qcow2
[root@cosa-devsh ~]# rpm-ostree rebase --experimental ostree-unverified-image:oci-archive:$(ls /mnt/workdir/builds/latest/x86_64/*.ociarchive)
[root@cosa-devsh ~]# ls -la /ostree/deploy/rhcos/deploy/*/.*.cfs
ls: cannot access '/ostree/deploy/rhcos/deploy/*/.*.cfs': No such file or directory
[root@cosa-devsh ~]# reboot
[root@cosa-devsh ~]# rpm-ostree status -v
State: idle
AutomaticUpdates: disabled
Deployments:
● ostree-unverified-image:oci-archive:/mnt/workdir/builds/latest/x86_64/scos-9.202412171851-0-ostree.x86_64.ociarchive (index: 0)
                   Digest: sha256:24eef5695d7c95bf07219093c8f6183596fe3b4144af38d673b8ebc7fe88ec2e
                  Version: 9.202412171851-0 (2024-12-17T18:55:00Z)
                   Commit: 35341846a7951dd16124f50a068c4e0d5cfc47d854fbc8ad7efedeb93c8867c9
                   Staged: no
                StateRoot: rhcos

  a99301b69bcdc27b5dba71aa103fdfd896e6ceb1345db9a108a4ee90b113bcd2 (index: 1)
                  Version: 412.86.202412161907-0 (2024-12-16T19:11:04Z)
                   Commit: a99301b69bcdc27b5dba71aa103fdfd896e6ceb1345db9a108a4ee90b113bcd2
                           ├─ rhel-8.6-appstream (2024-12-16T18:35:25Z)
                           ├─ rhel-8.6-baseos (2024-12-11T00:13:42Z)
                           ├─ rhel-8.6-fast-datapath (2024-12-10T00:08:42Z)
                           └─ rhel-8.6-server-ose-4.12 (2024-12-16T19:04:04Z)
                StateRoot: rhcos
[root@cosa-devsh ~]# rpm-ostree rebase :35341846a7951dd16124f50a068c4e0d5cfc47d854fbc8ad7efedeb93c8867c9
[root@cosa-devsh ~]# ls -la /ostree/deploy/rhcos/deploy/*/.*.cfs                                                                                                                                                                                                                                                            
-rw-r--r--. 1 root root 5849088 Dec 17 18:56 /ostree/deploy/rhcos/deploy/35341846a7951dd16124f50a068c4e0d5cfc47d854fbc8ad7efedeb93c8867c9.1/.ostree.cfs
[root@cosa-devsh ~]# reboot
[core@cosa-devsh ~]$ findmnt /
TARGET SOURCE    FSTYPE  OPTIONS
/      composefs overlay ro,relatime,seclabel,lowerdir=/run/ostree/.private/cfsroot-lower::/sysroot/ostree/repo/objects,redirect_dir=on,metacopy=on

@cgwalters cgwalters enabled auto-merge December 17, 2024 19:20
@cgwalters
Copy link
Member

/override ci/prow/fcos-e2e

Copy link

openshift-ci bot commented Dec 17, 2024

@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/fcos-e2e

In response to this:

/override ci/prow/fcos-e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@cgwalters cgwalters merged commit 64a38ae into ostreedev:main Dec 17, 2024
26 checks passed
jlebon added a commit to jlebon/os that referenced this pull request Dec 18, 2024
There are issues currently on ppc64le and kernel-64k in el9 that break
with composefs. So it's not sufficient to do an architecture check
anyway. See: https://issues.redhat.com/browse/RHEL-63985.

The upcoming release of libostree will turn on composefs by default, so
it's not sufficient to delete `prepare-root.conf`; we need to explicitly
turn it off. See: ostreedev/ostree#3353.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/composefs Issues related to composefs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants