Skip to content

containerd integration: compute containers layer size #44934

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 1 commit into from
Mar 8, 2023
Merged

containerd integration: compute containers layer size #44934

thaJeztah merged 1 commit into from
Mar 8, 2023

Conversation

@laurazard
Copy link
Member

@laurazard laurazard commented Feb 6, 2023 •
edited
Loading

- What I did

Implement GetContainerLayerSize using the containerd snapshotter

Upstreams:

- How I did it

- How to verify it

After #44804 is merged, run:

docker run --name hello hello-world
docker inspect -s hello | grep SizeRootFs

- Description for the changelog

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

image

@rumpl rumpl added area/images Image Distribution containerd-integration Issues and PRs related to containerd integration labels Feb 6, 2023
@laurazard laurazard marked this pull request as ready for review February 7, 2023 09:50
neersighted
neersighted reviewed Feb 9, 2023
View reviewed changes
@laurazard laurazard mentioned this pull request Feb 13, 2023
Merged
@laurazard laurazard mentioned this pull request Feb 25, 2023
Merged
@thaJeztah thaJeztah added this to the v-next milestone Mar 6, 2023
@laurazard laurazard force-pushed the c8d-layer-size branch 2 times, most recently from 918d65c to 4bd1444 Compare March 6, 2023 15:02
@laurazard
Copy link
Member Author

laurazard commented Mar 6, 2023

Rebased on top of #44958, which includes the necessary refactors to address @neersighted's comment's here – #44934 (comment).

Only the last commit includes actual relevant changes for this PR, the others come from #44958

@laurazard laurazard requested a review from neersighted March 6, 2023 15:29
neersighted
neersighted reviewed Mar 6, 2023
View reviewed changes
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Overall LGTM, some nits/questions.

@laurazard laurazard requested a review from neersighted March 6, 2023 15:57
neersighted
neersighted approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

I think the return to usage.Size is actually quite helpful here as it adds symmetry with snapshotSizeFn -- thanks for letting me pick your brain; LGTM with the tweaks.

thaJeztah
thaJeztah reviewed Mar 6, 2023
View reviewed changes
@laurazard laurazard mentioned this pull request Mar 7, 2023
Merged
thaJeztah
thaJeztah reviewed Mar 7, 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.

Thanks! Sorry for the delay; code overall looks good to me. While looking at it locally I saw some variables shadowing imports, and some suggestions on moving some code closer to where it's used.

Not necessarily a blocker, but let me know what you think 😅

cpuguy83
cpuguy83 approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM other than existing comments.

@laurazard @thaJeztah
c8d: Compute container's layer size
45ee4d7 
Co-authored-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
@thaJeztah
Copy link
Member

thaJeztah commented Mar 8, 2023

@laurazard is traveling today, so I just pushed my suggestion from above; https://github.com/moby/moby/compare/a64380b47f70a7335d08cc8aae9af8e4f619e99a..45ee4d7c78d4d128366f372c4657911880571528

@cpuguy83 @neersighted ptal if those LGTY (then I think we can merge this one)

thaJeztah
thaJeztah approved these changes Mar 8, 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 as well 😅

@thaJeztah thaJeztah merged commit d7e5708 into moby:master Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@neersighted neersighted neersighted approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@rumpl rumpl Awaiting requested review from rumpl

@vvoland vvoland Awaiting requested review from vvoland

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

@tianon tianon Awaiting requested review from tianon

Assignees

No one assigned

Labels

area/images Image Distribution containerd-integration Issues and PRs related to containerd integration status/2-code-review

Projects

containerd integration
Status: Done

Milestone

24.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@laurazard @thaJeztah @rumpl @cpuguy83 @neersighted