Skip to content

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Dec 14, 2023

These fields were an implementation detail of the classic image builder and are empty when using buildkit.

Deprecate them in API v1.44 (Moby v25) and schedule their removal for API v1.45 (Moby v26)

- What I did

- How I did it

- How to verify it

- Description for the changelog

- api: Deprecate `Container` and `ContainerConfig` properties for `/images/{id}/json` (`docker image inspect`).

- A picture of a cute animal (not mandatory but encouraged)

These fields were an implementation detail of the classic image builder
and are empty when using buildkit.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

discussed in the maintainers call; we can set an empty struct for API < 1.46, and remove it in API 1.46

@thaJeztah thaJeztah merged commit bd70d66 into moby:master Dec 14, 2023
@thaJeztah
Copy link
Member

For future reference; there was some back-and-forth on the deprecation;

  • The Container and ContainerConfig fields are set when committing a container to an image (docker commit)
  • When doing so, relevant fields from the container's config are copied to the image config (a subset of that config is part of the OCI specification)
  • The ContainerConfig is not part of the OCI config, and not used by other runtimes, such as containerd (they ignore the field)
  • However, we currently preserve the field when pushing an image, which means we keep non-OCI fields in the image that's "unused", but also affects the digest

While the information can be considered somewhat useful (e.g. docker container commit mycontainer image:foo would keep information about the container that was committed in docekr image inspect image:foo), that information is of limited use.

Re-implementing that functionality with the containerd integration involves storing it as custom metadata, which also involves significant additional complexity for a niche use-case, so;

  • for the graph-driver situation: on old API versions, continue setting the ContainerConfig field (but this can be an empty struct, as documented in the API spec (fields may be empty))
  • when pushing images, we should not preserve this information
  • for new API versions, we omit the fields entirely
  • when using the containerd store, also omit the fields

On a side note, we also briefly discussed NOT supporting older API versions when using the containerd integration. i.e., continue the road we started with;

And raise minimum supported API version with the containerd store enabled to only the last X API versions to reduce the amount of complexity for "brand new" code.

vvoland added a commit to vvoland/cli that referenced this pull request Feb 22, 2024
See moby/moby#46939

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
vvoland added a commit to vvoland/cli that referenced this pull request Feb 22, 2024
See moby/moby#46939

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
vvoland added a commit to vvoland/cli that referenced this pull request Feb 28, 2024
See moby/moby#46939

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
vvoland added a commit to vvoland/cli that referenced this pull request Mar 13, 2024
See moby/moby#46939

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
vvoland added a commit to vvoland/cli that referenced this pull request Mar 13, 2024
See moby/moby#46939

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants