-
Notifications
You must be signed in to change notification settings - Fork 617
Output correct image ID when using Docker with the containerd snapshotter #3122
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
Conversation
a53b360 to
7c62ca4
Compare
7c62ca4 to
8200edc
Compare
…tter. Prior to this change, the following command emits the wrong image ID when buildx uses the "docker-container" driver and Docker is configured with the containerd-snapshotter. $ docker buildx build --load --iidfile=img.txt $ docker run --rm "$(cat img.txt)" echo hello docker: Error response from daemon: No such image: sha256:4ac37e81e00f242010e42f3251094e47de6100e01d25e9bd0feac6b8906976df. See 'docker run --help'. The problem is that buildx is outputing the incorrect image ID in this scenario (it's outputing the container image config digest, instead of the container image digest used by the containerd-snapshotter). See moby/moby#45458. Signed-off-by: Cesar Talledo <[email protected]> [WIP]: update testImageIDOutput test. Signed-off-by: Cesar Talledo <[email protected]>
8200edc to
295058d
Compare
| return true | ||
| } | ||
|
|
||
| func (d *Driver) UsesContainerdSnapshotter(ctx context.Context) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just define a new Feature for this. Seems lot of duplication and extra unnecessary requests.
| driverType := b.Driver | ||
|
|
||
| dockerUsingContainerdSnapshotter := false | ||
| if driverType == "docker" || driverType == "docker-container" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is anything special for these drivers. For "docker" containerd support can be detected via feature flag like it is for other things not supported by graphdrivers.
For other drivers, I guess they are only affected if --load is set. If there is no --load then there is no reason to check a specific docker instance at build time, as it would be completely unused. If this feature has problems with other drivers as well on --load then I think we need to capture the image ID from moby side when we are doing docker load. So we load image into Moby, and it reports to us what is the ID of the image that was just imported (as it would be different in different Moby versions). It's quite weird though that the value of --iidfile depends on in --load is set or not.
|
After a conversation with @tonistiigi and @jsternberg, the solution being proposed by this PR is not ideal, because this PR selects the image ID returned by buildkit (i.e., image digest or image config digest) based on the build driver config (particularly whether the driver is "docker" or "docker-container" and Docker is configured with the containerd-snapshotter), but there's no guarantee that this image ID can be used to run the image since the run may occur under a different host config. The better solution is to have Moby run the image with either the image digest or image config digest. Closing this PR. |
Prior to this change, the following command emits the wrong image ID when using the "docker" driver and Docker is configured with the containerd snapshotter.
The problem is that buildx is outputing the incorrect image ID in this scenario (it's outputing the container image config digest, instead of the container image digest, and the latter is needed to run the image with containerd).
This change fixes this.
See moby/moby#45458.