install: add --source-imgref #263

ondrejbudai · 2024-01-16T22:46:28Z

blockdev: fix recognizing parent loop devices

lsblk returns TYPE == loop for loop devices, modify the code
so loop devices are correctly recognized.

install: make the source digest optional

This is a prep work for the following commit. It will implement pulling
the container image from an explicitly given container reference. Thus,
there's no need to always fetch the digest because the caller is being
explicit about the container image, so let's make the field optional.

install: add --source-imgref

bootc install and install-to-filesystem currently rely on the fact that they
run inside a podman container. That's quite inconvenient for using bootc
for osbuild, because osbuild already run everything in a container.
While having a container in a container is surely possible, it gets quite
messy.

Instead of going this route, this commit implements a new --source-imgref
argument. --source-imgref accepts a container image reference (the same one
that skopeo uses). When --source-imgref is used, bootc doesn't escape the
container to fetch the container image from host's container storage. Instead,
the container image given by --source-imgref is used.

Even when running in this mode, bootc needs to run in a container created from
the same container image that is passed using --source-imgref. However, this
isn't a problem to do in osbuild. This really just removes the need for bootc
to escape the container to the host mount namespace.

cgwalters

Cool, thanks for this! Overall mostly LGTM, just some nits and optionals.

cgwalters · 2024-01-16T23:01:21Z

lib/src/blockdev.rs

@@ -197,7 +197,7 @@ pub(crate) fn find_parent_devices(device: &str) -> Result<Vec<String>> {
        let kind = dev
            .get("TYPE")
            .with_context(|| format!("device in hierarchy of {device} missing TYPE"))?;
-        if kind == "disk" {
+        if kind == "disk" || kind == "loop" {


Ah yes. It would probably make sense to try to use a shared crate for dealing with block devices at some point.

cgwalters · 2024-01-16T23:03:58Z

lib/src/skopeo.rs

+}
+
+/// Given an image ID, return its manifest digest
+pub(crate) fn oci_archive_digest(imageref: &str) -> Result<String> {


This isn't specific to oci archives right? If we want it to be, let's make this a &Utf8Path, and then explicitly specify dir: below say.

If we want to keep it general...we also already use https://docs.rs/containers-image-proxy/latest/containers_image_proxy/ which is a handy Rust client for talking to skopeo, but I don't really in the end have a problem with being practical and just forking it either.

(Forgot to mention, see below where we could also probably drop this code)

Dropped (see below).

cgwalters · 2024-01-16T23:06:36Z

lib/src/install.rs

+            } else {
+                let td = tempfile::tempdir_in("/var/tmp")?;
+                let path: &Utf8Path = td.path().try_into().unwrap();
+                let r = copy_to_oci(&state.source.imageref, path)?;


(Unrelated to this change, but I would probably be fine just hard dropping this hacky "copy to oci" code that I don't think we need anymore since c9s has a new enough skopeo and that's fine from my PoV)

Totally optional of course...just reminded by the seeing the code again here. If you want I can also do a prep PR removing it (or feel free to yourself), it seems like it would simplify this change.

Ended up doing this in #265 - but I still don't have a really strong opinion - we can merge this before that one if you prefer too.

No objections from my side. #265 has a failing test suite currently, though. I think that rebasing #265 on top of this PR might be easier, so I prefer landing this one first. I must admit that it's a tad bit selfish view, though. 👼

cgwalters · 2024-01-16T23:24:23Z

lib/src/install.rs

+
+    #[context("Gathering source info from an external source")]
+    pub(crate) fn from_imageref(imageref: &str) -> Result<Self> {
+        let digest = crate::skopeo::oci_archive_digest(imageref)?;


Today, the ostree-container stack very much handles just being passed a non-digested pull spec; we'll do the digest resolution there. An alternative here is to make digest an Option<T> instead. I have a mild preference for this, but I'm also fine with the code as is.

I hope I understood your comment correctly. I made digest an Option<T>. This loses a bit of "null-safety" (handled gracefully, ofc), but it allowed me to drop the whole skopeo crate. I have a backup of the old code if we change our midns.

Let's just hard require a skopeo that can fetch from `containers-storage`. Motivated by containers#263 which was moving this code around. Signed-off-by: Colin Walters <[email protected]>

vrothberg

I'd love to see docs on that as well. Which containers-transports are supported? I feel that the name --source is so generic that different users/personas will have a very different interpretation/feeling for that a source may be.

lsblk returns TYPE == loop for loop devices, modify the code so loop devices are correctly recognized. Signed-off-by: Ondřej Budai <[email protected]>

This is a prep work for the following commit. It will implement pulling the container image from an explicitly given container reference. Thus, there's no need to always fetch the digest because the caller is being explicit about the container image, so let's make the field optional. Signed-off-by: Ondřej Budai <[email protected]>

ondrejbudai · 2024-01-17T10:03:42Z

I'd love to see docs on that as well. Which containers-transports are supported? I feel that the name --source is so generic that different users/personas will have a very different interpretation/feeling for that a source may be.

I renamed --source to --source-imgref, and added some documentation about it. Does it work for you?

cgwalters

LGTM, just one final nit, feel free to merge after addressing!

docs/install.md

cgwalters · 2024-01-17T12:20:09Z

lib/src/install.rs

+    /// it takes the container image to install from the podman's container registry.
+    /// If --source-imgref is given, bootc uses it as the installation source, instead of the behaviour explained
+    /// in the previous paragraph. See skopeo(1) for accepted formats.
+    #[clap(long, hide = true)]


But documenting seems to clash with hide = true. Either it's something we support ~forever or it's a hidden thing that could be removed in theory. I guess given the state, we should probably lift hide = true right?

I removed hide = true.

docs/install.md

bootc install and install-to-filesystem currently rely on the fact that they run inside a podman container. That's quite inconvenient for using bootc for osbuild, because osbuild already run everything in a container. While having a container in a container is surely possible, it gets quite messy. Instead of going this route, this commit implements a new --source-imgref argument. --source-imgref accepts a container image reference (the same one that skopeo uses). When --source-imgref is used, bootc doesn't escape the container to fetch the container image from host's container storage. Instead, the container image given by --source-imgref is used. Even when running in this mode, bootc needs to run in a container created from the same container image that is passed using --source-imgref. However, this isn't a problem to do in osbuild. This really just removes the need for bootc to escape the container to the host mount namespace. Signed-off-by: Ondřej Budai <[email protected]>

Let's just hard require a skopeo that can fetch from `containers-storage`. Motivated by containers#263 which was moving this code around. Signed-off-by: Colin Walters <[email protected]>

github-actions bot added the area/install Issues related to `bootc install` label Jan 16, 2024

ondrejbudai force-pushed the source-arg branch 4 times, most recently from c96d517 to 276c68a Compare January 16, 2024 22:52

cgwalters reviewed Jan 16, 2024

View reviewed changes

cgwalters mentioned this pull request Jan 17, 2024

install: Drop support for old skopeo #265

Merged

vrothberg reviewed Jan 17, 2024

View reviewed changes

blockdev: fix recognizing parent loop devices

c3d595c

lsblk returns TYPE == loop for loop devices, modify the code so loop devices are correctly recognized. Signed-off-by: Ondřej Budai <[email protected]>

ondrejbudai force-pushed the source-arg branch from 276c68a to e63c3b1 Compare January 17, 2024 09:58

github-actions bot added the documentation Improvements or additions to documentation label Jan 17, 2024

ondrejbudai force-pushed the source-arg branch from e63c3b1 to 5e4c639 Compare January 17, 2024 09:59

ondrejbudai changed the title ~~install: add --source~~ install: add --source-imgref Jan 17, 2024

ondrejbudai mentioned this pull request Jan 17, 2024

[meta] require approvals for PRs #266

Closed

cgwalters previously approved these changes Jan 17, 2024

View reviewed changes

ondrejbudai dismissed cgwalters’s stale review via 1db3040 January 17, 2024 12:22

ondrejbudai force-pushed the source-arg branch from 5e4c639 to 1db3040 Compare January 17, 2024 12:22

vrothberg reviewed Jan 17, 2024

View reviewed changes

docs/install.md Outdated Show resolved Hide resolved

ondrejbudai force-pushed the source-arg branch from 1db3040 to 7c3ed0b Compare January 17, 2024 12:28

cgwalters enabled auto-merge January 17, 2024 12:48

cgwalters approved these changes Jan 17, 2024

View reviewed changes

cgwalters merged commit 0910876 into containers:main Jan 17, 2024
11 checks passed

ondrejbudai deleted the source-arg branch January 17, 2024 13:28

ondrejbudai restored the source-arg branch January 17, 2024 13:28

mvo5 mentioned this pull request Jan 19, 2024

stages: add new org.osbuild.bootc.install-to-filesystem osbuild/osbuild#1547

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

install: add --source-imgref #263

install: add --source-imgref #263

ondrejbudai commented Jan 16, 2024 •

edited

Loading

cgwalters left a comment

cgwalters Jan 16, 2024

cgwalters Jan 16, 2024

cgwalters Jan 16, 2024

ondrejbudai Jan 17, 2024

cgwalters Jan 16, 2024

cgwalters Jan 17, 2024

ondrejbudai Jan 17, 2024

cgwalters Jan 16, 2024

ondrejbudai Jan 17, 2024

vrothberg left a comment

ondrejbudai commented Jan 17, 2024

cgwalters left a comment

cgwalters Jan 17, 2024

ondrejbudai Jan 17, 2024

install: add --source-imgref #263

install: add --source-imgref #263

Conversation

ondrejbudai commented Jan 16, 2024 • edited Loading

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

ondrejbudai commented Jan 17, 2024

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejbudai commented Jan 16, 2024 •

edited

Loading