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

core: also wrap kernel-install for scriptlets #4950

Merged
merged 2 commits into from
May 8, 2024

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented May 7, 2024

It's confusing right now how specifically for the kernel, one has to use this obscure rpm-ostree cliwrap install-to-root / command to make it work. Let's just always enable it: in the client-side layering case, we don't run kernel scriptlets anyway so the wrapper is unused, and in the container case, this will allow users to not have to enable cliwrap and have it leak into their derived image.

I guess in theory, this should also allow us to stop ignoring kernel scriptlets and rely on this instead, though let's leave that for a separate investigation.

Closes: #4949

@jlebon
Copy link
Member Author

jlebon commented May 7, 2024

Of course, this still requires using rpm-ostree for kernel installs instead of dnf. We really need #4726 to be able to move to dnf. The main thing I'm trying to avoid with this is not leaking cliwrap bits into the image, but dropping the requirement on install-to-root is nice too.

jlebon added 2 commits May 7, 2024 10:44
It's confusing right now how specifically for the kernel, one has to use
this obscure `rpm-ostree cliwrap install-to-root /` command to make it
work. Let's just always enable it: in the client-side layering case, we
don't run kernel scriptlets anyway so the wrapper is unused, and in the
container case, this will allow users to not have to enable cliwrap and
have it leak into their derived image.

I guess in theory, this should also allow us to *stop* ignoring kernel
scriptlets and rely on this instead, though let's leave that for a
separate investigation.

Closes: coreos#4949
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.

A few things we need to bear in mind here.

One is that we've also gotten burned with cliwrap + rpm-ostree/dnf upgrade because the rpm stack will happily replace the wrapped binary with really undesirable semantics. In this case that would again happen with updating systemd in a container build.

So I'm wavering a bit on whether it's better to do this now or just try to fix it by properly integrating into kernel-install.

There is another alternative path, where instead of using cliwrap in this way during scripts we just override it in base container builds.

Actually yeah, the core problem here remains in that people are still going to use dnf and if we don't cliwrap that too it remains broken.

So...not opposed but I'm not sure it really makes sense to invest in this.

@jlebon
Copy link
Member Author

jlebon commented May 7, 2024

A few things we need to bear in mind here.

One is that we've also gotten burned with cliwrap + rpm-ostree/dnf upgrade because the rpm stack will happily replace the wrapped binary with really undesirable semantics. In this case that would again happen with updating systemd in a container build.

Hmm, we're not shipping this wrapper in the container image. Only installing it during the transaction. It's the same as all the other systemctl and shadow-utils wrappers we currently temporary wrap and then unwrap.

So I'm wavering a bit on whether it's better to do this now or just try to fix it by properly integrating into kernel-install.

Actually yeah, the core problem here remains in that people are still going to use dnf and if we don't cliwrap that too it remains broken.

Yeah, that's definitely the end goal. I'm going for a tactical fix here because basically today changing the kernel as part of OCP layering requires cliwrap, and cliwrap breaks the MCD. So that means that the documented procedure for replacing the kernel via layering is broken in 4.16.

We're all agreed we should have dnf install written in those docs instead eventually, but (1) we need to keep rpm-ostree install working for a while, and (2) we can't switch to dnf in that specific example until we dive into doing this properly via kernel-install.d integration. (For examples that don't include the kernel, I think there's a docs ticket open to update them to use dnf instead since those should work fine now.)

This definitely affects 4.16, so we'd need it backported to 9.4. I haven't checked yet if 4.15 is affected, but likely not (the rpm -q failure is likely from the 9.4 rebase).

@jlebon
Copy link
Member Author

jlebon commented May 7, 2024

There is another alternative path, where instead of using cliwrap in this way during scripts we just override it in base container builds.

Hmm, isn't that CentOS/centos-bootc#377 ? Or are you suggesting lifting it out of rpm-ostree and carrying it somewhere?

@cgwalters
Copy link
Member

Hmm, we're not shipping this wrapper in the container image. Only installing it during the transaction. It's the same as all the other systemctl and shadow-utils wrappers we currently temporary wrap and then unwrap.

OK yes, you're right.

Hmm, isn't that CentOS/centos-bootc#377 ? Or are you suggesting lifting it out of rpm-ostree and carrying it somewhere?

I'm actually thinking more that we stop trying to use rpm-ostree's builtin kernel handling, and open-code it as hooks in /usr/lib/kernel-install.d. Though, this gets into which project owns those hooks. Maybe what would actually be best is to have them be in like a contrib/fedora directory in ostree?

This is what would allow us to support dnf.

@cgwalters cgwalters merged commit 5be2ecf into coreos:main May 8, 2024
17 checks passed
@jlebon
Copy link
Member Author

jlebon commented May 8, 2024

I'm actually thinking more that we stop trying to use rpm-ostree's builtin kernel handling, and open-code it as hooks in /usr/lib/kernel-install.d. Though, this gets into which project owns those hooks. Maybe what would actually be best is to have them be in like a contrib/fedora directory in ostree?

Having it live in ostree makes sense to me. Though ideally it can be used by any ostree user that also uses dracut + systemd's kernel-install and not only a Fedora-derivative thing?

That said, ideally long-term the same code that prepares the kernel and initramfs server-side is what regenerates it in the container derivation and client-side layering cases. E.g. we'd drain e.g. the dracut and depmod stuff from rpm-ostree and actually adapt the canonical scripts as needed to do the right thing.

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.

Auto-wrap kernel-install if layering kernel packages
2 participants