containerd integration: Persist Docker-specific ImageConfig fields by vvoland · Pull Request #46313 · moby/moby

vvoland · 2023-08-24T14:05:45Z

Depends on: image/spec: Add Go structs #46363

This fixes ONBUILD, MAINTAINER and Healthcheck in containerd integration.

- What I did
Fixed ONBUILD, MAINTAINER and healthchecks in containerd integration.

- How I did it
Use wrappers for OCI Image and ImageConfig which preserve Docker specific config fields when serializing/deserializing images.

- How to verify it

$ make DOCKER_GRAPHDRIVER=overlayfs  \
   DOCKER_BUILDKIT=0 \
   TEST_INTEGRATION_USE_SNAPSHOTTER=1  \
   TEST_FILTER='(Health|OnBuild|TestBuildMaintainer|TestBuildFrom)'    test-integration
(...)
- DONE 74 tests, 1 skipped, 17 failures in 85.404s
+ DONE 74 tests, 1 skipped in 58.001s

Also, test case (for healthcheck) reported by user:

$ docker run -d syncthing/syncthing
40280b7b22645ef2967bdae63fdfee3522e9e7003c3de7d5eae689f6a1713a53

$ docker ps
- CONTAINER ID   IMAGE                 COMMAND                  CREATED         STATUS        PORTS                                       NAMES
- 3e797ed5e0ee   syncthing/syncthing   "/bin/entrypoint.sh …"   2 seconds ago   Up 1 second   8384/tcp, 21027/udp, 22000/tcp, 22000/udp   kind_cray
+ CONTAINER ID   IMAGE                 COMMAND                  CREATED         STATUS                           PORTS                                       NAMES
+ 40280b7b2264   syncthing/syncthing   "/bin/entrypoint.sh …"   2 seconds ago   Up 1 second (health: starting)   8384/tcp, 21027/udp, 22000/tcp, 22000/udp   charming_shamir

- Description for the changelog

- containerd integration: Fixed `ONBUILD` and `MAINTAINER` Dockerfile instruction
- containerd integration: Fixed healthchecks

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

vvoland · 2023-08-24T14:08:06Z

pkg/dockerspec/image.go

@@ -0,0 +1,50 @@
+package dockerspec


I wasn't sure what's the best place to put these. As they're more an implementation detail than a part of the API.
The reason I thought creating a separate pkg for these is that this could be used by others interacting with these (like buildkit: https://github.com/moby/buildkit/blob/4376f3861b05e0b4a27c2259ee1fa499ac117a41/exporter/containerimage/image/docker_image.go).

Thoughts?

Alternatively, we could only have them in the daemon/containerd for now.

Would these be related to any of these?

https://github.com/moby/moby/tree/master/image/v1

https://github.com/moby/moby/tree/master/image/spec

ISTR we had that discussion, where BuildKit now had a definition, but the actual spec is in this repository

The discussion in question (I think) rumpl#121 (comment)

Yes, it's a https://github.com/moby/moby/blob/master/image/spec/v1.2.md (application/vnd.docker.container.image.v1+json).

thaJeztah · 2023-08-25T10:06:25Z

I wasn't sure what's the best place to put these. As they're more an implementation detail than a part of the API.
The reason I thought creating a separate pkg for these is that this could be used by others interacting with these (like buildkit:

We discussed this in the maintainers call Yesterday;

While the "Spec" (using quotes here, as it could use some TLC to make the wording more like a spec) lives in this repository (and was used as foundation for the OCI image spec), not having a a consumable, canonical, Go implementation for the specification is a clear omission.
Not having a canonical implementation is what resulted in BuildKit creating one in the BuildKit repository, which is "not ideal" (while discussing we also noticed that the implementation there had a mistake; docker: cleanup fields in image definition buildkit#4176)
We liked the current approach of "composing" the struct by embedding both the OCI spec and "Docker-Specific extensions", as it both makes it clear what the "Docker Extensions" are to the OCI spec, and allows users to "downgrade" a struct to a "vanilla" OCI Spec (without extensions), if they would want to.
It's worth noting that some of the "docker" extensions were accidentally left out of the OCI spec, but later added (such as ArgsEscaped)
And others are yet to be decided on (such as HealthCheck)

All of the above combined, here's what we came to as direction forward;

We want to move the Specification and a canonical go-implementation to a separate repository / module
Having the specification in its own repository / module makes it more discoverable, and allows it to be consumed by projects that want to provide compatibility with the Docker Extensions to the OCI Spec.
It also allows for the spec to evolve, separate from either the "Moby" or "BuildKit" release cadence, which may happen if (e.g.) Extensions are accepted into the OCI Image spec (and now can be removed from the "Docker" image spec)

As to the "shape" of that package (module), and "what to include";

We could consider to make the structure similar to the OCI Image spec (specification (Markdown) at the top-level, with a sub-directory for the go-implementation, which also would allow reference implementations for other languages beyond "go")
For the Go implementation, we briefly discussed "what to include", and the proposal was to "keep it lean", and start with just the Structs (similar to the OCI image spec)
But with the possibility to add additional "utilities" that may help consuming the module; this part may have to be looked at (are there utilities that would be useful?)

As to the "how"";

We do not want "moving the specification to a separate module" to block this PR (as it's orthogonal)
But to provide a smooth transition, we should aim for this PR to put the files in the right locations, so that we can do a clean "extract" / migration to a separate module (preserving history where possible)
Ideally have the right GoDoc / wording already in place, but these can be time-consuming, so we can go with "Good enough for this PR", and do some tweaking in a follow-up.

Once we have the package in a proper shape (which may be after follow-ups);

Extract the package to a separate module
Tag the module
Replace the code both in this repository and in BuildKit with the new module (for the Moby repository, this should look as a move/rename in Git)

I'll also create a separate ticket for the proposal to move the spec to a separate module.

Parent is a graph-driver only field which is stored in the ImageStore. It's not available when using containerd snapshotters. Signed-off-by: Paweł Gronowski <[email protected]>

This makes the c8d code which creates/reads OCI types not lose Docker-specific features like ONBUILD or Healthcheck. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland added area/builder Build area/images Image Distribution kind/bugfix PR's that fix bugs area/builder/classic-builder Build containerd-integration Issues and PRs related to containerd integration labels Aug 24, 2023

vvoland added this to the 25.0.0 milestone Aug 24, 2023

vvoland commented Aug 24, 2023

View reviewed changes

tonistiigi mentioned this pull request Aug 24, 2023

docker: cleanup fields in image definition moby/buildkit#4176

Merged

vvoland force-pushed the c8d-image-ociwrapper branch from 84efaf6 to f43ea4d Compare August 25, 2023 09:30

vvoland force-pushed the c8d-image-ociwrapper branch from f43ea4d to db9ecdd Compare August 29, 2023 11:16

vvoland marked this pull request as draft August 29, 2023 13:26

vvoland mentioned this pull request Aug 29, 2023

image/spec: Add Go structs #46363

Merged

vvoland force-pushed the c8d-image-ociwrapper branch from db9ecdd to fe71b71 Compare August 31, 2023 10:31

vvoland mentioned this pull request Aug 31, 2023

c8d/legacybuilder: Assorted fixes #46383

Merged

c8d/integration/TestBuildOnBuildCache skip parent check

14af90b

Parent is a graph-driver only field which is stored in the ImageStore. It's not available when using containerd snapshotters. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the c8d-image-ociwrapper branch from fe71b71 to 1b58e47 Compare August 31, 2023 14:23

vvoland marked this pull request as ready for review August 31, 2023 14:23

rumpl approved these changes Aug 31, 2023

View reviewed changes

neersighted approved these changes Aug 31, 2023

View reviewed changes

vvoland marked this pull request as draft August 31, 2023 15:13

daemon/c8d: Use Docker imagespec

0ffa3dd

This makes the c8d code which creates/reads OCI types not lose Docker-specific features like ONBUILD or Healthcheck. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the c8d-image-ociwrapper branch from 1b58e47 to 0ffa3dd Compare August 31, 2023 15:15

vvoland marked this pull request as ready for review August 31, 2023 15:18

neersighted approved these changes Aug 31, 2023

View reviewed changes

vvoland merged commit 8dfaf0c into moby:master Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

containerd integration: Persist Docker-specific ImageConfig fields#46313

containerd integration: Persist Docker-specific ImageConfig fields#46313
vvoland merged 2 commits intomoby:masterfrom
vvoland:c8d-image-ociwrapper

vvoland commented Aug 24, 2023 •

edited

Loading

Uh oh!

vvoland Aug 24, 2023

Uh oh!

vvoland Aug 24, 2023

Uh oh!

thaJeztah Aug 24, 2023

Uh oh!

rumpl Aug 24, 2023

Uh oh!

vvoland Aug 25, 2023

Uh oh!

thaJeztah commented Aug 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

vvoland commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vvoland Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

vvoland Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

rumpl Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

vvoland Aug 25, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vvoland commented Aug 24, 2023 •

edited

Loading