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

fixe image size #789

Merged
merged 1 commit into from
Feb 14, 2022
Merged

Conversation

fahedouch
Copy link
Member

@fahedouch fahedouch commented Feb 7, 2022

fixing #777
ImageSize is the size of the unpacked snapshots. for multi-platform image, snapshot host single platform layer(s). So the computing of image size shouldn't be based on image platform

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

@fahedouch fahedouch force-pushed the fix-image-size-output branch 2 times, most recently from 0829c5e to d057276 Compare February 7, 2022 14:13
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.

This only shows the size of the default platform

@@ -314,8 +314,8 @@ func (key snapshotKey) add(ctx context.Context, s snapshots.Snapshotter, usage *

// unpackedImageSize is the size of the unpacked snapshots.
// Does not contain the size of the blobs in the content store. (Corresponds to Docker).
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should show both the blob size and the unpacked size to avoid confusion for multi-platform images

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@fahedouch
Copy link
Member Author

related issue #796

@fahedouch fahedouch force-pushed the fix-image-size-output branch 2 times, most recently from eb61341 to 5a3e74f Compare February 13, 2022 19:39
@@ -109,7 +109,8 @@ type imagePrintable struct {
ID string // image target digest (not config digest, unlike Docker), or its short form
Repository string
Tag string // "<none>" or tag
Size string
UnpackedSize string // the size of the unpacked snapshots.
Copy link
Member

Choose a reason for hiding this comment

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

the Size field needs to be retained for Docker compatibility

} else {
fmt.Fprintln(w, "REPOSITORY\tTAG\tIMAGE ID\tCREATED\tPLATFORM\tSIZE")
fmt.Fprintln(w, "REPOSITORY\tTAG\tIMAGE ID\tCREATED\tPLATFORM\tSIZE\tBLOB_SIZE")
Copy link
Member

Choose a reason for hiding this comment

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

Do we use underscore in other places?

Copy link
Member Author

@fahedouch fahedouch Feb 14, 2022

Choose a reason for hiding this comment

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

@AkihiroSuda I see spaces , BLOB SIZE WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

looks good

@AkihiroSuda AkihiroSuda added this to the v0.17.0 milestone Feb 14, 2022
Signed-off-by: fahed dorgaa <fahed.dorgaa@gmail.com>
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

@AkihiroSuda
Copy link
Member

nerdctl images might be slower than before (which has been already slow).

We should do some benchmark and probably have --quick (--fast?, idk) flag to skip slow size calculation

@fahedouch
Copy link
Member Author

fahedouch commented Feb 14, 2022

@AkihiroSuda I think we can merge this, I will open a new PR for benchmark improvement

@AkihiroSuda AkihiroSuda merged commit 76e8d23 into containerd:master Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants