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

Add install --force-replacefiles #3125

Merged
merged 6 commits into from
Oct 14, 2021
Merged

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Sep 14, 2021

Add support for RPMs which want to override files from other RPMs. This
could be useful generally, but specifically for example in OCP where
we're toying with bundling user-provided overrides as RPMs.

We're effectively surfacing rpm's --replacefiles switch, though in a
slightly safer way because while we do turn on the associated rpmdb flag
globally (RPMPROB_FILTER_REPLACEOLDFILES), we're only allowing the
checkouts of those specific packages to replace files.

This is also rendered separately in status as LocalForcedPackages.

@openshift-ci
Copy link

openshift-ci bot commented Sep 14, 2021

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

@cgwalters
Copy link
Member

/test all

I think having a provides marker for this is OK, but it feels like it'd be too easy for some 3rd party rpm-md repo to add this and have something unexpected happen again like replacing libc.so.

I guess I'd lean more towards something like rpm-ostree install --force-replace ./kubelet.rpm or so - and on the DBus/origin side this would be an explicitly separate packages-force-replace or so?

@cgwalters
Copy link
Member

(I know you know this, but I'm just re-linking here for completness for others to #2326 because it would make things exactly like this easier to implement and more obvious to users I think if we had a declarative format)

@cgwalters
Copy link
Member

cgwalters commented Sep 16, 2021

That said if you prefer this model I'm OK with it, just two other things:

  • What if we at least suggested this to the rpm/dnf community? We've built up a lot of rpm-ostree "special sauce" and this would be another one. But it could make sense for rpm/dnf too right?
  • Would be good to have tests + doc somewhere

@jlebon
Copy link
Member Author

jlebon commented Sep 17, 2021

That said if you prefer this model I'm OK with it, just two other things:

I think having it be gated client-side makes sense. I was thinking at first the RPM itself should be marked somehow since it's a specialized payload (in the OCP use case). But higher-level, I think it's clearer to just generalize to "let's expose rpm's --replacefiles switch" (though slightly nicer because it'd be enabled per-RPM thanks to the ostree checkout and not just at the transaction level).

What if we at least suggested this to the rpm/dnf community? We've built up a lot of rpm-ostree "special sauce" and this would be another one. But it could make sense for rpm/dnf too right?

If we go that way, there wouldn't be any special rpm-ostree sauce. Though I noticed now that dnf/libdnf doesn't today expose a way to enable --replacefiles. I can file an RFE for that.

Would be good to have tests + doc somewhere

Yup, will do!

@jlebon
Copy link
Member Author

jlebon commented Sep 30, 2021

Circling back to this, I was hacking on adding rpm-ostree install --replacefiles, but... hesitating a bit on having this be under install. Being able to replace base files is more akin to overrides, but I'm not sure how to nicely fit it into the overrides command. Maybe just rpm-ostree override replacefiles foo.rpm?

@jlebon
Copy link
Member Author

jlebon commented Oct 1, 2021

No, I think putting it in install is still better overall UX-wise, and just making the switch scarier. I like --force-replace as suggested above, or maybe --force-replacefiles as a nod to the equivalent RPM option.

@jlebon jlebon changed the title core: Support "fileoverride" RPMs Add install --force-replacefiles Oct 8, 2021
@jlebon jlebon marked this pull request as ready for review October 8, 2021 17:05
@jlebon jlebon force-pushed the pr/override-rpm branch 2 times, most recently from d667ec0 to 9f247c1 Compare October 8, 2021 17:21
@jlebon
Copy link
Member Author

jlebon commented Oct 8, 2021

Updated this now and added a test! As expected, it's way more churn because we need to store it separately in the origin. I debated making this just a special kind of LocalPackages, maybe extending the sha256:nevra string format to something like sha256:nevra[,flags] or something but meh... wasn't hard either to just copy/paste where needed, and didn't want to change formats just to make things temporarily easier if we're going to rework the origin stuff anyway.

Can split prep patches out if desired.

jlebon added 6 commits October 8, 2021 16:35
I find it easier to use separate variables with names matching the
parameter name of the receiving function. And also defaulting them to an
empty strv.
Support for specifying local RPMs via `file://` was added in a previous
commit, so we need to update this bit too.
Straightforward. Prep for future patch.
That way we can clearly have associated comments with each one. Also
prep for conditionally adding another flag.
More verbose, but easier to follow.
Prep for adding more conditionals in that area.
Add support for RPMs which want to override files from other RPMs. This
could be useful generally, but specifically for example in OCP where
we're toying with bundling user-provided overrides as RPMs.

We're effectively surfacing rpm's `--replacefiles` switch, though in a
slightly safer way because while we do turn on the associated rpmdb flag
globally (`RPMPROB_FILTER_REPLACEOLDFILES`), we're only allowing the
checkouts of those specific packages to replace files.

This is also rendered separately in `status` as `LocalForcedPackages`.
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.

LGTM!

Just noting this is another case of needing a lot of "data plumbing" that would be less necessary with #2326

@cgwalters cgwalters merged commit 6783b0a into coreos:main Oct 14, 2021
@cgwalters
Copy link
Member

One thing I just wanted to add related to this; a way in which this is a very "rpm-ostree" feature is the way it's implemented it does not have hysteresis.

In other words, with traditional rpm, the fact one used --replacefiles doesn't persist - it's a point-in-time action that isn't recorded persistently.

But here, the fact that we're recording exactly which packages have this flag on in rpm-ostree status both makes the situation very clear and understandable, and allows reproducing it across upgrades.

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.

2 participants