so this seems to fix the problem: containers/podman#25606

@Luap99

I understand ~nothing about the problem space.

With idmapped mounts, there is no extra cost to change the IDs at runtime when using overlay.

This code has no condition for “using overlay”, so I don’t know how that relates. Doesn’t it break the other graph drivers?

This allows the image to be mounted directly with VFS, avoiding any mismatch that could occur when it was previously created with an explicit ID mapping.

AFAICS for non-writeable == image layers, uidMap, gidMap come from the options passed by c/image; and c/image passes nothing, so the existing condition is ~equivalent to Driver.SupportsShifting. That sounds, intuitively, about right? (See above about “using overlay”).

Also, assuming that uidMap/gidMap are always nil during pulls, the two if cases only differ in Host?IDMapping fields … and those fields are not used anywhere in the callees, AFAICS. So how does this make a difference? [Reusing the same types for different purposes is a mess…]

So… I’m confused. I appreciate that there are test PRs that suggest this does something, but just looking at this PR, I can’t see what, or how it possibly could.

(If other reviewers understand what is going on, that’s fine, this does not need to block on me.)

the reason why we have this code is that if you run something like:

podman run --uidmap A:B:C ... IMAGE

and the IMAGE is not present in the local storage, it is pulled directly with the specified mapping. In this way we don't need to create another copy of the image before running the container.

This is not an issue anymore with overlay, since with idmapped mounts we already store the "canonical" image, since s.canUseShifting(uidMap, gidMap) returns true for overlay.

With VFS it does not matter, because for the container we always need to create a full copy anyway.

The issue we had in Podman is that if you run something like:

podman --storage-driver vfs run --uidmap A:B:C ... IMAGE
podman --storage-driver vfs image mount IMAGE

we would end up mounting the image with the mapping A:B:C, which is of course wrong. An alternative solution would be to create a copy of the image at MountImage() time if there is not already a copy with the "canonical" mapping.

I've picked this alternative because it is easier to implement as we already have the logic to handle it, and because I don't see the value in doing this anymore, since the driver we care the most about (overlay) can do the mapping at runtime without any additional setup cost.

Oh, I see what I missed:

if !options.HostUIDMapping && len(options.UIDMap) == 0 {
			uidMap = s.uidMap
		}

in putLayer comes from the Podman CLI, without c/image involvement.

This is not an issue anymore with overlay, since with idmapped mounts we already store the "canonical" image, since s.canUseShifting(uidMap, gidMap) returns true for overlay.

Is that true for rootless? I thought rootless can never use idmapped mounts (unless we use fuse-overlayfs). As such this optimization would be lost for rootless pulls then?
Not sure if that matters because many ways how the images are pulled may not include the idmapped options via podman anyway (e.g. podman pull or the system service)

Is that true for rootless? I thought rootless can never use idmapped mounts (unless we use fuse-overlayfs). As such this optimization would be lost for rootless pulls then?
Not sure if that matters because many ways how the images are pulled may not include the idmapped options via podman anyway (e.g. podman pull or the system service)

that is a good point! No that is not true for rootless and it might impact toolbx

The issue we had in Podman is that if you run something like:

podman --storage-driver vfs run --uidmap A:B:C ... IMAGE
podman --storage-driver vfs image mount IMAGE

we would end up mounting the image with the mapping A:B:C, which is of course wrong.

So, approximately, MountImage is missing something like imageTopLayerForMapping?

The current approach ~relies on podman image mount not having an --uidmap option, so that we store layers in the format which podman image mount prefers.

But what about Store.Mount(containerID)? If different containers can have different ID maps, that effectively allows doing … mount --uidmap $arbitrary; so, from just locally reading the code, shouldn’t we be supporting the general case, with arbitrary mappings, anyway?

And I see a Podman caller of Mount(containerID).

I've picked this alternative because it is easier to implement as we already have the logic to handle it, and because I don't see the value in doing this anymore, since the driver we care the most about (overlay) can do the mapping at runtime without any additional setup cost.

The issue I have with this (if I understand things, which is unlikely) is that it is extremely indirect and invisible to readers of the code.

• This assumes we only have graph drivers like overlay (with trivially cheap ID mapping) or VFS (which do a full copy anyway). What about all the other graph drivers, then? That’s Btrfs, ZFS, AUFS, Windows. I mean, if we absolutely don’t care whether they work, we should just delete them right now.
• Even for overlay, Driver.SupportsShifting has ~4 conditions that need to be true. What about all of the other cases? Should we delete all of that code, and refuse to start?
• We have PutLayer nominally accepting ID mapping options… and it’s going to just ignore them because it assumes that they don’t matter. “How does c/storage.Store know?”; at this API layer, it seems to be possible for a caller to do PutLayer(nonDefaultIDMapping); Mount(), and the caller would be legitimate in expecting the ID mapping to be processed.

I’m all for simplifying things and not processing irrelevant options on unreachable code paths. But then the code paths should be recognizably unreachable. E.g. deprecate the public LayerOptions.*IDMap*, replace internal data structures with smaller better-focused structs that only carry the relevant fields, etc.

I suppose that’s a lot of work, quite possibly more work than making the top-level mapping work as expected for this use case.

But then again, I understand very little about all this code.

What about all the other graph drivers, then?

… and if either of the graph drivers decided to change these features (maybe VFS grew some kind of “fast copy” that required the UID maps to match), how would anyone notice that this putLayer needs updating? That’s what I mean by “invisible”.

• This assumes we only have graph drivers like overlay (with trivially cheap ID mapping) or VFS (which do a full copy anyway). What about all the other graph drivers, then? That’s Btrfs, ZFS, AUFS, Windows. I mean, if we absolutely don’t care whether they work, we should just delete them right now.

I don't know much about these drivers. If changes in c/storage would depend on these drivers we would just stop doing any.

Your analysis is correct, with containers we use imageTopLayerForMapping but then there is a fallback when the image is in an additional image store, and we can't do a copy. So the remapping is done in the container layer (https://github.com/containers/storage/blob/main/store.go#L1742-L1747). With images we can't do that, since we don't create a new layer, so I am not sure how we can deal with images in an additional image store

then there is a fallback when the image is in an additional image store, and we can't do a copy.

Interesting. Making a (potentially very expensive) copy in the primary store on a “mount” would be unexpected… and would we try to delete it again on unmount?

But what about Store.Mount(containerID)? If different containers can have different ID maps, that effectively allows doing … mount --uidmap $arbitrary

On second thought, that should be fine, because we would be mounting not an image layer, but the container’s layer, which is already ID-shifted, isn’t it?

I think the VFS situation can be modeled ~precisely by:

• Adding a Driver property saying what is special about VFS, ContainerCreationAlwaysExpensivelyCopiesLayer — and with that property, never shifting image layers
• Checking the ID mapping options on all other APIs (to do: enumerate), and failing if they specify any other ID mapping when using image layers.

And we would let the other graph drivers do what they have always done, blindly hoping that it is correct.

But I don’t see how this is desirable for the overlay cases where shifting is not available.

I think the VFS situation can be modeled ~precisely by:

• Adding a Driver property saying what is special about VFS, ContainerCreationAlwaysExpensivelyCopiesLayer — and with that property, never shifting image layers
• Checking the ID mapping options on all other APIs (to do: enumerate), and failing if they specify any other ID mapping when using image layers.

And we would let the other graph drivers do what they have always done, blindly hoping that it is correct.

But I don’t see how this is desirable for the overlay cases where shifting is not available.

I don't think we want to keep the current behavior in any case, an optimization makes sense only when the result is correct. If I run podman image mount fedora, I expect the files to be owned by root, not some random ID that was used for a user namespace. That is completely unexpected.

I agree “all image layers are always using the default mapping” is a reasonable and consistent design, in general. Now that we have the other one, switching could potentially have performance implications, by requiring the ID-mapped container layer to be now expensively created. I don’t know how much we care and I don’t know how to tell.

(Does not doing any mapping for image layers work for rootless? I guess it does because everything happens inside a namespace, but just to confirm.)

pushed a new version, now a canonical mapping is created by MountImage if one doesn't already exist. It fails if the image exists only in an additional image store without the canonical mapping, but I think that is a weird configuration anyway.

pushed a new version, now a canonical mapping is created by MountImage if one doesn't already exist. It fails if the image exists only in an additional image store without the canonical mapping, but I think that is a weird configuration anyway.

Sounds reasonable to me, can you please test this version again in podman to make sure there is no new regression.

EDIT: we can do that directly in Podman without adding another API.

I’d much rather add a specialized MountImageIsAlwaysCheap API, or something like that.

One reason is the general design aspect: it’s better to make precisely the promise the caller cares about, and not expose implementation details that affect that promise but don’t exactly match, where we wouldn’t notice when changing c/storage that the Podman CLI also needs updating.

An equally important reason is that we have all these conversations about redesigning the layer store backing (bootc), and redesigning layer identities (sha512); both almost certainly require changes to the driver API, so I’m thinking we should make it internal-only and deprecate the public store.GraphDriver() entirely. Last time I’ve looked, there’s ~1 user in each of Podman, c/common, CRI-O. So, I’d prefer not adding any more users.

@mtrmac fine to merge?

@nalind ok to merge (together with containers/common#2374)?

podman tests pass with the last version

Please consider #2285 (comment) , and #2285 (comment) .

And after looking at that, feel free to merge without another review.

thanks, pushed a new version. I'll merge once the CI is green.

Fine to merge containers/common#2374 too?

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, mtrmac

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

The pull request process is described here