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

Always generate composefs blob, don't enable runtime by default #3366

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

cgwalters
Copy link
Member

Followup to 9a0acd7

Basically our composefs enablement flag has long had a tension between trying to do two things:

  • Enable generating the composefs blob (at deployment time)
  • Enable at runtime in prepare-root

And we've hit issues in "ratcheting" enabling composefs across upgrades because of this.

This change builds on the previous one, and now it's really simple to talk about:

  • If composefs is enabled at build time, we always generate a composefs blob at deplyment time
  • Configuring the prepare-root config now mostly only affects the runtime state.

There is one detail though: in order to handle the verity requirement at deploy time, we do still parse the config then.

But for the basic "is composefs enabled at all at runtime" that is now fully keyed off the config, not the build time or (worse) whether the deployment happened to have a composefs blob.

For users who want composefs on, they need to do so in the base image configuration.

@cgwalters
Copy link
Member Author

Only compile tested locally so far

Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

LGTM for the goal. Not tested/fully reviewed the code change yet.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Hmm, I think this breaks openshift/os#1678 again, which was the primary motivation for #3353.

Well no, I guess it's fine. Previously, we'd delete the composefs enablement config file to rely on the maybe default. Now, I think we'd have to do an explicit sed to change from true to maybe, which seems cleaner anyway.

Anyway, LGTM overall.

docs/composefs.md Show resolved Hide resolved
@cgwalters
Copy link
Member Author

Hmm, I think this breaks openshift/os#1678 again, which was the primary motivation for #3353.
Well no, I guess it's fine. Previously, we'd delete the composefs enablement config file to rely on the maybe default. Now, I think we'd have to do an explicit sed to change from true to maybe, which seems cleaner anyway.

That's what the code is already doing, true? https://github.com/openshift/os/pull/1700/files#diff-20b4b6908742b7029e7ed3ac70d6db47f9b9420aaf337aee712da093d1f8d129R67

@cgwalters cgwalters force-pushed the unconditional-cfs branch 2 times, most recently from 43a8d2b to 95b13de Compare January 9, 2025 22:57
Followup to ostreedev@9a0acd7

Basically our composefs enablement flag has long had a tension between
trying to do two things:

- Enable generating the composefs blob (at deployment time)
- Enable at runtime in prepare-root

And we've hit issues in "ratcheting" enabling composefs
across upgrades because of this.

This change builds on the previous one, and now it's really
simple to talk about:

- If composefs is enabled at build time, we *always*
  generate a composefs blob at deplyment time
- Configuring the prepare-root config now mostly
  only affects the runtime state.

There is one detail though: in order to handle the
verity requirement at deploy time, we do still parse
the config then.

But for the basic "is composefs enabled at all at runtime"
that is now fully keyed off the config, not the build time
or (worse) whether the deployment happened to have a composefs
blob.

For users who want composefs on, they need to do so in the base
image configuration.

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters added this to the -antirust milestone Jan 10, 2025
@jlebon jlebon merged commit 639db09 into ostreedev:main Jan 10, 2025
26 checks passed
@jlebon
Copy link
Member

jlebon commented Jan 10, 2025

Hmm, I think this breaks openshift/os#1678 again, which was the primary motivation for #3353.
Well no, I guess it's fine. Previously, we'd delete the composefs enablement config file to rely on the maybe default. Now, I think we'd have to do an explicit sed to change from true to maybe, which seems cleaner anyway.

That's what the code is already doing, true? openshift/os#1700 (files)

Yes, we're currently editing it but we used to delete it. Mostly saying I guess we won't revert once we're ready to turn it back on but rather tweak it.

@champtar
Copy link

Just a comment because I haven't seen it mentioned, enabling composefs requires some prep work, so having it turn on by default would have break some users:

@cgwalters
Copy link
Member Author

grub2-probe fails with composefs so you can't use grub2-mkconfig (migrate to bootupd + migrate the grub config)

https://bugzilla.redhat.com/show_bug.cgi?id=2308594

@cgwalters cgwalters removed this from the -antirust milestone Jan 14, 2025
@jmarrero jmarrero mentioned this pull request Jan 15, 2025
@champtar
Copy link

grub2-probe fails with composefs so you can't use grub2-mkconfig (migrate to bootupd + migrate the grub config)

https://bugzilla.redhat.com/show_bug.cgi?id=2308594

Yup I know this is a bug that might get fixed, but at the same time this is not the first breakage that I see with grub2-probe (xfs bigtime and EL8), so it's a good excuse to migrate to bootupd

@champtar
Copy link

@cgwalters I think this PR breaks the downgrade (not rollback) use case

You use ostree with composefs compiled in but no prepare-root.conf, you then upgrade to ostree 2025.1, it breaks, so you add a prepare-root.conf to disable composefs, but once you are using 2025.1, you have no way to downgrade to a previous build that didn't have an explicit config, except maybe remount /sysroot and delete .ostree.cfs ?

@cgwalters
Copy link
Member Author

The runtime default for composefs is off since this PR. If you don't have a config, you won't get composefs, even if we generate the composefs blob.

Unless I'm missing something I don't see how your case would be broken.

@champtar
Copy link

If you downgrade to an old build with no config and an ostree version without this PR, default runtime was maybe

@cgwalters
Copy link
Member Author

Yes sorry, I think you're right. We could add an option to disable generating the composefs blob too, and then one could ship that until the longest release from which you don't want to support rolling back to. Would be ugly.

except maybe remount /sysroot and delete .ostree.cfs ?

Yes. I guess I'd probably recommend this as the most practical fix for folks in this situation - ship a systemd unit which runs e.g. After=ostree-finalize-staged.service, and deletes the .ostree.cfs files in the deployment roots. We're not going to do anything which would break code like that even though the filename is sort of an ostree implementation detail, because it's kind of an internal ABI between ostree versions at least.

@cgwalters
Copy link
Member Author

I'm also open to adding a configuration option now which turns off the composefs blob generation, it'd be relatively straightforward. I guess it'd be way less hacky than deleting the blob.

But of course, it just re-creates the ratcheting problem for the case where you do want to start a migration.

@champtar
Copy link

I think I've nailed down my migration, no need for extra config

If using ostree < 2025.1, to always generate the composefs blob:

cat > /usr/lib/systemd/system/ostree-enable-composefs.service <<'EOF'
[Unit]
Description=OSTree enable ex-integrity.composefs
ConditionKernelCommandLine=ostree
Before=ostree-readonly-sysroot-migration.service
Before=tuned.service
Before=rpm-ostreed.service

[Service]
Type=oneshot
RemainAfterExit=yes
ExecStart=/usr/bin/ostree config set ex-integrity.composefs true
ExecStop=/usr/bin/ostree config unset ex-integrity.composefs
KillSignal=SIGCONT

[Install]
WantedBy=multi-user.target
EOF

(we unset on stop as we don't want to leak the setting to other deployments)

And then to allow rollback, we delete the composefs blob when prepare-root.conf is missing

cat > /usr/lib/systemd/system/ostree-finalize-staged.service.d/ostree-composefs-fixup.conf <<'EOF'
[Service]
ExecStop=/usr/libexec/ostree-composefs-fixup
EOF
cat > /usr/libexec/ostree-composefs-fixup <<'EOF'
#!/usr/bin/bash
set -euo pipefail
shopt -s nullglob

mount -o remount,rw /sysroot

for cfs in /sysroot/ostree/deploy/*/deploy/*/.ostree.cfs; do
    deploy="${cfs%/*}"
    if [ ! -e "${deploy}/usr/lib/ostree/prepare-root.conf" ]; then
        echo "No prepare-root.conf, assuming no composefs support, removing '${cfs}'"
        lsattr="$(lsattr -d ${deploy})"
        [[ "${lsattr%% *}" != *i* ]] || chattr -i "${deploy}"
        rm "${cfs}"
        [[ "${lsattr%% *}" != *i* ]] || chattr +i "${deploy}"
    fi
done
EOF

(no need to remount ro as we are in a mount namespace that is about to be destroyed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants