Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quadlet - support ImageName for .image files #20368

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

ygalblum
Copy link
Contributor

@ygalblum ygalblum commented Oct 16, 2023

Allow overriding the image name resolved from the .image file

Does this PR introduce a user-facing change?

Yes

Quadlet - support ImageTag for .image files

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ygalblum

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2023
@packit-as-a-service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

cat > $quadlet_image_file <<EOF
[Image]
Image=docker-archive:$archive_file
ImageTag=$image_for_test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be another name than the one we used in podman save?

I though that ImageTag would case the loaded/pull image to be tagged as such but I do not see the plumbing for it. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. I have the same question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me explain.

First, the lines:

run_podman image tag $IMAGE $image_for_test
run_podman image save --format docker-archive --output $archive_file $image_for_test
run_podman image rm $image_for_test

are the setup of the test, they are not the test itself.

After them, we are left with a docker-archive file saved under $archive_file that when pulled will translate to an image with the tag $image_for_test in the local container storage (podman save stores the tag as well). To make sure the archive file is in fact pulled, the setup removes the tag from the local container storage.

When the test runs, we have a dependency of container->volume->image, so starting the container will lead to the following:
Image Service: podman image pull docker-archive:$archive_file
Volume Service: podman volume create --driver image -o image=$image_for_test

The reason I needed the volume in this test is that podman run accepts docker-archive:$archive_file as the image to use and as a result, not setting ImageTag works as well. podman volume create however, does not support all transports. So, if ImageTag is not set, the command fails.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for elaborating, @ygalblum.

So the problem is that the .volume is pointed to an .image where there latter does not expose an image reference that the .volume needs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
When a Quadlet file (.container or .volume) references a .image file, Quadlet translates the reference to the name of the resource (same as it does for .volume or .network). By default, the name of the resource produced by an .image file is the image passed in its Image key. However, in some cases, this does not work and for this reason, I added the new key ImageTag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example in the man page explain exactly that.

Almost, but not quite, and I think that was the source of my confusion. The phrasing is ambiguous: I interpreted "name of the image after being pulled" as "once the image is pulled, quadlet will rename/retag it with this new name". I now understand my mistake, but future readers might appreciate a clearer description. (I don't have a good suggestion right now, I'm sorry).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point taken. I thought the example explains it and I guess the root cause is that I'm explaining what I know without addressing what someone else does not.
I'll welcome any rephrasing. Maybe @TomSweeneyRedHat, you usually make my doc stuff better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ygalblum. The additional .container example convinced me that's the right solution 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename to ExternalImageName?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard -.-

@ygalblum ygalblum force-pushed the quadlet-image-name branch 3 times, most recently from f6398db to b20093e Compare October 17, 2023 11:38
Comment on lines 1151 to 1158
The name of the image after being pulled.
This parameter should be used when the value of `Image` is not the name of the resulting image.

For example, an image that was saved into a `docker-archive` with the following command:

`podman image save --format docker-archive --output /tmp/archive-file.tar quay.io/podman/stable:latest`

will require setting `Image=docker-archive:/tmp/archive-file.tar` and `ImageTag=quay.io/podman/stable:latest`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible different direction:

Actual FQIN of the referenced `Image`; only meaningful when source is a file or directory
archive. For example, this locally-saved image file:

    # podman image save --format docker-archive -o /tmp/archive-file.tar quay.io/podman/stable:latest

...would be referenced as:

    Image=docker-archive:/tmp/archive-file.tar
    ImageTag=quay.io/podman/stable:latest

The name of the image after being pulled.
This parameter should be used when the value of `Image` is not the name of the resulting image.

For example, an image that was saved into a `docker-archive` with the following command:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For example, an image that was saved into a `docker-archive` with the following command:
For example, an image that was saved into a `docker-archive` with the following Podman command:


`podman image save --format docker-archive --output /tmp/archive-file.tar quay.io/podman/stable:latest`

will require setting `Image=docker-archive:/tmp/archive-file.tar` and `ImageTag=quay.io/podman/stable:latest`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK if this is closer, I'm struggling with the wording myself.

Suggested change
will require setting `Image=docker-archive:/tmp/archive-file.tar` and `ImageTag=quay.io/podman/stable:latest`
would require setting the value of the `Image` and `ImageTag` systemd configuration options to `Image=docker-archive:/tmp/archive-file.tar` and `ImageTag=quay.io/podman/stable:latest`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requires setting the Image=docker-archive:/tmp/archive-file.tar and ImageTag=quay.io/podman/stable:latest

Take the will and would out of it, and adding the image and imagetag names twice does not lead to clarity.

Allow overriding the image name resolved from the .image file
Add test and doc

Signed-off-by: Ygal Blum <[email protected]>
@ygalblum
Copy link
Contributor Author

@rhatdan @vrothberg @edsantiago @TomSweeneyRedHat
Thanks all for your suggestions. I've tried to rephrase according to them. I hope this is better now. I've kept the name ImageTag since I'm not sure ExternalImageName is better.

Please let me know if you think of a better phrasing/name

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rhatdan
Copy link
Member

rhatdan commented Oct 18, 2023

At a certain point, we are bikeshedding.
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2023
@openshift-ci openshift-ci bot merged commit 553cfb6 into containers:main Oct 18, 2023
@ygalblum ygalblum deleted the quadlet-image-name branch October 19, 2023 06:10
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jan 18, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants