This is really awesome progress on a very old proposal, nice work! 🥳

From a usability perspective, I wonder if this needs to introduce a new from= parameter for specifying the image so that we could mount a subdirectory of an image instead of just the root, like we can with the BuildKit functionality. 🤔
(ie, source would default to / in that case, so you could have --mount type=image,from=...,destination=... for the simple form)

There's probably also more tests this should add, right? Maybe some integration tests?

(To be clear, I'm looking for more maintainer opinions on this -- please don't rush to implement any of my suggestions until we hear from others! 😄 ❤️)

I was waiting on confirmation that how I implemented it was sound before adding the tests but will definitely add them

❤️ Thanks for working on this!! Yes, definitely want eyes from @vvoland and @dmcgowan on this (I know some work is also in progress in containerd to support mounting from OCI artefacts, so we should check if things "conflict" or are "complementary".

wonder if this needs to introduce a new from= parameter for specifying the image so that we could mount a subdirectory of an image instead of just the root

Good point; was thinking that as well (as mounting form an image, especially if that image is a "runnable" image, likely would involve mounting only specific paths.

FWIW; for volume we implemented sub-path mounts, so we could align with that;

• mount: Add volume-subpath option  docker/cli#4331
• volumes: Implement subpath mount #45687

☝️ like Tianon, this is also just a quick blurb from my side; happy to see this, but haven't looked at the nitty-gritty details yet!

One unit test failure on Windows; probably needs to either be excluded if we can't support it on Windows and/or a custom error in the Windows mount parsing code to handle this case;

=== FAIL: github.com/docker/docker/volume/mounts TestParseMountSpec/#06 (0.00s)
    parser_test.go:89: assertion failed: error is not nil: invalid mount config for type "image": mount type unknown

Looks like various "docker build" tests in integration-cli tests are failing, but not exactly sure what's happening there.

one failure on Windows;

=== Failed
=== FAIL: github.com/docker/docker/integration/volume TestRunMountImage (0.02s)
    mount_test.go:303: {"stream":"Step 1/2 : FROM scratch"}
        {"stream":"\n"}
        {"errorDetail":{"message":"Windows does not support FROM scratch"},"error":"Windows does not support FROM scratch"}
        
    mount_test.go:305: assertion failed: error is not nil: Error response from daemon: No such image: test-image:latest

That's strange! It should be excluded as per https://github.com/moby/moby/pull/48798/files#diff-a60d56a02728a9de744c7445cb4b12e1a20dd0e99594b63e327ce7ed55fd63e7R141 and the build in my fork worked

Fixed. I think the failure now are transitive

@vvoland I didn't see that as a problem. Because it creates a UnionFS of the rootfs, deleting the image doesn't affect the running containers. It would only be an issue if trying to recreate the container.

That being said, I can create an integration test to demonstrate the running container being unaffected

@vvoland added the integration test for deleting an image mounted to a container.

https://github.com/moby/moby/pull/48798/files#diff-a60d56a02728a9de744c7445cb4b12e1a20dd0e99594b63e327ce7ed55fd63e7R193-R199

Discussed in today's Moby maintainers meeting, with a few notes:

• no major objections to the feature in general (🥳 🚀)
• this needs to have the containerd implementation also
• we need to handle docker run --mount type=image,from=image-that-does-not-exist ... image-to-run in a sane way;
  right now the default "pull" behavior is that if the "docker create" API call returns something 404-shaped, it does a pull of the image-to-run, which obviously won't get us image-that-does-not-exist;
  options discussed:
  • start with a 400 error for these so the client doesn't try pulling the wrong image
  • an explicit "run" API endpoint so pull policy can be handled daemon side
  • structured errors so the 404 can include which images need to be pulled
    (I think this is the short-term answer that had the most maintainer support? @thaJeztah @cpuguy83 @laurazard ? maybe we can get some details of what we prefer typed up here? ❤️)

For the image to mount not being present locally, I would argue that it can be pushed after this gets merged because the feature is behind a flag that is not enabled by default.

Currently the behavior is like this:

docker run -it --rm --mount type=image,source=postgres,target=/postgres -v test:/test alpine
Unable to find image 'alpine:latest' locally
latest: Pulling from library/alpine
9986a736f7d3: Pull complete 
Digest: sha256:1e42bbe2508154c9126d48c2b8a75420c3544343bf86fd041fb7527e017a4b4a
Status: Downloaded newer image for alpine:latest
docker: Error response from daemon: No such image: postgres:latest.
See 'docker run --help'.

I think having the ability to pull multiple images on a single container create should be a seperate image that would improve the developer experience of this feature.

I created a separate draft PR for the contrainerd part: #49044