containers / podman

Podman: A tool for managing OCI containers and pods.
https://podman.io
Apache License 2.0
22.37k stars 2.31k forks source link

Quadlet: add -list-images #23065

Closed vrothberg closed 4 days ago

vrothberg commented 1 week ago

Add a new -list-images string flag Quadlet which will write a list of images mentioned in .image Quadlets to specified file.

In order to avoid disk bloat, many bootable containers do not embed application-container images. Hence, those must be pulled in some use-case dependent way. Quadlet is perfectly capable of pulling the images on boot or on demand but that does not satisfy all use cases as it slows down boot or initialization of the workloads. In those cases, we want a bootc upgrade to first pull down the bootable container image and then analyze its root FS to extract all referenced images in (rootful) .image Quadlets. This change is a first step in this direction.

Does this PR introduce a user-facing change?

Add a `-list-images` flag to Quadlet to list images referenced in `.image` Quadlets.
openshift-ci[bot] commented 1 week ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/containers/podman/blob/main/OWNERS)~~ [vrothberg] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
vrothberg commented 1 week ago

@cgwalters, we also need another flag that alters the root / to something customizable. I can add that to this PR (or a follow-up one) but first wanted to make sure we're on the same page.

Luap99 commented 1 week ago

As for custom root I don't see why this is something quadlet/podman would need to handle, bootc can just spawn the quadlet process in the correct rootfs/mountns I think.

vrothberg commented 1 week ago

Why write to a file? Normal stdout should be all that is needed I think.

Writing to stdout would work as well but I found writing to a file to be more future-proof. In case debug or logs will be added in the future.

As for custom root I don't see why this is something quadlet/podman would need to handle, bootc can just spawn the quadlet process in the correct rootfs/mountns I think.

Yes, that would be an alternative.

cgwalters commented 1 week ago

As for custom root I don't see why this is something quadlet/podman would need to handle, bootc can just spawn the quadlet process in the correct rootfs/mountns I think.

There's positives and negatives to that approach. A slight negative is that it generally implies we'll be running a potentially new quadlet binary from the new root. So far we've generally avoided trying to execute code from the new root in general in bootc/ostree. The rationale is doing so increases "skew" possibilities where e.g. things in the new root actually expect the new kernel (think major OS upgrades, etc.)

In practice of course, the rise of containerization has meant that most of userspace ends up needing to have compat with maintained LTS/enterprise kernels; the chance that e.g. a new quadlet binary doesn't work on RHEL8's kernel when we're going from RHEL8 to 9 for example is probably close to nil, as it's basically just reading some files in Go.

Things will get more interesting though if we take the path (as I'd like to do) of also teaching quadlet to help "pin" these images by creating stopped containers for them. That will inherently involve a lot more of the "container runtime" code. But probably what would work is to just get the list from the new root, and then we can run the current root's podman to do pinning from there.

For these reasons, I personally would have made it so quadlet accepts a directory fd or starts from its own working directory; it's cheaper and easier to unit test.

Luap99 commented 1 week ago

For these reasons, I personally would have made it so quadlet accepts a directory fd or starts from its own working directory; it's cheaper and easier to unit test.

I guess given that we just read files it would be simple enough. However if we have to setup mount namespaces and such it gets very tricky with the goroutine != thread default behaviours.

rhatdan commented 1 week ago

Just a thought, but any reason we are not listing the Image field in a .Container quadlet?

ygalblum commented 1 week ago

You'll probably not agree with me, but I have to say I didn't like this solution when I started looking at this PR and reading @cgwalters's comments, I like it even less. Quadlet is a systemd generator. The only flags added to it are for debugging. Now, this PR starts piggybacking on it with additional requirements. I like that we try to maintain separation of concerns. Can't this be implemented in a separate executable?

vrothberg commented 1 week ago

Quadlet is a systemd generator. The only flags added to it are for debugging. Now, this PR starts piggybacking on it with additional requirements. I like that we try to maintain separation of concerns. Can't this be implemented in a separate executable?

I share your concern about the separation of concerns. I want to change Quadlet as little as possible. Listing .image files and extracting information in Quadlet seems more attractive to me than re-implementing it's parsing logic though; that's concern of Quadlet IMO. Let's see where the conversation goes.

ygalblum commented 1 week ago

First, a lot of the parsing is implemented in a separate package and what's not can probably be moved somewhere. So, we won't re-implement Quadlet just reuse some of the packages it uses.

An example of my concern is this line from the PR description:

Quadlet is perfectly capable of pulling the images on boot or on demand

This statement is incorrect. Quadlet does not pull images or run containers. What it does is translate the files it supports into systemd service unit files.

vrothberg commented 1 week ago

This statement is incorrect. Quadlet does not pull images or run containers. What it does is translate the files it supports into systemd service unit files.

That was just vagueness on my end, sorry. I am fully aware of how Quadlet works, so I meant the services generated by Quadlet can pull images.

cgwalters commented 1 week ago

One thing I'd leave open as a high level possibility here is adding Go to bootc...and if we do that we could choose in the short term to copy the quadlet code. Taking a step farther down, we could later try to factor it out into a shared package.

A lot of tradeoffs there of course...

ckyrouac commented 1 week ago

Can't this be implemented in a separate executable?

Given the desire to keep the scope of the quadlet binary to only be a systemd generator, this seems like the most sensible approach to me. Something like quadlet-utils or quadlet-cli. The primary downside to this approach is it will be yet another binary to maintain. I don't think that downside is too significant though.

cgwalters commented 1 week ago

How about a quick sync meeting on this to get rough consensus?

vrothberg commented 1 week ago

How about a quick sync meeting on this to get rough consensus?

Created one on Thursday :heavy_check_mark:

Luap99 commented 1 week ago

How about a quick sync meeting on this to get rough consensus?

Created one on Thursday ✔️

Please add me as well.


Overall this looks like something bootc specific that nobody else is going to use. I understand that you fine with good enough but I don't think it is good enough. Trying to mimic the systemd unit doesn't seem feasible. Saying systemd specifiers are not supported is not good and trying to reverse engineer systemd specifier expansion is not practical either. As such I really do not think this is practical outside the basic examples.

And to be honest if your goal is to pass the full files you might as well just use the existing quadlet -dryrun option and parse the output from there. That adds zero additional complexity to quadlet and you can just define what bootc supports or not on your side. The decision would not longer be up to quadlet/podman.

cgwalters commented 1 week ago

Overall this looks like something bootc specific that nobody else is going to use.

I think we can define something generic that would work with other image based update systems if that's your concern.

I guess bigger picture I'd been envisioning close cooperation between the podman and bootc projects at an organizational and technical level. But that's just my vision and I'm getting the feeling it's not yours :smile:

Backing up then, we can clearly not try to extend .image files at all (and more generally, not try to change podman or align) and introduce in bootc something like /usr/lib/bootc/images which could be a simple toml and not try to be unit file like for example, where bootc owns executing the code required to do the image pulling and pinning.

The user experience then would typically involve writing a foo.container podman unit, and a separate /usr/lib/bootc/images/foo.image file, distinct from the podman .image file. There would be a clear separation, reflecting the current clear separation between the two projects.

Luap99 commented 1 week ago

Overall this looks like something bootc specific that nobody else is going to use.

I think we can define something generic that would work with other image based update systems if that's your concern.

I guess bigger picture I'd been envisioning close cooperation between the podman and bootc projects at an organizational and technical level. But that's just my vision and I'm getting the feeling it's not yours 😄

I mean I haven't made my mind up yet regarding how much bootc and podman should or should not be integrated. My vision is to keep things as simple as possible for us so I tend to lean towards not adding special bootc things into podman/quadlet. I am also not a fan of half backed solutions. That doesn't mean I object out of principle, it would help to get our other maintainers involved as well in some larger meeting where you can lay out your long term vision of how things should work.

I am aware of https://github.com/containers/podman/issues/22785 but this only really touches on the one feature here.

cgwalters commented 1 week ago

I am also not a fan of half backed solutions.

No one does; let's keep this focused and technical though please and make clear what you think needs improvement so we can reach rough consensus and working code. I thought the specifiers was a valid concern, but you didn't reply to my specific assertion there that the use cases for specifiers for .image files was low to nonexistent.

That said, the more I think about this, I'm leaning towards something like https://github.com/containers/bootc/issues/640 (which is a lower level more general mechanism).

The key question though that would remain even after that is whether my initial intuition/suggestion that extending .image files for this use case makes sense. It feels natural I think if one starts from how the docs look today: https://docs.fedoraproject.org/en-US/bootc/running-containers/

But, I dunno; having /usr/lib/bootc/bound-images/foo.toml would honestly also probably be fine. And yes, there'd be no specifiers there since it wouldn't be a systemd unit-like file at all.

vrothberg commented 6 days ago

But, I dunno; having /usr/lib/bootc/bound-images/foo.toml would honestly also probably be fine. And yes, there'd be no specifiers there since it wouldn't be a systemd unit-like file at all.

I begin to gravitate toward such a file as well. We can discuss details in the meeting tomorrow but it feels like a nice generic solution that can also tackle non-Quadlet use cases.

Luap99 commented 6 days ago

No one does; let's keep this focused and technical though please and make clear what you think needs improvement so we can reach rough consensus and working code.

Well the fact the we cannot expand systemd specifiers is amjpr downside to me, it would also be unable to deal with any ExecStartPre/Post lines that, i.e. could be used to generate auth tokens... I agree that a mjaority of users would likely not need that but to me this just doesn't sound like good idea to commit to this just to then get complains from users relying on these things. If I would see a way for us to implement it in the future I wouldn't object but to me it seems impossible to reverse engineer the way system would call the command without actually letting system execute it.

But, I dunno; having /usr/lib/bootc/bound-images/foo.toml would honestly also probably be fine. And yes, there'd be no specifiers there since it wouldn't be a systemd unit-like file at all.

Adding separate file sounds good to me. I guess that could also be useful for the "pinning" thing where we could have a podman config option to read this file and not remove the images listed there. Of course end users would need to deal with the duplicating in image names between the quadlet file and this special file but I don't think that would be a big issue.

rhatdan commented 6 days ago

Having the separate file would also help in the situations where quadlets only use .container or .kube to pull images.

I am not sure how/If something like MicroShift would preinstall container images as well.

vrothberg commented 6 days ago

Agreed. There are use cases even outside of Quadlet which could benefit from a more generic solution.

cgwalters commented 6 days ago

I am not sure how/If something like MicroShift would preinstall container images as well.

This is a tangent, but an interesting one: today Microshift using ostree was explicitly relying on the fact that ostree never itself used whiteouts! I brought this up in https://github.com/ostreedev/ostree/issues/2712

Now that we're using OCI on the wire, this concept of nesting (as you know) comes to the fore.

Also another tangent, I think it doesn't make sense to do RUN podman pull ... even for "physically" bound images, we should support a flow that basically takes the underlying layers, and renames all the files, plus a bit that adds all the metadata as a final layer - this would help ensure that we never re-pull unchanged layers even for "physically" bound images.

IOW it'd look like

[base image layer 1]
[base image layer 2]
...
[embedded content layer 1, but with all files included renamed to prefix with /usr/share/containers/storage/overlay/<blobid>]
...
[embedded layer with everything else in /usr/share/containers/storage *except* the layers]
...
cgwalters commented 6 days ago

Whether microshift would want to switch from "physically bound" to "logically bound" images is an interesting topic, I don't know the answer. I think it'd make the most sense to do logically bound for microshift personally.

ckyrouac commented 6 days ago

It seems like a separate file would technically work well, I still have a hard time accepting a degraded user experience (even relatively minor). A separate file would mean the user would need to potentially duplicate all of these options to define the image. This can obviously be mitigated with tooling, still these minor inconveniences tend to add up.

In lieu of adding a bootc-bound-image field to the quadlet spec, another option we might consider is storing the quadlet image file in a subdirectory, e.g. /etc/containers/systemd/bootc-bound/my-app.image or something. Any image files in this directory would be considered bound to the bootc image. Then, bootc would call QUADLET_UNIT_DIRS=/etc/containers/systemd/bootc /usr/lib/systemd/system-generators/podman-system-generator to generate the systemd service to pull the image. In the case where the image is only defined on the container quadlet, we would require it to also be defined in an image quadlet in the bootc-bound dir. AFAICT the container spec doesn't support the entire set of options in the image spec, so this would not result in a lot of duplicated config.

You all are way more familiar with these projects than me so I'm sure I'm missing something here though.

cgwalters commented 6 days ago

@baude any opinions on this?

cgwalters commented 5 days ago

We had a realtime sync on this...which was wide ranging and interesting. Hopefully someone else can chime in with a broader summary but I think my takeaway-proposal here is:

Let's add a special /usr/lib/bootc/bound-images (in toml format say) for now, implement it entirely in bootc (leveraging https://github.com/containers/bootc/issues/640 internally) and revisit later larger picture designs for how image "binding" in podman/containers stack might look like in general.

A good start here on the bootc side would just be:

That'd force us to work though associated problems like the bootc-image-builder bug I hit, thinking about how pull secrets work at install time too, etc. It'd even be generally useful of course to support pre-fetching at install time without also doing "binding"; something to consider in the bootc config format. For example we might have Mode=prefetch vs Mode=bind.

vrothberg commented 4 days ago

Based on the conversations, I am going to close this PR.