add artifact mount support #25397

Luap99 · 2025-02-25T18:19:18Z

Add support for mounting artifacts with type=artifact,src=quay.io/libpod/testartifact:20250206-single,dst=/data

see commits

Does this PR introduce a user-facing change?

The --mount option accepts a new mount type `artifact` to mount a artifact into a container.

openshift-ci · 2025-02-25T18:19:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

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]

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

Luap99 · 2025-02-25T18:22:35Z

@mheon @baude PTAL

Opening to allow early review. I still have to add a tests of course, one thing that is broken is selinux support, the files must be labelled properly so the container is allowed to read them, containers/container-selinux#360. In case someone likes to test locally you can use --security-opt label=disable for now to work around it.

cc @mtrmac (in case you are interested how the caller of GetLocalBlobPath() looks)

mtrmac

(Only really looking at the c/image part, and quickly scrolling through everything else.)

libpod/container_internal_common.go

pkg/libartifact/store/store.go

libpod/container_internal_common.go

libpod/runtime.go

mheon · 2025-02-26T14:51:25Z

pkg/specgenutil/volumes.go

+			return nil, err
+		}
+	}
+	for dest := range unifiedContainerMounts.artifactVolumes {


When you add tests, please add tests for mount conflicts - bind mounts, overlay mounts, and image mounts that try to mount to the same place as an artifact volume.

One thing that is a bit unclear is that the destination for an artifact mount is not the actual mount destination. If ...,dst=/data is used the actual artifact blobs are mounted to /data/blob1,etc... So technically speaking mounting -v /host:/data does not conflict, we could still mount the artifacts in there and then the other files are still viable next to it.

So I am not sure if we even want to cause conflicts here because it might be a valid use case?

If we don't conflict and individual files in the artifact do conflict with something else we mounted in, this turns into undefined behavior - it's up to the OCI runtime to decide what happens. I really don't like that. Simpler to conflict IMO. We can handle this at a later date if we really need to.

But we have no way here to correctly conflict, if I mount -v somfile:/data/testfile and --mount type=artifact,src=quay.io/libpod/testartifact:20250206-single,dst=/data then no conflict can be generated here as we cannot lookup the artifact here, and anyway the artifact can change later.

The behavior should not be undefined, the mount order in the runtime spec is an array, the order is exactly as we instruct it it to

I would say that /data would be a conflict here. Yes, it works in this case, but that is far from guaranteed and we cannot know until the container is actually started. IMO, sanest thing is to treat the attempt to share /data as a conflict, as we cannot know for sure if it's safe.

I don't really see a specific need to do mounts on top of artifacts either. It doesn't seem like something that anyone would intentionally do.

I would argue the /data conflict is what we should do, not because it's correct, but for architectural reasons. With client-side conflict detection, we can't do this the right way (and even if we could, the fact that there is no binding to the artifact at present makes it pointless - the user can replace the artifact (assuming a digest wasn't specified) whenever they want to, causing new conflicts we can't anticipate.

The alternative, I suppose, would be to do detection at OCI spec generation time. I suppose it wouldn't be that expensive for most containers? I definitely don't want to hand over a spec with a duplicated mount to the OCI runtime though.

Yes if you want that level of verification it must be where we actual generate the spec

The OCI runtime doesn't decide how the mounts are applied, they are just done in the specified order in the spec file. It will catch such conflicts since if you try to use the same destination for a file and then a directory (or the other way around), the bind mount will fail.

But we lose control of error messages that way. crun and runc are probably fine, but someone's custom runtime may give us back something completely unintelligible for a problem we should know about and catch before generating a bad spec.

The thing is we cannot know, nested mounts are valid and podman does not perform the mounts so we cannot know the content of mount to check if the mount if a file or dir to know if they will trigger a mount failure in advance.

I have the obvious duplicate dest logic that check the paths (see the test), but for the complex nested mounts scenarios we just have to trust the oci runtimes to provide helpful error messages.

baude · 2025-03-03T22:04:28Z

LGTM

mheon · 2025-03-05T13:52:59Z

You have conflicts. But LGTM once you rebase. Let's get this landed.

Luap99 · 2025-03-05T18:01:59Z

Yeah well that sucks, I cannot rebase the vendor conflict away because vendoring is broken.

c/image updated docker to v28 but this contains changes that need to be fixed in c/common and podman first #25371

My previous commit was using an c/image commit before the docker update which was fine, but now to the v5.34.1 release it thinks that is newer than my commit (well it partially is), now that we merged the buildah update and that uses v5.34.1 so go will always update me to that even when I try to pin an older commit.

So I either use a ugly replace or this has to wait until we resolve all docker vendor issue first.

The main point of this is so that I can share the same lookup logic between Extract() and then the new blob path API I add next. Signed-off-by: Paul Holzinger <[email protected]>

Signed-off-by: Paul Holzinger <[email protected]>

Create a getArtifactAndImageSource() function so this one can be shared with the new mount blob API that is added next to avoid code duplication. Signed-off-by: Paul Holzinger <[email protected]>

The goal of this new interface is to expose the blob source path and the target file name for a bind mount into a container. libpod will call this and then take care of setting up the actual mounts based on the returned paths. Signed-off-by: Paul Holzinger <[email protected]>

Instead of duplicating the NewArtifactStore() call in many places and having to make sure we always pass the same path to it define it as function on the runtime. This allows any caller with access to the libpod runtime to create the store easily. This is suing a sync.OnceValues() function so the store is initialized only once and only when actually needed. Signed-off-by: Paul Holzinger <[email protected]>

Will safe a few memory copies, we must do that only after namesOrDigests was populated so the len() does not report zero. Signed-off-by: Paul Holzinger <[email protected]>

The function is never used elsewhere so do not export it. Signed-off-by: Paul Holzinger <[email protected]>

Use a helper struct to hold the mounts instead of returning 5+ return values from the functions. This allows use to easily add more volume types without having to update all return lines every time in the future. And 5+ return values are really not readable anymore so this should make it easier to follow the code. Signed-off-by: Paul Holzinger <[email protected]>

There is no need whatsoever to run container to populate a random file, this is just much slower than just writing some random bytes directly without having to run a container and run dd in it. Also the function accepted the number of bytes, however because dd uses a minimum block size of 512 bytes it was actually numBytes * 1024 which where written. That makes no sense so fix the two tests that depended on the wrong number. Signed-off-by: Paul Holzinger <[email protected]>

Add a new option to allow for mounting artifacts in the container, the syntax is added to the existing --mount option: type=artifact,src=$artifactName,dest=/path[,digest=x][,title=x] This works very similar to image mounts. The name is passed down into the container config and then on each start we lookup the artifact and the figure out which blobs to mount. There is no protaction against a user removing the artifact while still being used in a container. When the container is running the bind mounted files will stay there (as the kernel keeps the mounts active even if the bind source was deleted). On the next start it will fail to start as if it does not find the artifact. The good thing is that this technically allows someone to update the artifact with the new file by creating a new artifact with the same name. Signed-off-by: Paul Holzinger <[email protected]>

The oci layout code can handle a relative path find but all paths returned by the code then will alos be relative, this can be bad and result in bugs if something ever changes the cwd. The graphroot path we pass should already be always absolute, so just add a sanity check here given libartifact is planned to be moved as sperate lib and we cannot assume anything about the path we will be given there. Signed-off-by: Paul Holzinger <[email protected]>

mheon · 2025-03-12T18:43:37Z

/lgtm

TomSweeneyRedHat · 2025-03-12T20:24:30Z

docs/source/markdown/options/mount.md

+does not exist the digest will be used as filename instead. This results in all blobs
+of the artifact mounted into the container at the given path.
+
+However if the *dst* path is a existing file in the container then the blob will be


Suggested change

However if the *dst* path is a existing file in the container then the blob will be

However, if the *dst* path is an existing file in the container, then the blob will be

nits, if you have other updates.

TomSweeneyRedHat · 2025-03-12T20:25:08Z

docs/source/markdown/options/mount.md

+of the artifact mounted into the container at the given path.
+
+However if the *dst* path is a existing file in the container then the blob will be
+mounted directly on it. This only works when the artifact contains of a single blob


Suggested change

mounted directly on it. This only works when the artifact contains of a single blob

mounted directly on it. This only works when the artifact contains a single blob

nits, if you have other changes.

TomSweeneyRedHat · 2025-03-12T20:33:10Z

Just a couple of nits in docs that could be touched up separately
LGTM

As pointed out by Tom on the PR containers#25397. Signed-off-by: Paul Holzinger <[email protected]>

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Feb 25, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2025

mtrmac reviewed Feb 25, 2025

View reviewed changes

libpod/container_internal_common.go Outdated Show resolved Hide resolved

pkg/libartifact/store/store.go Show resolved Hide resolved

Luap99 mentioned this pull request Feb 26, 2025

add container_ro_file_t to the podman artifact store containers/container-selinux#360

Merged

mheon reviewed Feb 26, 2025

View reviewed changes

libpod/container_internal_common.go Outdated Show resolved Hide resolved

libpod/container_internal_common.go Outdated Show resolved Hide resolved

mheon reviewed Feb 26, 2025

View reviewed changes

libpod/runtime.go Show resolved Hide resolved

mheon reviewed Feb 26, 2025

View reviewed changes

Luap99 force-pushed the artifact-mount branch from 2887fe7 to 2578ad1 Compare February 27, 2025 14:18

Luap99 marked this pull request as ready for review February 27, 2025 14:20

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 27, 2025

Luap99 added the bloat_approved Approve a PR in which binary file size grows by over 50k label Feb 27, 2025

Luap99 mentioned this pull request Feb 27, 2025

Allow image mounts to use mount options, such as noexec #25407

Open

Luap99 force-pushed the artifact-mount branch from 2578ad1 to 71a7724 Compare February 27, 2025 15:08

This was referenced Feb 27, 2025

Add OCI artifact support using libimage cri-o/cri-o#8996

Merged

Mount OCI artifacts cri-o/cri-o#9026

Closed

saschagrunert mentioned this pull request Mar 4, 2025

libimage artifact support shared between CRI-O and Podman containers/common#2347

Open

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2025

Luap99 added 7 commits March 12, 2025 19:42

libartifact: create FilterBlobOptions

618e377

The main point of this is so that I can share the same lookup logic between Extract() and then the new blob path API I add next. Signed-off-by: Paul Holzinger <[email protected]>

libartifact: fix comment on Extract()

4cd19b7

Signed-off-by: Paul Holzinger <[email protected]>

libartifact: extract common code into helper

86a6539

Create a getArtifactAndImageSource() function so this one can be shared with the new mount blob API that is added next to avoid code duplication. Signed-off-by: Paul Holzinger <[email protected]>

correctly preallocate artifactDigests in ArtifactRm()

51cfcc6

Will safe a few memory copies, we must do that only after namesOrDigests was populated so the len() does not report zero. Signed-off-by: Paul Holzinger <[email protected]>

pkg/specgenutil: unexport Mounts()

590bf8b

The function is never used elsewhere so do not export it. Signed-off-by: Paul Holzinger <[email protected]>

Luap99 added 4 commits March 12, 2025 19:42

Luap99 force-pushed the artifact-mount branch from 71a7724 to c05908a Compare March 12, 2025 18:42

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2025

openshift-ci bot assigned mheon Mar 12, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2025

TomSweeneyRedHat reviewed Mar 12, 2025

View reviewed changes

openshift-merge-bot bot merged commit 6e34514 into containers:main Mar 13, 2025
78 of 79 checks passed

Luap99 deleted the artifact-mount branch March 13, 2025 13:01

Luap99 added a commit to Luap99/libpod that referenced this pull request Mar 13, 2025

docs: fix some minor issues

94cacea

As pointed out by Tom on the PR containers#25397. Signed-off-by: Paul Holzinger <[email protected]>

Luap99 mentioned this pull request Apr 3, 2025

RFE: Add artifact quadlet unit type #25778

Open

amisevsk mentioned this pull request Jun 4, 2025

Consider using the org.opencontainers.image.title to define filename for binary layer files modelpack/model-spec#58

Open

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 Jun 12, 2025

stale-locking-app bot locked as resolved and limited conversation to collaborators Jun 12, 2025

	However if the dst path is a existing file in the container then the blob will be
	However, if the dst path is an existing file in the container, then the blob will be

	mounted directly on it. This only works when the artifact contains of a single blob
	mounted directly on it. This only works when the artifact contains a single blob

add artifact mount support #25397

add artifact mount support #25397

Uh oh!

Conversation

Luap99 commented Feb 25, 2025

Does this PR introduce a user-facing change?

Uh oh!

openshift-ci bot commented Feb 25, 2025

Uh oh!

Luap99 commented Feb 25, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Mar 3, 2025

Uh oh!

mheon commented Mar 5, 2025

Uh oh!

Luap99 commented Mar 5, 2025

Uh oh!

mheon commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented Mar 12, 2025

Uh oh!

Uh oh!

Uh oh!