rust/src/client: Use microdnf when installing pkgs on a container #3280

jmarrero · 2021-12-16T05:36:37Z

proof of concept, current version works. Example Dockerfile:

FROM localhost/myfcos
RUN mkdir /tmp
ADD rpm-ostree /usr/bin/

RUN rpm-ostree install htop
RUN rpm-ostree install zsh strace

openshift-ci · 2021-12-16T05:36:38Z

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

Thanks for working on this!

cgwalters · 2021-12-16T13:54:48Z

rust/src/client.rs

+        packages = packages.replace(" ", "");
+    }
+    let microdnf_status = Command::new("microdnf")
+        .args(&["install", &packages, "-y"]).status().expect("No package matches");


Let's use ? here instead of .expect().

A failure in this path would happen when we can't even execute the binary (e.g. it's missing) - not that it fails at runtime.

Either way, we shouldn't abort the process, but just return a regular error.

On another topic, the use of -y here is of course interesting...this relates heavily to #2883 (comment)

The thing is, because we are mutating the running root live (even if in a container) it is kind of tempting to me to say we should add rpm-ostree install -y, and pass through that option down into microdnf if present.

What would be the behavior of -y for non container systems? Do we ignore it or we change rpm-ostree to be interactive without it and need user ack before proceeding?

I see your comment on using it with -A but if we wanted 1:1 mapping i.e. microdnf install -y = rpmostree install -y since we already don't have user interaction after rpm-ostree install pkg1 not sure what we accomplish for the user experience. If I want to call rpm-ostree on my build like I do on my system then not requiring -y and just adding it in the background seems to be a consistent experience for the user.

src/app/libmain.cxx

rust/src/client.rs

src/app/libmain.cxx

I plan to set this up to run as part of our Prow builds to help test coreos#3280

cgwalters · 2021-12-16T22:04:01Z

I'm setting up some CI scaffolding for this here #3282

Once that lands and I do the PR for openshift/release, then this PR can adjust that code to do basically: mkdir -p /sysroot/ostree/repo to fake things out for now, then validate that package installs work.

(Hmm, and I guess we'll need to deal with the microdnf dependency too which actually breaks the hack of reusing coreos-assembler for this...)

jmarrero · 2021-12-17T02:46:48Z

I'm setting up some CI scaffolding for this here #3282

Once that lands and I do the PR for openshift/release, then this PR can adjust that code to do basically: mkdir -p /sysroot/ostree/repo to fake things out for now, then validate that package installs work.

(Hmm, and I guess we'll need to deal with the microdnf dependency too which actually breaks the hack of reusing coreos-assembler for this...)

You mean creating the fake path only for CI? We can't just add a buildconfig for testing this flow? The current checks seem to work now. I might be seeing the full picture of why we need that fake path created.

cgwalters

Cool, this is looking pretty good to me. Let's just do that prep PR split for the ostree-ext bump.

cgwalters · 2021-12-16T21:53:59Z

rust/src/client.rs

@@ -235,6 +235,15 @@ pub(crate) fn client_render_download_progress(
    }
 }

+pub(crate) fn microdnf_install(args:  Vec<String>) -> Result<()> {
+    let microdnf_status = Command::new("microdnf")
+        .arg("install").args(&args).arg("-y").status()?; 


Minor nit, let's put -y before the package list. I don't think it matters, but it's a bit more normal.

cgwalters · 2021-12-17T14:45:43Z

Cargo.toml

@@ -40,7 +40,7 @@ envsubst = "0.2.0"
 env_logger = "0.9.0"
 fail = { version = "0.5", features = ["failpoints"] }
 fn-error-context = "0.2.0"
-futures = "0.3.18"
+futures = "0.3.17"


Is this a result of a merge conflict? I think downgrading should have a strong rationale.

Yeah, I just made it build, not trying to propose this as the final push. But I made ostree-ext point to git and called: cargo update which started complaining about 0.3.18. Moving it down to 0.3.17 made it build.

Cargo.lock

cgwalters · 2021-12-17T14:50:59Z

The current checks seem to work now.

Right, but (unless I'm missing something) we aren't testing the new code here, right?

EDIT: To be clear, I don't think we need to block this PR on testing the new code, but we should have it as a priority soon after merging at least.

cgwalters · 2021-12-17T14:51:47Z

Huh, from CI:

[2021-12-17T03:35:52.199Z] error: Importing 2c06545acdbd78911b267874a55130c5bc3515b8e318a8d04f138ee3721f79fd.dirmeta: linkat: No such file or directory

That's...interesting. I'm suspecting it's fallout from ostreedev/ostree-rs-ext#186

Looking

cgwalters · 2021-12-17T15:00:00Z

/test all

cgwalters · 2021-12-17T15:05:25Z

Yep reproduced locally with:

alters@toolbox /v/s/w/b/fcos> ostree-ext-cli container encapsulate --repo=tmp/repo 35.20211215.dev.4 oci:tmp/fcos.oci
sha256:ca97c16e34f5d2b8c2e5f53ba73f92bf197d7765a05b8d5c94423f12c12071ac
walters@toolbox /v/s/w/b/fcos> ostree-ext-cli container unencapsulate --repo=tmp/repo2 oci:tmp/fcos.oci
error: Invalid value for '<imgref>': Invalid ostree image reference scheme: oci
walters@toolbox /v/s/w/b/fcos [1]> ostree-ext-cli container unencapsulate --repo=tmp/repo2 ostree-unverified-image:oci:tmp/fcos.oci
   Processed: 1.35GiB
Imported: 283381d94d0a4414dc1c3817586591a4e52cddaf8e4569d0405b9c50831d9a1e
walters@toolbox /v/s/w/b/fcos> ostree --repo=tmp/repo2 refs
walters@toolbox /v/s/w/b/fcos> ostree --repo=tmp/repo2 ls -R 283381d94d0a4414dc1c3817586591a4e52cddaf8e4569d0405b9c50831d9a1e
d00755 0 0      0 /
l00777 0 0      0 /bin -> usr/bin
l00777 0 0      0 /home -> var/home
l00777 0 0      0 /lib -> usr/lib
l00777 0 0      0 /lib64 -> usr/lib64
l00777 0 0      0 /media -> run/media
l00777 0 0      0 /mnt -> var/mnt
l00777 0 0      0 /opt -> var/opt
l00777 0 0      0 /ostree -> sysroot/ostree
l00777 0 0      0 /root -> var/roothome
l00777 0 0      0 /sbin -> usr/sbin
l00777 0 0      0 /srv -> var/srv
d00755 0 0      0 /boot
d00755 0 0      0 /dev
d00755 0 0      0 /proc
d00755 0 0      0 /run
d00755 0 0      0 /sys
error: No such metadata object 2c06545acdbd78911b267874a55130c5bc3515b8e318a8d04f138ee3721f79fd.dirmeta
walters@toolbox /v/s/w/b/fcos [1]>

jmarrero · 2021-12-17T15:11:57Z

EDIT: To be clear, I don't think we need to block this PR on testing the new code, but we should have it as a priority soon after merging at least.

I can add the tests today on this PR, that is no issue. I was just giving an update on that the current code worked to me locally. I can also expect the microdnf bin to be on /usr/lib/rpm-ostree like it was suggested here: coreos/fedora-coreos-tracker#1050 (comment)

cgwalters · 2021-12-17T15:40:38Z

PR in ostreedev/ostree-rs-ext#191

cgwalters

Nice, thanks for this!

Not a blocker (I'm going to click auto-merge) but let's try to get in the habit of adding at least brief documentation comments.

rust/src/client.rs

cgwalters · 2021-12-17T20:42:06Z

Looks like this needs a cargo fmt

Co-authored-by: Colin Walters <[email protected]>

jmarrero · 2021-12-17T20:51:22Z

Looks like this needs a cargo fmt

sorry, forgot to run it. Should be set now.

jmarrero · 2021-12-17T20:53:55Z

Thank you for all the help and mentoring with thru this @cgwalters

This will be used for an integration test once we rebuild our current container image. But also, this will help out cases like coreos/rpm-ostree#3280 As well as supporting finalization inside a container.

openshift-ci bot added the do-not-merge/work-in-progress label Dec 16, 2021

cgwalters reviewed Dec 16, 2021

View reviewed changes

cgwalters mentioned this pull request Dec 16, 2021

New Package Request: microdnf - non user facing change coreos/fedora-coreos-tracker#1050

Closed

jmarrero commented Dec 16, 2021

View reviewed changes

src/app/libmain.cxx Show resolved Hide resolved

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Dec 16, 2021

ci: Add new e2e-as-container script

6c5a271

I plan to set this up to run as part of our Prow builds to help test coreos#3280

cgwalters mentioned this pull request Dec 16, 2021

ci: Add new e2e-as-container script #3282

Closed

jmarrero force-pushed the install-on-oci-build branch from 14f5f8c to 90d94bd Compare December 17, 2021 02:24

jmarrero changed the title ~~microdnf call builds and installs~~ rpmostreecxx::running_in_container() && rpmostreecxx::ostree_path_exists() Dec 17, 2021

jmarrero changed the title ~~rpmostreecxx::running_in_container() && rpmostreecxx::ostree_path_exists()~~ rust/src/client: Use microdnf when installing pkgs on a container Dec 17, 2021

jmarrero force-pushed the install-on-oci-build branch from 90d94bd to f3c6e3f Compare December 17, 2021 02:36

cgwalters reviewed Dec 17, 2021

View reviewed changes

cgwalters mentioned this pull request Dec 17, 2021

tar/export: Do emit dirmeta for /sysroot ostreedev/ostree-rs-ext#191

Merged

jmarrero mentioned this pull request Dec 17, 2021

Cargo.toml: Update ostree-ext to use main branch code #3283

Merged

jmarrero force-pushed the install-on-oci-build branch from f3c6e3f to 2fe22a4 Compare December 17, 2021 20:34

jmarrero marked this pull request as ready for review December 17, 2021 20:35

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 17, 2021

jmarrero requested a review from cgwalters December 17, 2021 20:35

cgwalters previously approved these changes Dec 17, 2021

View reviewed changes

rust/src/client.rs Show resolved Hide resolved

jmarrero dismissed cgwalters’s stale review via 79374b1 December 17, 2021 20:43

rust/src/client: Use microdnf when installing pkgs on a container

f2f529d

Co-authored-by: Colin Walters <[email protected]>

jmarrero force-pushed the install-on-oci-build branch from 6c75f25 to f2f529d Compare December 17, 2021 20:48

jmarrero requested a review from cgwalters December 17, 2021 20:50

cgwalters approved these changes Dec 17, 2021

View reviewed changes

cgwalters enabled auto-merge (rebase) December 17, 2021 20:51

jmarrero mentioned this pull request Dec 17, 2021

Dockerfile: use rpm-ostree to install strace coreos/layering-examples#1

Merged

cgwalters mentioned this pull request Dec 17, 2021

tar/export: Add a repo/config with mode bare-split-xattrs ostreedev/ostree-rs-ext#193

Merged

cgwalters merged commit a30dec3 into coreos:main Dec 17, 2021

cgwalters mentioned this pull request Jan 24, 2022

Support rpm-ostree install in container builds #3227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust/src/client: Use microdnf when installing pkgs on a container #3280

rust/src/client: Use microdnf when installing pkgs on a container #3280

jmarrero commented Dec 16, 2021 •

edited

Loading

openshift-ci bot commented Dec 16, 2021

cgwalters left a comment

cgwalters Dec 16, 2021

cgwalters Dec 16, 2021

jmarrero Dec 17, 2021

cgwalters commented Dec 16, 2021

jmarrero commented Dec 17, 2021 •

edited

Loading

cgwalters left a comment

cgwalters Dec 16, 2021

cgwalters Dec 17, 2021

jmarrero Dec 17, 2021

cgwalters commented Dec 17, 2021 •

edited

Loading

cgwalters commented Dec 17, 2021

cgwalters commented Dec 17, 2021

cgwalters commented Dec 17, 2021

jmarrero commented Dec 17, 2021

cgwalters commented Dec 17, 2021

cgwalters left a comment

cgwalters commented Dec 17, 2021

jmarrero commented Dec 17, 2021 •

edited

Loading

jmarrero commented Dec 17, 2021

rust/src/client: Use microdnf when installing pkgs on a container #3280

rust/src/client: Use microdnf when installing pkgs on a container #3280

Conversation

jmarrero commented Dec 16, 2021 • edited Loading

openshift-ci bot commented Dec 16, 2021

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Dec 16, 2021

Choose a reason for hiding this comment

cgwalters Dec 16, 2021

Choose a reason for hiding this comment

jmarrero Dec 17, 2021

Choose a reason for hiding this comment

cgwalters commented Dec 16, 2021

jmarrero commented Dec 17, 2021 • edited Loading

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters Dec 16, 2021

Choose a reason for hiding this comment

cgwalters Dec 17, 2021

Choose a reason for hiding this comment

jmarrero Dec 17, 2021

Choose a reason for hiding this comment

cgwalters commented Dec 17, 2021 • edited Loading

cgwalters commented Dec 17, 2021

cgwalters commented Dec 17, 2021

cgwalters commented Dec 17, 2021

jmarrero commented Dec 17, 2021

cgwalters commented Dec 17, 2021

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters commented Dec 17, 2021

jmarrero commented Dec 17, 2021 • edited Loading

jmarrero commented Dec 17, 2021

jmarrero commented Dec 16, 2021 •

edited

Loading

jmarrero commented Dec 17, 2021 •

edited

Loading

cgwalters commented Dec 17, 2021 •

edited

Loading

jmarrero commented Dec 17, 2021 •

edited

Loading