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

prepare-root: Add support for root.transient #3114

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

cgwalters
Copy link
Member

sysroot: Drop unused prototype

This function doesn't exist anymore.


prepare-root: Add an autofree

This doesn't matter at all, but I just noticed this while working
on the code.


prepare-root: Add support for root.transient

Closes: #3113

It'd greatly improve compatibility with things like RPMs that install
in /opt if we supported a full "original docker" style model where
/ is a transient overlayfs. We'd still keep our semantics for /etc
and /var by default, but e.g. we'd stop recommending
/opt ➡️ /var/opt, in this model,
so /opt would be on the overlayfs.

Note this all aligns with composefs, where we'd actually be making
/ a read-only overlayfs by default; it'd be really nice of course
to implement this by just making the composefs overlayfs writable,
but I am not sure we can hard require composefs for this right now.

So this change adds support for root.transient = true
in /usr/lib/ostree/prepare-root.conf.

The major downside is that people could be surprised if files they
write to e.g. /opt don't persist across upgrades. But, that's
already again how it works since Docker started.

Note as part of the implementation of this, we need to add a whole
new "backing" directory distinct from the deployment directories.

(Tangentially related to this, it's tempting to switch to always
using a read-only overlay mount by default.


This function doesn't exist anymore.
This doesn't matter at all, but I just noticed this while working
on the code.
Closes: ostreedev#3113

It'd greatly improve compatibility with things like RPMs that install
in `/opt` if we supported a full "original docker" style model where
`/` is a transient overlayfs.  We'd still keep our semantics for `/etc`
and `/var` by default, but e.g. we'd stop recommending
`/opt` ➡️ `/var/opt`, in this model,
so `/opt` would be on the overlayfs.

Note this all aligns with composefs, where we'd actually be making
`/` a *read-only* overlayfs by default; it'd be really nice of course
to *implement* this by just making the composefs overlayfs writable,
but I am not sure we can hard require composefs for this right now.

So this change adds support for `root.transient = true`
in `/usr/lib/ostree/prepare-root.conf`.

The major downside is that people could be surprised if files they
write to e.g. `/opt` don't persist across upgrades.  But, that's
already again how it works since Docker started.

Note as part of the implementation of this, we need to add a whole
new "backing" directory distinct from the deployment directories.

(Tangentially related to this, it's tempting to switch to always
 using a *read-only* overlay mount by default.
Copy link

openshift-ci bot commented Dec 7, 2023

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

@github-actions github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label Dec 7, 2023
@cgwalters
Copy link
Member Author

cgwalters commented Dec 7, 2023

So this seems to work well:

[root@cosa-devsh ~]# findmnt /
TARGET
  SOURCE  FSTYPE  OPTIONS
/ overlay overlay rw,relatime,seclabel,lowerdir=.,upperdir=../../backing/4b42f60ddbb87dbcabdf17e15b1414234ce03eefab0b2f0c2e7972dc1bb816bf.0/root-transient/upper,workdir=../../backing/4b42f60ddbb87dbcabdf17e15b1414234ce03eefab0b2f0c2e7972dc1bb816bf.0/root-transient/work,uuid=on

Then I did rm /opt && mkdir /opt, changes there persist across reboot.

A next step here is to change rpm-ostree to detect this configuration and stop doing the opt ➡️ var/opt link (edit: done in coreos/rpm-ostree#4719 )

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Dec 7, 2023
This pairs with ostreedev/ostree#3114

Basically we want to detect the case where the OS has opted-in
to this new mode and *not* symlink things.

I originally thought we could implement this by just moving
all the toplevel directories, but then I hit on the fact that
because the `filesystem` package is creating all the toplevel
directories in lua script which we ignore...that doesn't work.

So we need to keep making them by hand.
cgwalters added a commit to cgwalters/ostree-rs-ext that referenced this pull request Dec 8, 2023
This pairs with ostreedev/ostree#3114

Basically if ostree has an overlayfs (whether writable or a readonly
composefs), we're safe to handle non-`/usr` content.

However, we still *always* drop content from the
"API filesystems" like `/run`, `/dev` etc.  Because really,
no one should be adding stuff there.
@cgwalters cgwalters marked this pull request as ready for review December 8, 2023 12:54
@cgwalters
Copy link
Member Author

This also wants ostreedev/ostree-rs-ext#570

cgwalters added a commit to cgwalters/centos-bootc that referenced this pull request Dec 8, 2023
@cgwalters
Copy link
Member Author

BTW this change also requires coreos/coreos-installer#1203 for FCOS for the same reason as composefs.
And will also trip over os-prober failing. os-prober is just the worst 😢
Anyways though, I think I'd like to just get this (and all its dependent patches) in since it should be inert code until explicitly turned on (example).

@rhatdan
Copy link
Contributor

rhatdan commented Dec 8, 2023

LGTM
@vrothberg @jlebon PTAL

Copy link
Collaborator

@ericcurtin ericcurtin left a comment

Choose a reason for hiding this comment

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

LGTM

@ericcurtin ericcurtin merged commit 071053d into ostreedev:main Dec 8, 2023
24 checks passed
@@ -3360,6 +3407,9 @@ sysroot_finalize_deployment (OstreeSysroot *self, OstreeDeployment *deployment,
if (!selinux_relabel_var_if_needed (self, sepolicy, os_deploy_dfd, cancellable, error))
return FALSE;

if (!sysroot_initialize_deployment_backing (self, deployment, sepolicy, error))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be gated on root.transient?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right to call this out, but this was semi-intentional because one thing I want to do is change usroverlay to write here instead of the /var/tmp it does today - because that would just be better; the content would be clearly "lifecycle bound" to the deployment and we wouldn't need that hacky tmpfiles.d snippet we carry:

# https://github.com/ostreedev/ostree/issues/393
R! /var/tmp/ostree-unlock-ovl.*

(That said to do that note we'd need to also change some logic to ensure the overlay content for it is actually removed on reboot by default)

This all said, yes we could still create it dynamically in either case. I think we actually should have logic here in ostree to do what is being replicated in ostree-rs-ext and rpm-ostree around querying the initramfs config. Then we could drop the ex-integrity.composefs repo config.

@@ -3078,6 +3078,10 @@ sysroot_initialize_deployment (OstreeSysroot *self, const char *osname, const ch
if (!require_stateroot (self, osname, error))
return FALSE;

g_autofree char *stateroot_backing = g_strdup_printf ("ostree/deploy/%s/backing", osname);
if (!glnx_shutil_mkdir_p_at (self->sysroot_fd, stateroot_backing, 0700, cancellable, error))
Copy link
Member

Choose a reason for hiding this comment

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

And this too.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Dec 8, 2023
This pairs with ostreedev/ostree#3114

Basically we want to detect the case where the OS has opted-in
to this new mode and *not* symlink things.

I originally thought we could implement this by just moving
all the toplevel directories, but then I hit on the fact that
because the `filesystem` package is creating all the toplevel
directories in lua script which we ignore...that doesn't work.

So we need to keep making them by hand.
cgwalters added a commit to cgwalters/ostree-rs-ext that referenced this pull request Dec 9, 2023
This pairs with ostreedev/ostree#3114

Basically if ostree has an overlayfs (whether writable or a readonly
composefs), we're safe to handle non-`/usr` content.

However, we still *always* drop content from the
"API filesystems" like `/run`, `/dev` etc.  Because really,
no one should be adding stuff there.
cgwalters added a commit to cgwalters/rhel9-bootc-demos that referenced this pull request Dec 11, 2023
This builds on top of the test image using
ostreedev/ostree#3114
and related PRs.
cgwalters added a commit to cgwalters/rhel9-bootc-demos that referenced this pull request Dec 11, 2023
This builds on top of the test image using
ostreedev/ostree#3114
and related PRs.
cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Dec 11, 2023
This pairs with ostreedev/ostree#3114

Basically we want to detect the case where the OS has opted-in
to this new mode and *not* symlink things.

I originally thought we could implement this by just moving
all the toplevel directories, but then I hit on the fact that
because the `filesystem` package is creating all the toplevel
directories in lua script which we ignore...that doesn't work.

So we need to keep making them by hand.
cgwalters added a commit to coreos/rpm-ostree that referenced this pull request Dec 12, 2023
This pairs with ostreedev/ostree#3114

Basically we want to detect the case where the OS has opted-in
to this new mode and *not* symlink things.

I originally thought we could implement this by just moving
all the toplevel directories, but then I hit on the fact that
because the `filesystem` package is creating all the toplevel
directories in lua script which we ignore...that doesn't work.

So we need to keep making them by hand.
@cgwalters
Copy link
Member Author

BTW for people seeing this now, it's also worth looking at #3120

@travier
Copy link
Member

travier commented Jan 10, 2024

It's unclear to me reading the commit message and the issues here what exactly happens when we turn this knob on. Could you write a bit of docs about it? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prepare-root Issue relates to ostree-prepare-root
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for full overlayfs for /
5 participants