Skip to content

containerd integration: docker commit #44958

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.

Sign up for GitHub

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

Jump to bottom
Merged
thaJeztah merged 2 commits into from
Mar 6, 2023
Merged

containerd integration: docker commit #44958

thaJeztah merged 2 commits into from
Mar 6, 2023

Conversation

@laurazard
Copy link
Member

@laurazard laurazard commented Feb 8, 2023 •
edited
Loading

- What I did

Implement docker commit with containerd store

- How I did it

Upstream (with some adjustments):

- How to verify it

(run a container and create a file)

  • docker run -it --name hello alpine
  • / # echo world > world.txt && exit
  • docker commit hello hello/world

(verify file was written to new image)

  • docker run hello/world cat world.txt

- Description for the changelog

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

image

laurazard
laurazard commented Feb 9, 2023
View reviewed changes
@thaJeztah thaJeztah added status/2-code-review area/images Image Distribution containerd-integration Issues and PRs related to containerd integration labels Feb 9, 2023
@thaJeztah thaJeztah added this to the v-next milestone Feb 9, 2023
AkihiroSuda
AkihiroSuda reviewed Feb 9, 2023
View reviewed changes
@neersighted neersighted mentioned this pull request Feb 9, 2023
Merged
@laurazard laurazard mentioned this pull request Feb 13, 2023
Merged
@laurazard
Copy link
Member Author

laurazard commented Feb 25, 2023 •
edited
Loading

Just pushed a commit addressing the issues discussed in the maintainers call (and referenced in #44934 (comment)).

It resolves the root issue of not being able to deterministically resolve the image variant used to run a container after the container is created by storing the digest of the image-variant digest, and then using that later when necessary to fetch the content from the store and using that instead of making assumptions about the platform.

I'd appreciate some input about the design itself - I tried avoiding introducing another daemon.UsesSnapshotter() and a containerd specific ImageService method, but in the end this seemed the cleanest way to (for now) achieve the desired outcome. Maybe in the future another abstraction will make more sense. The specific refactor code is in this commit – 041da7a. We can squash it later, but I thought it better to leave it separate for now to make discussion clearer.

cc @neersighted @thaJeztah

thaJeztah
thaJeztah reviewed Mar 2, 2023
View reviewed changes
tianon
tianon reviewed Mar 2, 2023
View reviewed changes
vvoland
vvoland reviewed Mar 2, 2023
View reviewed changes
tianon
tianon reviewed Mar 2, 2023
View reviewed changes
@laurazard laurazard force-pushed the c8d-docker-commit branch from 041da7a to 49e55ad Compare March 5, 2023 16:01
@laurazard laurazard requested review from neersighted, thaJeztah, tianon and vvoland and removed request for neersighted, thaJeztah, tianon and vvoland March 5, 2023 16:04
@laurazard
Copy link
Member Author

laurazard commented Mar 5, 2023 •
edited
Loading

Github is having a tough time with the review requests 👀 Only keeps the latest 😬

@laurazard laurazard requested review from tianon and removed request for neersighted March 5, 2023 16:05
@laurazard laurazard force-pushed the c8d-docker-commit branch from 49e55ad to d76bb73 Compare March 5, 2023 17:38
@laurazard
Copy link
Member Author

laurazard commented Mar 6, 2023

Oooooops, didn't realize @neersighted managed to request the reviews (as they hadn't updated on my end) and they disappeared again when I tried to do them 😨 Github why.

vvoland
vvoland approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM; there's just one little debug log that's left 😄

@laurazard laurazard force-pushed the c8d-docker-commit branch from 843c002 to 2bb51ea Compare March 6, 2023 10:08
@thaJeztah
Copy link
Member

thaJeztah commented Mar 6, 2023

code LGTM; given that the first 5 commits are mostly fixing up previous changes that were added in this PR, perhaps we should squash the first 5 ones (last one may be ok to keep separate, as it's additional functionality); WDYT @vvoland @laurazard ?

@vvoland
Copy link
Contributor

vvoland commented Mar 6, 2023

+1 on squashing

ndeloof and others added 2 commits March 6, 2023 15:11
@ndeloof @laurazard
@thaJeztah @vvoland
Introduce support for docker commit
168ca2d 
Signed-off-by: Laura Brehm <[email protected]>
Co-authored-by: Laura Brehm <[email protected]>
Co-authored-by: Sebastiaan van Stijn <[email protected]>
Co-authored-by: Paweł Gronowski <[email protected]>
Co-authored-by: Nicolas De Loof <[email protected]>
@laurazard
Resolve and store manifest when creating container
a34060c 
This addresses the previous issue with the containerd store where, after a container is created, we can't deterministically resolve which image variant was used to run it (since we also don't store what platform the image was fetched for).

This is required for things like `docker commit`, and computing the containers layer size later, since we need to resolve the specific image variant.

Signed-off-by: Laura Brehm <[email protected]>
@laurazard laurazard force-pushed the c8d-docker-commit branch from 2bb51ea to a34060c Compare March 6, 2023 14:14
@laurazard
Copy link
Member Author

laurazard commented Mar 6, 2023

Sound good to me :) Squashed all the previous commits, and kept the last one separate @thaJeztah

thaJeztah
thaJeztah approved these changes Mar 6, 2023
View reviewed changes
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, thanks!

@thaJeztah
Copy link
Member

thaJeztah commented Mar 6, 2023

Hmm... looks like CI completed, but still shows as "running" on GitHub? https://github.com/moby/moby/actions/runs/4344357044/jobs/7587778593

Screenshot 2023-03-06 at 16 18 28

Screenshot 2023-03-06 at 16 19 48

@thaJeztah
Copy link
Member

thaJeztah commented Mar 6, 2023

Oh! Maybe that's because #44934 also includes that commit, which confuses GHA?

@laurazard
Copy link
Member Author

laurazard commented Mar 6, 2023 •
edited
Loading

Just Github things 😅

Edit: it's greeeeen 🟢

@thaJeztah
Copy link
Member

thaJeztah commented Mar 6, 2023

All green now! 🎉

@thaJeztah thaJeztah merged commit 6f719c7 into moby:master Mar 6, 2023
@vvoland vvoland mentioned this pull request Mar 7, 2023
Merged
@laurazard laurazard mentioned this pull request Mar 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tianon tianon tianon left review comments

@cpuguy83 cpuguy83 cpuguy83 left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@neersighted neersighted neersighted approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@vvoland vvoland vvoland approved these changes

Assignees

No one assigned

Labels

area/images Image Distribution containerd-integration Issues and PRs related to containerd integration status/4-merge

Projects

containerd integration
Status: Done

Milestone

24.0.0

Development

Successfully merging this pull request may close these issues.

8 participants

@laurazard @thaJeztah @vvoland @tianon @cpuguy83 @neersighted @AkihiroSuda @ndeloof