Replace `--local` with `--pull=[always,never,missing]` (HMS-3792) #418

kingsleyzissou · May 3, 2024

This is a small POC to see how we can switch from using the --local flag to --pull which is both more descriptive and more inline with how podman behaves. This ~~is a breaking change~~ changes the existing behaviour, but it makes sense as it feels more intuitive and feels better from UX point-of-view.

This PR introduces 3 cases:

always, which always pulls the container image
missing, only pulls the container image when it is missing
never, don't pull the image (which is the same as the previous --local flag)

cgwalters · May 3, 2024

This is a breaking change,

Is it? Looks to me like you're just deprecating --local instead of breaking it, which seems fine.

kingsleyzissou · May 3, 2024

Valid point

achilleas-k · May 3, 2024

Yeah, this is nice. Gives the user more control and it aligns with what we're doing internally (and how podman works) so it should be easier to understand.

mvo5

Thank you! Some quick drive-by ideas inline, I would also love to see a test but we can chat next week, the existing test_opts.py hopefully makes this straightforward (but I need to double check :)

bib/cmd/bootc-image-builder/main.go

In order to be more consistent with how podman behaves, add initial `pull=never` and `pull=always` flags.

Deprecate the `--local` flag in favour of the `--pull` flag as the latter is more descriptive and is more inline with how podman behaves. Add a deprecated warning message for using the `--local` flag.

Add the `pull=missing` flag and set it as the default. Add the logic to check if the container is missing from local storage and pull the image if it is not there. Note: it is a little difficult to check for cross-arch images if a container already exists for the host's architecture. If the image exists and the container arch doesn't match, just pull the image.

Replace all instances of the `--local` flag in the test suites with `--pull=never` flag.

Update the README to replace the deprecated `--local` flag with the `--pull=always` flag to ensure that the image has been pulled into the local containers storage. Add a note to explain the 3 different options for the pull flag, namely: - `always`, which always pulls the container image - `missing`, only pulls the container image when it is missing - `never`, don't pull the image (same behaviour as the --local flag)

rhatdan · May 4, 2024

README.md

    quay.io/centos-bootc/bootc-image-builder:latest \
    --type qcow2 \
-    --local \
+    --pull=always \ # Ensure the image is fetched


PLEASE make this missing not always.

rhatdan · May 4, 2024

README.md

+The `--pull` flag is based on the behaviour of `podman-build(1)` and has the following three options:
+- `--pull=always`, always pull the remote image
+- `--pull=missing`, only pull the image if it is not in local storage
+- `--pull=never`, don't pull the image (this is functionally the same as the deprecated `--local` flag)


Podman build also supports newer.

$ podman run --help | grep -- --pull
--pull string Pull image policy ("always"|"missing"|"never"|"newer") (default "missing")

$ podman build --help | grep -- --pull
--pull string[="true"] Pull image policy ("always/true"|"missing"|"never/false"|"newer") (default "missing")

rhatdan · May 4, 2024

bib/cmd/bootc-image-builder/main.go

+
+func newPullPolicy(s string) (pullPolicy, error) {
+	switch s {
+	case "always", "never":


What about missing and newer?

Thanks good catch re missing. We still need to add more tests and figuring out how to approach newer

rhatdan · May 4, 2024

test/test_build.py

            *upload_args,
            *target_arch_args,
-            "--local" if local else "--local=false",
+            "--pull=never" if local else "--pull=always",


sed s/always/missing/g

cgwalters · May 4, 2024

OK this topic is subtle and confusing. I am coming around to the PoV that the only viable solution is to change all the documentation for bib to say:

podman pull <image>  # If not present
podman run quay.io/bootc-image-builder ... <image>

And we should not expose --pull as an argument to bib itself. Here's why:

While today bib is invoking podman pull - it's doing so inside the container context itself. In a scenario like Podman Machine on macOS - the default thing that happens when the user types podman is that the pull secret is passed via remoting from the client to the Linux VM. The pull secret is sitting out there on the user's home directory, and is not present in the "system context" in the Linux VM except via the virtiofs/9p bind mount.

Trying to replicate the effect of that remoting work from inside is of course technically possible, but I suspect is going to be quite fiddly.

Also as I've been arguing in a few contexts for a while, IMO one of the primary entrypoints to bib should be users building their own derived containers they used locally - for which there's no need to pull at all.

The case of building a "remote" image of course definitely exists - in fact it's very much a "production" use case I think for e.g. having one build pipeline (Tekton, Actions, etc.) churn out container images, and then a distinct pipeline asynchronously generating disk images by polling the registry (cc #162 ).

But anyways there again - it's simple, the pipeline itself just does a podman pull of the image before invoking bib.

cgwalters · May 4, 2024

Or to say ⬆️ another way: all this "bib controlling pulls" superficially appears to work except when dealing with images that require authentication, in which case it's all just broken. And we shouldn't make the authenticated case feel broken.

cgwalters

Marking as requested-changes unless someone can argue against the authenticated image problem.

ondrejbudai · May 6, 2024

Just to zoom out a bit: The core problem is that we want to do authenticated pulls on the host from a container (bib). The usual pattern used in the wild is to mount the docker/podman socket inside your application container and control the host's container engine remotely. See e.g. watchtower.

On the other hand, the socket approach isn't that common in the podman world (except for remote use-cases), it feels cumbersome to set up, and I actually don't know at all how it would work in case of podman machine.

Additionally, bib doesn't only need to pull images, but it also needs access to the image's underlying layers, which means that even if wanted to experiment with using a socket, we would still need to mount the storage inside.

I just wanted to mention sockets for the sake of completeness, but I fully acknowledge that this is not the right way.

I think that having --pull=XYZ has some benefits (I love one-liners), but I do agree that establishing the following pattern might be better long-term, as there's just one mental path for everything.

podman pull <image>  # If not present
podman run quay.io/bootc-image-builder ... <image>

achilleas-k · May 6, 2024

FWIW, I like the simplicity of limiting the behaviour of BIB to local containers only (no pulling ever) and letting the user take care of having the containers locally, either by building them or pulling them ahead of time. It reduces the complexity of the project as well as the mental load for users trying to wrap their heads around what BIB is doing when pulling, whether it needs authentication or not, etc.

cgwalters · May 6, 2024

The usual pattern used in the wild is to mount the docker/podman socket inside your application container and control the host's container engine remotely. See e.g. watchtower.

I don't think interacting with the socket helps, it gives the same semantics as just running the podman CLI.

Things like watchtower will generally assume that you've configured the pull secret for the container image globally for the container engine, AFAICS.

The problem is the Podman-machine case again; if you follow the default developer UX of running e.g. podman login + podman pull etc. on the host system, it will not work to do podman machine ssh and do podman pull from there, because the auth file is on the host and podman-in-vm doesn't know about it.

rhatdan · May 8, 2024

I agree simplify BIB and only work on local storage. Force the caller to pull the image first or build the image locally.

kingsleyzissou mentioned this pull request May 3, 2024

README: update local storage docs #303

Closed

achilleas-k mentioned this pull request May 3, 2024

README.md: Link upstream docs, default to --local #415

Merged

kingsleyzissou force-pushed the bib-pull branch 5 times, most recently from ab2f6af to e2da71f Compare May 3, 2024 14:39

kingsleyzissou changed the title ~~Replace --local with --pull=[always,never,missing]~~ Replace --local with --pull=[always,never,missing] (HMS-3792) May 3, 2024

kingsleyzissou requested review from achilleas-k, mvo5 and ondrejbudai May 3, 2024 14:40

kingsleyzissou marked this pull request as ready for review May 3, 2024 14:41

mvo5 reviewed May 3, 2024

View reviewed changes

bib/cmd/bootc-image-builder/main.go Outdated Show resolved Hide resolved

bib/cmd/bootc-image-builder/main.go Outdated Show resolved Hide resolved

kingsleyzissou added 5 commits May 3, 2024 16:24

bib: add pull=always & pull=never flags

964d991

In order to be more consistent with how podman behaves, add initial `pull=never` and `pull=always` flags.

bib: deprecate --local for --pull

624852c

Deprecate the `--local` flag in favour of the `--pull` flag as the latter is more descriptive and is more inline with how podman behaves. Add a deprecated warning message for using the `--local` flag.

tests: update tests to use pull flag

1b23959

Replace all instances of the `--local` flag in the test suites with `--pull=never` flag.

kingsleyzissou force-pushed the bib-pull branch from e2da71f to c39d3d5 Compare May 3, 2024 15:26

rhatdan reviewed May 4, 2024

View reviewed changes

cgwalters requested changes May 4, 2024

View reviewed changes

kingsleyzissou closed this May 10, 2024

cgwalters mentioned this pull request Jul 23, 2024

install: Fetch bound images from host's /var/lib/containers bootc-dev/bootc#719

Merged

Search code, repositories, users, issues, pull requests...

Replace --local with --pull=[always,never,missing] (HMS-3792) #418

Replace --local with --pull=[always,never,missing] (HMS-3792) #418

Uh oh!

Conversation

kingsleyzissou commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented May 3, 2024

Uh oh!

kingsleyzissou commented May 3, 2024

Uh oh!

achilleas-k commented May 3, 2024

Uh oh!

mvo5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rhatdan May 4, 2024

Choose a reason for hiding this comment

Uh oh!

rhatdan May 4, 2024

Choose a reason for hiding this comment

Uh oh!

rhatdan May 4, 2024

Choose a reason for hiding this comment

Uh oh!

rhatdan May 4, 2024

Choose a reason for hiding this comment

Uh oh!

kingsleyzissou May 4, 2024

Choose a reason for hiding this comment

Uh oh!

rhatdan May 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters commented May 4, 2024

Uh oh!

cgwalters commented May 4, 2024

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

ondrejbudai commented May 6, 2024

Uh oh!

achilleas-k commented May 6, 2024

Uh oh!

cgwalters commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhatdan commented May 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Replace `--local` with `--pull=[always,never,missing]` (HMS-3792) #418

Replace `--local` with `--pull=[always,never,missing]` (HMS-3792) #418

kingsleyzissou commented May 3, 2024 •

edited

Loading

rhatdan May 4, 2024 •

edited

Loading

cgwalters commented May 6, 2024 •

edited

Loading