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.

Sign up for GitHub

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

Jump to bottom

Reapply "manifests: enable cliwrap on Fedora 40+" #2887

Closed
wants to merge 2 commits into from
Closed

Reapply "manifests: enable cliwrap on Fedora 40+" #2887

wants to merge 2 commits into from

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Feb 29, 2024

This reverts commit 3789b06.

The reported issue that motivated the revert[1] is due to an rpm-ostree bug[2].

We can trivially work around this bug until the rpm-ostree fix lands in FCOS, so let's do that and re-enable cliwrap.

@jlebon
Reapply "manifests: enable cliwrap on Fedora 40+"
3ce2f77 
This reverts commit 3789b06.

The reported issue that motivated the revert[[1]] is due to an
rpm-ostree bug[[2]].

We can trivially work around this bug until the rpm-ostree fix lands in
FCOS, so let's do that and re-enable cliwrap.

[1]: coreos/fedora-coreos-tracker#1679
[2]: coreos/rpm-ostree#4848
@dustymabe
Copy link
Member

dustymabe commented Feb 29, 2024 via email

@dustymabe
Copy link
Member

dustymabe commented Feb 29, 2024 via email

@jlebon
Copy link
Member Author

jlebon commented Feb 29, 2024

Yes, this needs to be fixed in RHCOS too. I wasn't planning on immediately backporting everywhere in case more bugs show up. But I did plan to eventually make sure it's fixed before f40 GA, whether by backporting or just because we've cut a new release. (Notice I didn't mark this PR as closing coreos/fedora-coreos-tracker#1679 :) ). Not too concerned about leaving this in Beta for now.

Guess we could denylist, yeah... Though kernel replacement in the layering flow is a important feature. I don't want us to lose coverage of that in f40+ in the meantime.

dustymabe
dustymabe previously approved these changes Feb 29, 2024
View reviewed changes
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@cgwalters
Copy link
Member

FWIW (as the person who pushed for cliwrap and wrote a lot of the code) I now consider it a technological dead end and also a giant social mistake (effectively agreeing with initial feedback!)

The big technology problem here is that upgrading a package in a container build (whether via rpm or dnf or rpm-ostree if it ever supported rpm-ostree upgrade <package> is that the librpm stack will happily blow away the cliwrap entrypoint wrapper.

@jlebon
Copy link
Member Author

jlebon commented Feb 29, 2024

FWIW (as the person who pushed for cliwrap and wrote a lot of the code) I now consider it a technological dead end and also a giant social mistake (effectively agreeing with initial feedback!)

The big technology problem here is that upgrading a package in a container build (whether via rpm or dnf or rpm-ostree if it ever supported rpm-ostree upgrade <package> is that the librpm stack will happily blow away the cliwrap entrypoint wrapper.

Yikes, yes that's not great.

I'm still also not a big fan of the approach overall. The main thing I'm interested in really is trying to make dnf install work in layered builds. Once we have a Python-free dnf that works well in image mode, I'd rather we ship that in FCOS/RHCOS and back out cliwrap. Hmm, so I think actually this argues for a cliwrap-dnf: true to limit the wrapping to just dnf? The upgrade issue shouldn't be an issue there.

@jlebon
Copy link
Member Author

jlebon commented Feb 29, 2024

Hmm but actually, the confusing thing here is that we definitely do at least need the kernel-install wrapper for kernel replacements to work, right? I suppose the plan there is to have the real kernel-install be adapted to Just Work but that hasn't happened yet?

I don't see a kernel replacement example in https://gitlab.com/bootc-org/examples and https://github.com/coreos/layering-examples/blob/main/replace-kernel/Containerfile still includes an explicit RUN rpm-ostree cliwrap install-to-root /. Should we add cliwrap-binaries: [kernel-install] in the meantime in the bootc base image composes?

@cgwalters
Copy link
Member

I suppose the plan there is to have the real kernel-install be adapted to Just Work but that hasn't happened yet?

yeah cc coreos/rpm-ostree#4726

Should we add cliwrap-binaries: [kernel-install] in the meantime in the bootc base image composes?

Yeah, would probably make sense except do note it comes from systemd and is hence pretty likely to get replaced when upgrading systemd

@jlebon jlebon mentioned this pull request Feb 29, 2024
Closed
@jlebon
cliwrap: only wrap dnf and kernel-install
9968717 
All we really want to cliwrap is `dnf` so that a `dnf install ...` will
work. Again, as mentioned in the commit that introduced this, ideally we
can actually ship dnf itself once it's ready and then we can back this
out.

Also wrap `kernel-install` for now to make kernel replacements smoother
until we can get it to work seamlessly (this matches [[1]]).

[1]: CentOS/centos-bootc#377
@jlebon
Copy link
Member Author

jlebon commented Feb 29, 2024

OK, I changed the proposal here to only wrap dnf and kernel-install!

Yeah, would probably make sense except do note it comes from systemd and is hence pretty likely to get replaced when upgrading systemd

Done in CentOS/centos-bootc#377.

jlebon added a commit to jlebon/os that referenced this pull request Mar 4, 2024
@jlebon
Revert "common.yaml: enable cliwrap"
930d8cc 
This reverts commit ff686c6.

With cliwrap, rpm-ostree runs rpm with less privileges. We special-case
`--query`, so e.g. `rpm -qf` still runs with dropped privileges.

But anyway, we actually want to reduce this to wrapping *just* `dnf` for
now (see discussions in
coreos/fedora-coreos-config#2887).

Resolves: https://issues.redhat.com/browse/OCPBUGS-30149
This was referenced Mar 4, 2024
Merged
Closed
@jlebon
Copy link
Member Author

jlebon commented Mar 7, 2024

Retracting this. Will open a tracker issue with another proposal.

@jlebon jlebon closed this Mar 7, 2024
jbtrystram pushed a commit to jbtrystram/openshift-os that referenced this pull request Apr 22, 2024
@jlebon @jbtrystram
Revert "common.yaml: enable cliwrap"
1007de9 
This reverts commit ff686c6.

With cliwrap, rpm-ostree runs rpm with less privileges. We special-case
`--query`, so e.g. `rpm -qf` still runs with dropped privileges.

But anyway, we actually want to reduce this to wrapping *just* `dnf` for
now (see discussions in
coreos/fedora-coreos-config#2887).

Resolves: https://issues.redhat.com/browse/OCPBUGS-30149
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dustymabe dustymabe dustymabe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jlebon @dustymabe @cgwalters