Skip to content
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.

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

fix image size #310

Merged
merged 1 commit into from
Aug 18, 2021
Merged

fix image size #310

merged 1 commit into from
Aug 18, 2021

Conversation

fahedouch
Copy link
Member

the actual image size is equal to the ChainID size.

this PR use the Walk API and Usage on each parent of ChainID to calculate the total usage of unpacked image layers

related issue #304

Signed-off-by: fahed dorgaa fahed.dorgaa@gmail.com

@AkihiroSuda AkihiroSuda requested a review from ktock August 8, 2021 15:37
Copy link
Member

@ktock ktock 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 but minor nits

cmd/nerdctl/images.go Outdated Show resolved Hide resolved
cmd/nerdctl/images.go Outdated Show resolved Hide resolved
cmd/nerdctl/images.go Outdated Show resolved Hide resolved
@fahedouch fahedouch force-pushed the fix-images-size branch 3 times, most recently from 6cdffbb to 7048508 Compare August 9, 2021 09:47
@fahedouch
Copy link
Member Author

@ktock done

Comment on lines 179 to 189
walkFn := func(ctx context.Context, info snapshots.Info) error {
keys[info.Name] = info.Parent
return nil
}
if err := s.Walk(ctx, walkFn); err != nil {
return 0, err
}
Copy link
Member

Choose a reason for hiding this comment

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

Why does it scan all snapshots? Can we walk snapshots only in the chain? I think we can do this by following parents from the topmost snapshot using Stat API.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ktock thank you, This is what I was looking for ==> Stat API . I made modification.

Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
@AkihiroSuda
Copy link
Member

CI wasn't triggered?

@@ -143,19 +146,51 @@ func imagesBashComplete(clicontext *cli.Context) {
bashCompleteImageNames(clicontext)
}

func unpackedImageSize(ctx context.Context, clicontext *cli.Context, client *containerd.Client, i images.Image) (int64, error) {
type snapshotKey string
Copy link
Member

@AkihiroSuda AkihiroSuda Aug 12, 2021

Choose a reason for hiding this comment

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

Why define new type just for a string?

Copy link
Member Author

@fahedouch fahedouch Aug 12, 2021

Choose a reason for hiding this comment

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

I have to define new local type to use it as a receiver to the add method. We cannot define the add method on non-local type string

@AkihiroSuda AkihiroSuda reopened this Aug 12, 2021
@AkihiroSuda AkihiroSuda added this to the v0.12.0 milestone Aug 12, 2021
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants