Add the ability to automount images as volumes via play #22410

mheon · 2024-04-17T14:54:44Z

Effectively, this is an ability to take an image already pulled to the system, and automatically mount it into one or more containers defined in Kubernetes YAML accepted by podman play.

Requirements:

The image must already exist in storage.
The image must have a single Volume directive. We will mount to the path defined in the Volume directive.
More than one image can be specified, but the paths they mount to (the Volume directives) must be different for all images mounted into a single container.
The images are always mounted read-only
Images to mount are defined in the annotation "io.podman.annotations.kube.image.automount/$ctrname" as a semicolon-separated list. They are mounted into a single container in the pod, not the whole pod.

As we're using a nonstandard annotation, this is Podman only, any Kubernetes install will just ignore this.

Underneath, this compiles down to an image volume
(podman run --mount type=image,...).

Does this PR introduce a user-facing change?

A list of images to automatically mount as volumes can now be specified in Kubernetes YAML via the "io.podman.annotations.kube.image.automount/$ctrname" annotation.
Image-based mounts using `podman run --mount type=image,...` now support a new option, `subpath`, to mount only part of the image into the container.

mheon · 2024-04-17T14:55:24Z

Just compile tested right now, will get real tests done after lunch.
@vikas-goel PTAL - is this what you were looking for?

packit-as-a-service · 2024-04-17T15:15:44Z

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

vikas-goel · 2024-04-17T21:35:22Z

pkg/domain/infra/abi/play.go

+		}
+
+		volumes := inspect.Config.Volumes
+		if len(volumes) != 1 {


The data volume container images can have a single VOLUME directive as a list of volumes. For example,
VOLUME ["/path1/to/vol1", "/path1/to/vol2", "/path2/to/vol3" ]

In that case, do you want to mount the image to every path given as a VOLUME?

Right, all the defined volumes to the respective path.

Also, just to be clear it’s not the entire image that we want to mount. Just the specific volume paths in the image.

So, to be clear, if the volume is /path/to/vol1, you want to mount /path/to/vol1 in the image to /path/to/vol1 in the container?

Alright. Based on this, new requirements are:

The image must already exist in storage.

The image must have at least 1 Volume directive.

The volume path in the image will be mounted to the same path in the container; IE, if the image specifies a volume at /my/volume/a, the content in the image at /my/volume/a will be made available in the container at /my/volume/a

More than one image can be specified, but the paths they mount to (the Volume directives) must be different for all volumes in all images mounted into a single container.

The images are always mounted read-only.

Images to mount are defined in the annotation "io.podman.annotations.kube.image.automount/$ctrname" as a semicolon-separated list. They are mounted into a single container in the pod, not the whole pod.

Correct. Thanks.

On the requirement "but the paths they mount to (the Volume directives) must be different for all volumes in all images mounted into a single container", I was thinking that instead of aborting the operation can we just keep overriding the mount point with the next one in the order in case of conflict? Podman can log a warning message though.

That should be workable.

Thanks Matt.

vikas-goel · 2024-04-17T21:36:11Z

pkg/domain/infra/abi/play.go

+		}
+
+		volumes := inspect.Config.Volumes
+		if len(volumes) != 1 {


Is there a reason to limit this to single volume?

vikas-goel · 2024-04-17T21:37:46Z

Just compile tested right now, will get real tests done after lunch. @vikas-goel PTAL - is this what you were looking for?

@mheon ,
The requirements listed in the description look good except that there can be multiple volumes in an image. Thank you.

vikas-goel · 2024-04-17T23:35:25Z

pkg/domain/infra/abi/play.go

+		}
+
+		imgVol := new(specgen.ImageVolume)
+		imgVol.Source = fullName


I think the Source would be something like fullName + volPath

vikas-goel · 2024-04-18T20:58:55Z

libpod/define/annotations.go

@@ -160,6 +160,9 @@ const (
 	// the k8s behavior of waiting for the intialDelaySeconds to be over before updating the status
 	KubeHealthCheckAnnotation = "io.podman.annotations.kube.health.check"

+	// KubeImageAutomountAnnotation
+	KubeImageAutomountAnnotation = "io.podman.annotations.kube.image.automount"


Should this be named io.podman.annotations.image-volumes.mount?

mheon · 2024-04-19T14:30:07Z

Repushed. Still need manpages and a test for the play kube bits.

mheon · 2024-04-19T14:30:40Z

@Luap99 I think this one is ready for initial review now.

Luap99

overall looks good, using SubPath for this is a elegant solution

Luap99 · 2024-04-19T15:12:17Z

pkg/domain/infra/abi/play.go

+		volumes := inspect.Config.Volumes
+
+		for path, _ := range volumes {
+			if oldPath, ok := volMap[path]; ok && oldPath != nil {


oldPath != nil check seems a bit pointless given we already now we have a map entry by checking ok

Luap99 · 2024-04-19T15:14:00Z

pkg/domain/infra/abi/play.go

+
+		for path := range volumes {
+			if oldPath, ok := volMap[path]; ok && oldPath != nil {
+				logrus.Warnf("Multiple volume mounts to %s requested, overriding image %s with image %s", path, oldPath.Source, fullName)


Can you sue %q for the paths, given this comes from unknown images we may face weird input such as empty string which would be hard to catch otherwise.

mheon · 2024-04-23T19:29:26Z

This should be ready for final review.

mheon · 2024-04-23T19:30:37Z

test/system/700-play.bats

@@ -981,3 +981,45 @@ _EOF
    run_podman pod rm -t 0 -f test_pod
    run_podman rmi -f userimage:latest $from_image
 }
+
+@test "podman play with automount volume" {


@edsantiago PTAL if you get a chance, this test works but it can probably be improved

edsantiago

A few nits and suggestions

edsantiago · 2024-04-23T20:00:26Z

test/system/700-play.bats

+RUN sh -c 'mkdir /test1 /test2'
+RUN sh -c 'touch /test1/a /test1/b /test1/c'
+RUN sh -c 'touch /test2/asdf /test2/ejgre /test2/lteghe'


Why the sh -c? All of those commands should work just fine with RUN. (Update: they do)

edsantiago · 2024-04-23T20:04:34Z

test/system/700-play.bats

+spec:
+  restartPolicy: Never
+  containers:
+    - name: test1


ARGH. As a bear of very little brain, I found it impossible to figure out "is this test1 the volume or test1 the container?" Could you please rename this to testctr (or whatever), and similarly update lines 1014, 1018, 1023?

edsantiago · 2024-04-23T20:05:41Z

test/system/700-play.bats

+    run_podman run --rm automount_test ls /test1
+    run_out_test1="$output"
+    run_podman exec test_pod-test1 ls /test1
+    assert "$output" = "$run_out_test1"


Please add a fourth arg here, something like "exec ls /test1"...

edsantiago · 2024-04-23T20:06:35Z

test/system/700-play.bats

+    run_podman run --rm automount_test ls /test2
+    run_out_test2="$output"
+    run_podman exec test_pod-test1 ls /test2
+    assert "$output" = "$run_out_test2"


...and likewise here, "exec ls /test2".

That makes it possible for a human viewing test results to understand what happened without having to scroll up, look at line numbers, scroll back down, etc

edsantiago · 2024-04-23T20:07:01Z

test/system/700-play.bats

+    run_out_test2="$output"
+    run_podman exec test_pod-test1 ls /test2
+    assert "$output" = "$run_out_test2"
+}


Please add cleanup to avoid nasty red clutter in test logs:

run_podman rm -f -t0 -a run_podman rmi automount_test

mheon · 2024-04-24T13:45:54Z

Final comments addressed, CI should go green, @containers/podman-maintainers PTAL

edsantiago · 2024-04-24T13:48:55Z

LGTM!

Luap99 · 2024-04-24T14:51:06Z

pkg/domain/infra/abi/play.go

+	toReturn := make([]*specgen.ImageVolume, 0, len(volMap))
+	for _, vol := range volMap {
+		toReturn = append(toReturn, vol)
+	}


Could the ordering matter? I just noticed that we turn it from something order to a map back then back to a array, the map looping is not deterministic. As such we open the door for undeterministic flakes here if the order veer matters somewhere. I cannot think of any example where this might be a issue but I like to point it out regardless.

I have to hope that crun/runc are capable of sorting mounts so they happen in the correct order (e.g. /etc before /etc/mydir if both are volumes) otherwise we'd have a lot more issues than just this.

I am pretty sure the runtimes don't do that, they mount in the order they got AFAIK.

Hm. Maybe we do a sort before we pass them over in Libpod, during final spec generation? I'll do a bit of poking, but I'm pretty confident ordering is a solved problem - just a question of where.

ah yeah sortMounts(g.Mounts()) in the spec generation, so this is good then

Luap99

LGTM

openshift-ci · 2024-04-24T15:11:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, mheon

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~~ [Luap99,mheon]

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

mheon · 2024-04-24T19:48:20Z

Bud errors are happening on multiple PRs, something is up with CI

Image volumes (the `--mount type=image,...` kind, not the `podman volume create --driver image ...` kind - it's strange that we have two) are needed for our automount scheme, but the request is that we mount only specific subpaths from the image into the container. To do that, we need image volume subpath support. Not that difficult code-wise, mostly just plumbing. Also, add support to the CLI; not strictly necessary, but it doesn't hurt anything and will make testing easier. Signed-off-by: Matt Heon <[email protected]>

Effectively, this is an ability to take an image already pulled to the system, and automatically mount it into one or more containers defined in Kubernetes YAML accepted by `podman play`. Requirements: - The image must already exist in storage. - The image must have at least 1 volume directive. - The path given by the volume directive will be mounted from the image into the container. For example, an image with a volume at `/test/test_dir` will have `/test/test_dir` in the image mounted to `/test/test_dir` in the container. - Multiple images can be specified. If multiple images have a volume at a specific path, the last image specified trumps. - The images are always mounted read-only. - Images to mount are defined in the annotation "io.podman.annotations.kube.image.automount/$ctrname" as a semicolon-separated list. They are mounted into a single container in the pod, not the whole pod. As we're using a nonstandard annotation, this is Podman only, any Kubernetes install will just ignore this. Underneath, this compiles down to an image volume (`podman run --mount type=image,...`) with subpaths to specify what bits we want to mount into the container. Signed-off-by: Matt Heon <[email protected]>

baude · 2024-04-25T18:14:00Z

/lgtm

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 17, 2024

vikas-goel reviewed Apr 17, 2024

View reviewed changes

vikas-goel reviewed Apr 18, 2024

View reviewed changes

mheon force-pushed the automount_images_k8s branch from bc75132 to fecebdd Compare April 19, 2024 14:28

mheon force-pushed the automount_images_k8s branch 2 times, most recently from 1b84967 to 08ce620 Compare April 19, 2024 15:13

Luap99 reviewed Apr 19, 2024

View reviewed changes

mheon force-pushed the automount_images_k8s branch 9 times, most recently from 9feb50d to 739e87f Compare April 23, 2024 19:29

mheon commented Apr 23, 2024

View reviewed changes

edsantiago reviewed Apr 23, 2024

View reviewed changes

mheon force-pushed the automount_images_k8s branch from 739e87f to 687165c Compare April 24, 2024 13:45

Luap99 reviewed Apr 24, 2024

View reviewed changes

Luap99 approved these changes Apr 24, 2024

View reviewed changes

mheon force-pushed the automount_images_k8s branch from 687165c to 0010185 Compare April 24, 2024 18:37

mheon added 2 commits April 25, 2024 14:12

mheon force-pushed the automount_images_k8s branch from 0010185 to 30e2c92 Compare April 25, 2024 18:12

openshift-ci bot assigned baude Apr 25, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2024

openshift-merge-bot bot merged commit 15cc886 into containers:main Apr 25, 2024
86 of 91 checks passed

basilgello mentioned this pull request May 8, 2024

Add missing user-facing part to allow volume subpath mounts #22040

Closed

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jul 25, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators Jul 25, 2024

Add the ability to automount images as volumes via play #22410

Add the ability to automount images as volumes via play #22410

Conversation

mheon commented Apr 17, 2024 • edited Loading

Does this PR introduce a user-facing change?

mheon commented Apr 17, 2024

packit-as-a-service bot commented Apr 17, 2024

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

Choose a reason for hiding this comment

vikas-goel commented Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Apr 19, 2024

mheon commented Apr 19, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Apr 23, 2024

Choose a reason for hiding this comment

edsantiago 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

mheon commented Apr 24, 2024

edsantiago commented Apr 24, 2024

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

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 24, 2024

mheon commented Apr 24, 2024

baude commented Apr 25, 2024

mheon commented Apr 17, 2024 •

edited

Loading

vikas-goel commented Apr 17, 2024 •

edited

Loading