treefile: Drop override-replace-local-rpms and support install and overrides by URL in container path #3670

jlebon · 2022-05-10T17:16:59Z

The addition of override-replace-local-rpms was a hack to bridge the
gap between the container and client-side flow where only the latter has
access to a pkgcache repo.

This patch enhances the existing override-replace-local key to work in
the container flow: we stage RPMs in a directory in /run, and the core
knows to look there for them.

This then allows us to remove the override-replace-local-rpms key and
clean up a bunch of things related to it.

To be nice, we might have to in the future expose the staging logic as a
separate command which outputs on stdout the digest and NEVRA so that
users can include them in their YAML dropins.

This is prep also for supporting local RPMs, which will work a similar
way.

openshift-ci · 2022-05-10T17:17:02Z

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

jlebon · 2022-05-10T17:17:24Z

Keeping this as draft; trying to oxidize stage_container_rpms.

jlebon · 2022-05-10T17:20:50Z

Split out prep patches in #3671.

cgwalters · 2022-05-10T21:20:11Z

client-side flow where only the latter has access to a pkgcache repo.

Well, we can start making use of e.g. ostreedev/ostree#2532

jlebon · 2022-05-11T15:38:04Z

client-side flow where only the latter has access to a pkgcache repo.

Well, we can start making use of e.g. ostreedev/ostree#2532

We need write support too. Unless you're suggesting doing that through ostree-rs-ext, but I'm not sure it's worth the complexity.

cgwalters · 2022-05-11T17:35:29Z

I think we should add write support to ostree-rs-ext. But this is a larger topic.

jlebon · 2022-05-19T22:45:05Z

OK latest commits add:

support for rpm-ostree install of local RPMs and URLs in the container flow (local rpms are ignored when running rpm-ostree in a container. #3560)
support for rpm-ostree override replace via HTTP/Koji/Bodhi URLs in the container flow

Though one thing missing is inactive override detection like we have in the client-side. I think we need to lower that logic from the upgrader to the core.

Will split this up into multiple commits/PRs.

cgwalters

Overall direction LGTM!

rust/src/core.rs

cgwalters · 2022-05-24T12:24:49Z

rust/src/lockfile.rs

+        let mut pkg = unsafe {
+            libdnf_sys::dnf_package_from_ptr(&mut pkg.0 as *mut _ as *mut libdnf_sys::FFIDnfPackage)
+        };
+        let name = pkg.pin_mut().get_name();


I think it'd be a bit cleaner to do:

let mut pkg = ...; let pkg = pkg.pin_mut(); let name = pkg.get_name() let evr = pkg.get_evr()

etc.

Heh, I initially did that, but it complains about the value being moved on the first call. TBH, I don't quite understand the pinning stuff, so just went with this.

Came up in review of coreos#3671.

Instead of having individual separate functions acting on a pointer to a libdnf object, bundle them up into proper classes instead. This not only makes it feel cleaner on the Rust side, but also has the major advantage of being able to hold a reference to an object. This means that it's easier to wrap libdnf functions that return new references for use on the Rust side, which is something we'll do in an upcoming patch.

Prep for next patch.

The addition of `override-replace-local-rpms` was a hack to bridge the gap between the container and client-side flow where only the latter has access to a pkgcache repo. This patch enhances the existing `override-replace-local` key to work in the container flow: we stage RPMs in a directory in `/run`, and the core knows to look there for them. This then allows us to remove the `override-replace-local-rpms` key and clean up a bunch of things related to it. To be nice, we might have to in the future expose the staging logic as a separate command which outputs on stdout the digest and NEVRA so that users can include them in their YAML dropins. This is prep also for supporting local RPMs, which will work a similar way.

Use the same `client_handle_fd_argument` function which knows how to handle Bodhi/Koji URLs and regular RPM filenames and URLs in the container handling paths for `install` and `override replace`. This also allows us to drop another treefile mutator that's no longer necessary (`set_packages`). Closes: coreos#3560

jlebon · 2022-05-24T20:40:54Z

OK cleaned this up and made separate commits now!

cgwalters · 2022-05-25T08:05:35Z

rust/libdnf-sys/cxx/libdnf.cxx

-dnf_package_get_arch (DnfPackage &pkg)
-{
-  return rust::String (::dnf_package_get_arch (&pkg));
+  return std::make_unique<DnfPackage> (g_object_ref (pkg));


The downside is we're double boxing. But, eh.

cgwalters · 2022-05-25T08:30:20Z

rust/src/core.rs

@@ -224,6 +229,45 @@ pub(crate) fn verify_kernel_hmac(rootfs: i32, moddir: &str) -> CxxResult<()> {
    verify_kernel_hmac_impl(&moddir).map_err(Into::into)
 }

+pub(crate) fn stage_container_rpms(rpms: Vec<String>) -> CxxResult<Vec<String>> {


I'm fine with this as is, but: why not switch to doing this in the daemon path too?

Yeah, maybe.

The idea with this is that it acts like the pkgcache repo in the container case where we don't have one. So we could use this client-side too, but then turn around and immediately import them into the pkgcache? I could see some utility to that. But I was actually thinking of going the other way and eventually drop this once we can use bare-split-xattrs more easily. That opens the door to using the full core assembly code. So then e.g. things like kernel replacement would re-use the same path.

jlebon · 2022-05-25T13:54:03Z

OK, merging this!

openshift-ci bot added the do-not-merge/work-in-progress label May 10, 2022

openshift-ci bot added the needs-rebase label May 11, 2022

jlebon force-pushed the pr/rework-local-overrides branch from 0418fe8 to 465dc7f Compare May 11, 2022 15:31

jlebon mentioned this pull request May 11, 2022

Prep patches from https://github.com/coreos/rpm-ostree/pull/3670 #3671

Merged

jlebon force-pushed the pr/rework-local-overrides branch from 465dc7f to b9dab84 Compare May 11, 2022 15:36

openshift-ci bot removed the needs-rebase label May 11, 2022

jlebon force-pushed the pr/rework-local-overrides branch 2 times, most recently from a0c7533 to 783c2c4 Compare May 19, 2022 20:48

jlebon force-pushed the pr/rework-local-overrides branch 2 times, most recently from 8f94699 to 4411d91 Compare May 20, 2022 14:34

cgwalters reviewed May 24, 2022

View reviewed changes

jlebon added 6 commits May 24, 2022 15:32

ci: Add override CLI container tests

d6ffe1a

Came up in review of coreos#3671.

rust/core: Import anyhow macro for brevity

0e3109d

rust/libdnf-sys: Bridge DnfSack

f06d711

Prep for next patch.

jlebon force-pushed the pr/rework-local-overrides branch from 4411d91 to 7355631 Compare May 24, 2022 20:34

jlebon marked this pull request as ready for review May 24, 2022 20:34

openshift-ci bot removed the do-not-merge/work-in-progress label May 24, 2022

cgwalters approved these changes May 25, 2022

View reviewed changes

jlebon merged commit 9b3ca74 into coreos:main May 25, 2022

jlebon deleted the pr/rework-local-overrides branch May 25, 2022 13:54

jlebon changed the title ~~treefile: Drop override-replace-local-rpms~~ treefile: Drop override-replace-local-rpms and support install and overrides by URL in container path May 25, 2022

jlebon mentioned this pull request May 26, 2022

replace-systemd: new example coreos/layering-examples#25

Merged

lucab mentioned this pull request May 30, 2022

rust/libdnf: fix some implicit pointer conversions #3710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

treefile: Drop override-replace-local-rpms and support install and overrides by URL in container path #3670

treefile: Drop override-replace-local-rpms and support install and overrides by URL in container path #3670

jlebon commented May 10, 2022 •

edited

Loading

openshift-ci bot commented May 10, 2022

jlebon commented May 10, 2022

jlebon commented May 10, 2022

cgwalters commented May 10, 2022

jlebon commented May 11, 2022

cgwalters commented May 11, 2022

jlebon commented May 19, 2022 •

edited

Loading

cgwalters left a comment

cgwalters May 24, 2022

jlebon May 24, 2022

jlebon commented May 24, 2022

cgwalters May 25, 2022

cgwalters May 25, 2022

jlebon May 25, 2022

jlebon commented May 25, 2022

treefile: Drop override-replace-local-rpms and support install and overrides by URL in container path #3670

treefile: Drop override-replace-local-rpms and support install and overrides by URL in container path #3670

Conversation

jlebon commented May 10, 2022 • edited Loading

openshift-ci bot commented May 10, 2022

jlebon commented May 10, 2022

jlebon commented May 10, 2022

cgwalters commented May 10, 2022

jlebon commented May 11, 2022

cgwalters commented May 11, 2022

jlebon commented May 19, 2022 • edited Loading

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters May 24, 2022

Choose a reason for hiding this comment

jlebon May 24, 2022

Choose a reason for hiding this comment

jlebon commented May 24, 2022

cgwalters May 25, 2022

Choose a reason for hiding this comment

cgwalters May 25, 2022

Choose a reason for hiding this comment

jlebon May 25, 2022

Choose a reason for hiding this comment

jlebon commented May 25, 2022

jlebon commented May 10, 2022 •

edited

Loading

jlebon commented May 19, 2022 •

edited

Loading