-
Notifications
You must be signed in to change notification settings - Fork 192
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
Support RPMs installing in /opt
and /usr/local
#4728
Conversation
Skipping CI for Draft Pull Request. |
Requires: ostreedev/ostree#3120 This also still needs tests. |
I know this is quite optimistic. :) But I think this is pretty close to a fix for it, even if there are some subtle caveats. With the experimental knob there, I'm hoping we can get feedback on how well this works in practice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just starting to dig into this...seems like we could split out a bit of this as prep commits?
if path.starts_with("/usr/") && !path.starts_with("/usr/local") { | ||
if path.starts_with("/usr/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this change needs to be gated on us detecting that a state overlay is enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to be strict, yes. I didn't bother with this because /usr/local
RPMs are known broken right now anyway and this PR is working to improve the situation. With this hunk but without state overlays enabled, it'll still be broken, but in a different way (though true, in a more subtle way than the current explicit error message users get -- content in /usr/local
will just be deleted in postprocessing when replaced by the /var/usrlocal
symlink).
Can pipe through the needed flag to retain current behaviour if wanted.
6f67c71
to
6021d66
Compare
537846b
to
c59a6cf
Compare
c59a6cf
to
d8ce778
Compare
d8ce778
to
1cec27b
Compare
1cec27b
to
4cba254
Compare
Now rebased and with tests! I'll note that there should be no functional change here if state overlays are not enabled. |
|
4cba254
to
3f3cd76
Compare
]; | ||
|
||
UNITS.par_iter().try_for_each(|&unit| { | ||
let target = Path::new("..").join(unit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, doesn't cap-std barf on this? I did this PR a while ago which we use in other places; though maybe it doesn't actually check for ..
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, seems to work here! :)
@@ -955,6 +1008,17 @@ fn convert_path_to_tmpfiles_d_recurse( | |||
Ok(()) | |||
} | |||
|
|||
fn state_overlay_enabled(rootfs_dfd: &cap_std::fs::Dir, state_overlay: &str) -> Result<bool> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function in theory could live in ostree's rust code or ostree-ext, not that it matters much in practice. (Please don't move it just for this, but just a thought)
@@ -4062,6 +4062,55 @@ rpmostree_context_assemble (RpmOstreeContext *self, GCancellable *cancellable, G | |||
if (!build_rootfs_usrlinks (self, error)) | |||
return FALSE; | |||
|
|||
/* This is purely for making it easier for people to test out the | |||
* state-overlay stuff until it's stabilized and part of base composes. */ | |||
if (g_getenv ("RPMOSTREE_EXPERIMENTAL_FORCE_OPT_USRLOCAL_OVERLAY")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to document how to do this in a derived container build? The ergonomics around providing an environment variable in this context seems...difficult.
systemctl stop rpm-ostreed | ||
unshare -m /bin/bash -c 'mount -o rw,remount /sysroot && sed -i -e "s/refspec=.*/refspec=kolatest/" /ostree/deploy/*/deploy/*.origin' | ||
|
||
# XXX: until ostree v2024.1 hits FCOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like that should happen in a day or two at most
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it hasn't synced to the mirrors yet. I'll keep it in for now and pull it out once it makes it in (whether here or in a follow-up).
mkdir -p /etc/systemd/system/rpm-ostreed.service.d/ | ||
cat > /etc/systemd/system/rpm-ostreed.service.d/state-overlay.conf <<EOF | ||
[Service] | ||
Environment=RPMOSTREE_EXPERIMENTAL_FORCE_OPT_USRLOCAL_OVERLAY=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh I see it was for CI testing...ah. WDYT about changing this test to use a container derivation instead? Which could be done in-kola, although...
Or alternatively...this may argue for changing our CI here to have a separate flow which does separate treecompose runs and separate CI off each of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this from a container requires ostreedev/ostree-rs-ext#573, no? Or do you mean fixing the directories and enabling the services in a container build but still installing the RPM client-side? That feels like the worst of both worlds. :)
Not too concerned about ergonomics for this. A service dropin isn't too bad a way to opt into experimental stuff, no? Though definitely once we have support in the container path, we should add a test for that.
Re. doing a separate treecompose run, yeah I think we do want that too (and has definitely come up before). Hmm, how about for now instead of adding another run to CI, we exercise it in the fcos-e2e
tests since that one also does a full compose and kola tests? We can adapt the test to skip the experimental knob injection if it detects that it's already on in the base compose.
Obviously, this alone is not enough to expose that content but it's a start. Currently as is, it'll get nuked when we replace `/usr/local` by a symlink in postprocessing. A future patch will address that part. Part of coreos#233.
This solves the `/opt` problem by using the new state overlay concept in OSTree: an overlay filesystem is mounted on top of `/usr/lib/opt` and the upper dir is automatically "rebased" whenever new content comes in. Concretely, this means that app state is carried forward, all while allowing the (OSTree-managed) package contents to be updated. We also solve the `/usr/local` problem the same way. The app state issue isn't really present there, but `/usr/local` has traditionally been system state. We want to keep supporting dropping files there all while also supporting shipping OSTree-owned content. See also: ostreedev/ostree#3113 Fixes: coreos#233
7a2287b
to
7fd72dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
let mut db = cap_std::fs::DirBuilder::new(); | ||
db.recursive(true); | ||
db.mode(0o755); | ||
let localfs_requires = Path::new("usr/lib/systemd/system/local-fs.target.requires"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one could probably live as a const
given it's referenced twice.
Follow-up from coreos#4728.
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options. Follow-up to coreos#4728.
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options without losing coverage on the default path. Follow-up to coreos#4728.
Follow-up from coreos#4728.
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options without losing coverage on the default path. Follow-up to coreos#4728.
Follow-ups to this in #4810. |
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options without losing coverage on the default path. Follow-up to coreos#4728.
The new experimental `opt-usrlocal-overlays` treefile knob[1] allows users to overlay packages with `/opt`/`/usr/local` content (and eventually, rebase to container images with content in those places). However, booting a base compose with this knob will break Ignition configs that currently write data in e.g. `/opt` since that now points to `/usr/lib/opt`, which is of course read-only. We need to assemble the state overlays from the initramfs so that those configs keep working seamlessly. This is a no-op if state overlays are off (status quo), but it'll make it easier for people and CI to test the feature. This is a tiny part of the required bits if we want to eventually turn this on in FCOS/RHCOS. The other major part is migrating existing systems. [1] coreos/rpm-ostree#4728
Follow-up from coreos#4728.
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options without losing coverage on the default path. Follow-up to coreos#4728.
The new experimental `opt-usrlocal-overlays` treefile knob[1] allows users to overlay packages with `/opt`/`/usr/local` content (and eventually, rebase to container images with content in those places). However, booting a base compose with this knob will break Ignition configs that currently write data in e.g. `/opt` since that now points to `/usr/lib/opt`, which is of course read-only. We need to assemble the state overlays from the initramfs so that those configs keep working seamlessly. This is a no-op if state overlays are off (status quo), but it'll make it easier for people and CI to test the feature. This is a tiny part of the required bits if we want to eventually turn this on in FCOS/RHCOS. The other major part is migrating existing systems. [1] coreos/rpm-ostree#4728
The new experimental `opt-usrlocal-overlays` treefile knob[1] allows users to overlay packages with `/opt`/`/usr/local` content (and eventually, rebase to container images with content in those places). However, booting a base compose with this knob will break Ignition configs that currently write data in e.g. `/opt` since that now points to `/usr/lib/opt`, which is of course read-only. We need to assemble the state overlays from the initramfs so that those configs keep working seamlessly. This is a no-op if state overlays are off (status quo), but it'll make it easier for people and CI to test the feature. This is a tiny part of the required bits if we want to eventually turn this on in FCOS/RHCOS. The other major part is migrating existing systems. [1] coreos/rpm-ostree#4728
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options without losing coverage on the default path. Follow-up to coreos#4728.
Follow-up from coreos#4728.
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options without losing coverage on the default path. Follow-up to coreos#4728.
Follow-up from coreos#4728.
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options without losing coverage on the default path. Follow-up to coreos#4728.
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options without losing coverage on the default path. Follow-up to coreos#4728.
Since both Prow and CoreOS CI do a compose and run kola tests, we can change one of them to cover some different options without losing coverage on the default path. Follow-up to #4728.
The new experimental `opt-usrlocal-overlays` treefile knob[1] allows users to overlay packages with `/opt`/`/usr/local` content (and eventually, rebase to container images with content in those places). However, booting a base compose with this knob will break Ignition configs that currently write data in e.g. `/opt` since that now points to `/usr/lib/opt`, which is of course read-only. We need to assemble the state overlays from the initramfs so that those configs keep working seamlessly. This is a no-op if state overlays are off (status quo), but it'll make it easier for people and CI to test the feature. This is a tiny part of the required bits if we want to eventually turn this on in FCOS/RHCOS. The other major part is migrating existing systems. [1] coreos/rpm-ostree#4728
This solves the
/opt
problem by using the new state overlay concept inOSTree: an overlay filesystem is mounted on top of
/usr/lib/opt
andthe upper dir is automatically "rebased" whenever new content comes in.
Concretely, this means that app state is carried forward, all while
allowing the (OSTree-managed) package contents to be updated.
We also solve the
/usr/local
problem the same way. The app state issueisn't really present there, but
/usr/local
has traditionally beensystem state. We want to keep supporting dropping files there all while
also supporting shipping OSTree-owned content.
See also: ostreedev/ostree#3113
Fixes: #233