Skip to content

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Dec 13, 2023

- What I did
Store the container ID the image was committed from in containerd image object label.

This fixes test_commit_with_changes and partially test_commit from tests.integration.api_image_test.CommitTest.

- How I did it

- How to verify it
docker-py: test_commit_with_changes should pass now

- Description for the changelog

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

Before this change `ParentId` was filled for images when calling the
`/images/json` (image list) endpoint but was not for the
`/images/<image>/json` (image inspect).

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland added status/2-code-review area/images Image Distribution kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration labels Dec 13, 2023
@vvoland vvoland added this to the 25.0.0 milestone Dec 13, 2023
@vvoland vvoland self-assigned this Dec 13, 2023
Store the container ID the image was committed from in containerd image
object label.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland force-pushed the c8d-inspect-container branch from 9707e98 to ad6b579 Compare December 13, 2023 11:36
@thaJeztah
Copy link
Member

Do we use this property for anything? I'm leaning towards going the reverse direction, and officially deprecating this information.

If I recall correctly, this is information that was either used by the classic builder, or was more a "side effect" of how the classic builder (which uses docker commit) was implemented.

However, this information has been cause for confusion in many situations; inspecting an image shows multiple similar fields (e.g. Command, Env, and such). In the end, "what container was used" to produce the images config doesn't really matter. The result of that action is committed in the Image's config, and that's the only part that's used from the image. Everything else may be just noise.

@vvoland
Copy link
Contributor Author

vvoland commented Dec 13, 2023

No, we don't need it from the engine side. Not sure if docker-py tests check it because it's important or just happened to be a property of a committed image. If you think it's fine to deprecate, I'm all in 😅

@thaJeztah
Copy link
Member

I think there's quite some tests in docker-py testing "what they observed" at the time. Which is good because it also spotted breakage in some occasions (fields that were there but not strictly relevant for internal use).

But I also think that the containerd integration gives us a good opportunity to re-evaluate some of those. We can consider some "defendable" changes in the API (directly related to legacy implementation; no longer relevant with containerd). Of course; within reason (i.e. don't blindly just rip-out important fields)

I recall I already set the stage for that by adding some disclaimers that these fields may be empty, depending on how the image was built;

moby/api/swagger.yaml

Lines 1719 to 1728 in f3cc936

Parent:
description: |
ID of the parent image.
Depending on how the image was created, this field may be empty and
is only set for images that were built/created locally. This field
is empty if the image was pulled from an image registry.
type: "string"
x-nullable: false
example: ""

moby/api/swagger.yaml

Lines 1742 to 1749 in f3cc936

Container:
description: |
The ID of the container that was used to create the image.
Depending on how the image was created, this field may be empty.
type: "string"
x-nullable: false
example: "65974bc86f1770ae4bff79f651ebdbce166ae9aada632ee3fa9af3a264911735"

But perhaps this is the moment to start making those official deprecations (which could mean: omitting the field entirely in a future version of the API)

@thaJeztah
Copy link
Member

Also related to the above;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/images Image Distribution containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs status/2-code-review
Projects
Development

Successfully merging this pull request may close these issues.

2 participants