Add support for calculating virtual image size as well. #1918

manugupt1 · 2023-01-24T05:52:51Z

Testing details

➜  nerdctl git:(virtual-size) ✗ nerdctl image list --format 'table ID:{{.ID}};Repo:{{.Repository}};Tag:{{.Tag}};Size:{{.Size}};VirtSize:{{.VirtualSize}}'

 table ID:f271e74b17ce;Repo:alpine;Tag:latest;Size:8.3 MiB;VirtSize:8.3 MiB table ID:3487c98481d1;Repo:fedora;Tag:latest;Size:195.1 MiB;VirtSize:195.1 MiB

AkihiroSuda · 2023-01-24T06:06:39Z

Size:8.3 MiB;VirtSize:8.3 MiB

This shouldn't be same?

AkihiroSuda · 2023-01-24T06:08:54Z

pkg/cmd/image/list.go

+// https://github.com/moby/moby/blob/65c371ed6e48d0014a4826a6933f9231f688327a/daemon/containerd/image_list.go#L184-L194
+// What is VirtualSize: https://github.com/docker/docs/issues/1520#issuecomment-305179362
+func computeVirtualSize(ctx context.Context, s snapshots.Snapshotter, img containerd.Image) (int64, error) {
+	chainIDs, err := img.RootFS(ctx)


diffIDs, not chainID
https://github.com/opencontainers/image-spec/blob/v1.1.0-rc2/config.md#layer-chainid

AkihiroSuda · 2023-01-24T06:09:30Z

pkg/cmd/image/list.go

+	}
+	var virtualSize int64
+	for _, chainID := range chainIDs {
+		usage, err := s.Usage(ctx, chainID.String())


Can we cache this to avoid calculating the usage twice

manugupt1 · 2023-01-24T06:15:04Z

docker/docs#1520 (comment)

See: docker/docs#16315
I think for images it is the same. For containers when we have a writeable layer; it will change.

Also this: moby/moby#43862 :/

manugupt1 · 2023-01-25T06:04:25Z

@AkihiroSuda I made the change equivalent to moby's containerd implementation. Should we keep blob size now or remove it?
This is sort of a breaking change; will you like to keep the previous behavior or maintain compatibility with moby.

manugupt1 · 2023-02-05T03:08:21Z

@fahedouch Can you please have a look? Let me know what do you think?

fahedouch · 2023-02-07T20:53:21Z

pkg/cmd/image/list.go

-	//add ChainID's parent usage to the total usage
-	if err := snapshotKey(info.Parent).add(ctx, s, &usage); err != nil {
-		return 0, err
+		virtualSize += usage.Size


is the container writable layer counted here?

For images; it is zero; so it really does not matter.
For containers; the writable layer size will be added.

@manugupt1

My understanding is that VirtualSize = blob size + writable layer size

but here you are not counting the writable layer size (before your modification it was counted by adding the parent of most top DiffId size to the total size)

What I did in the past is

calculate the chainID of the readonly layer

add chainID size to the total size

calculate the chainID parent size ( writable layer)

add the chainID parent size to the total size

info, err := s.Stat(ctx, chainID) if err != nil { return 0, err } //add ChainID's parent usage to the total usage if err := snapshotKey(info.Parent).add(ctx, s, &usage); err != nil {

@fahedouch Sorry for the late response. I have updated this PR; the previous behavior was correct and when I read through the moby issue; this is the behavior that we want.
However, now containerd provides withSnapshotUsage so we can re-use it. I have refactored the code and also added the field for VirtualSize.
Please let me know if this sounds good to you. I can write tests if this sounds good to you.

fahedouch · 2023-02-07T20:59:16Z

pkg/cmd/image/list.go

@@ -304,7 +303,8 @@ func (x *imagePrinter) printImageSinglePlatform(ctx context.Context, img images.
 		Tag:          tag,
 		Name:         img.Name,
 		Size:         progress.Bytes(size).String(),
-		BlobSize:     progress.Bytes(blobSize).String(),
+		VirtualSize:  progress.Bytes(virtualSize).String(),
+		BlobSize:     progress.Bytes(size).String(), // Was introduced since docker had a different behavior earlier.


do we really need BlobSize here ? I think we should keep either size or BlobSize

I think we should keep Size (needed your opinion on it).
This will make tests docker incompatible until docker moves to containerd.

fahedouch · 2023-02-07T21:04:11Z

pkg/cmd/image/list.go

@@ -177,7 +175,8 @@ type imagePrintable struct {
 	Name         string // image name
 	Size         string // the size of the unpacked snapshots.


since your modification // the size of the unpacked snapshots. => wrong

fahedouch

Could you please check my comment above.

Signed-off-by: Manu Gupta <manugupt1@gmail.com>

AkihiroSuda · 2023-03-14T04:03:48Z

pkg/cmd/image/list.go

@@ -245,6 +246,7 @@ func (x *imagePrinter) printImageSinglePlatform(ctx context.Context, img images.
 		Tag:          tag,
 		Name:         img.Name,
 		Size:         progress.Bytes(size).String(),
+		VirtualSize:  progress.Bytes(size).String(),


Needs comment lines to explain why these are same

Also needs integration tests

yep! Wanted to get an initial look. Will add by the end of this week.
Thanks!

AkihiroSuda · 2023-03-14T04:04:06Z

pkg/cmd/image/list.go

@@ -119,6 +119,7 @@ type imagePrintable struct {
 	Name         string // image name
 	Size         string // the size of the unpacked snapshots.
 	BlobSize     string // the size of the blobs in the content store (nerdctl extension)
+	VirtualSize  string


Needs a comment line

AkihiroSuda · 2023-03-14T04:05:59Z

pkg/imgutil/imgutil.go

-	}
-	return usage.Size, nil
+func UnpackedImageSize(ctx context.Context, img containerd.Image) (int64, error) {
+	return img.Usage(ctx, containerd.WithSnapshotUsage())


Is this just a refactoring, or does this change the computed size?
Could you consider making this a separate commit/PR ?

It does not change the computed size at all. I verified locally.

manugupt1 · 2023-03-16T00:46:30Z

@fahedouch Ping

manugupt1 · 2023-03-19T20:28:03Z

Even the sizes are inconistent now with docker and nerdctl.
See: moby/moby#45180
Closing this.

fahedouch · 2023-03-19T21:02:56Z

@manugupt1 why closing this ?

manugupt1 · 2023-03-20T03:25:13Z

I mostly closed this because of moby/moby#45180
Also: As per moby: https://github.com/moby/moby/blob/7489b51f610104ab5acc43f4e77142927e7b522e/api/types/types.go#L128 this can be deprecated and no longer relevant.

manugupt1 force-pushed the virtual-size branch from 057538e to eb885bb Compare January 24, 2023 05:55

AkihiroSuda reviewed Jan 24, 2023

View reviewed changes

manugupt1 force-pushed the virtual-size branch 2 times, most recently from 28c774c to 2464f7f Compare January 25, 2023 06:02

manugupt1 requested a review from AkihiroSuda January 25, 2023 06:04

AkihiroSuda requested a review from fahedouch January 25, 2023 06:14

fahedouch reviewed Feb 7, 2023

View reviewed changes

fahedouch self-requested a review February 10, 2023 23:22

fahedouch reviewed Feb 15, 2023

View reviewed changes

manugupt1 force-pushed the virtual-size branch 3 times, most recently from 6866ce5 to 07a9347 Compare March 8, 2023 07:30

Add support for virtual size

f2ddf08

Signed-off-by: Manu Gupta <manugupt1@gmail.com>

manugupt1 force-pushed the virtual-size branch from 07a9347 to f2ddf08 Compare March 8, 2023 07:36

manugupt1 requested a review from fahedouch March 8, 2023 07:48

manugupt1 marked this pull request as ready for review March 10, 2023 03:09

manugupt1 added this to the v1.4.0 (tentative) milestone Mar 10, 2023

manugupt1 added the kind/refactor label Mar 10, 2023

AkihiroSuda reviewed Mar 14, 2023

View reviewed changes

manugupt1 closed this Mar 19, 2023

manugupt1 deleted the virtual-size branch March 19, 2023 20:28

AkihiroSuda removed this from the v1.3.1 milestone Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for calculating virtual image size as well. #1918

Add support for calculating virtual image size as well. #1918

manugupt1 commented Jan 24, 2023 •

edited

Loading

AkihiroSuda commented Jan 24, 2023

AkihiroSuda Jan 24, 2023

AkihiroSuda Jan 24, 2023

manugupt1 commented Jan 24, 2023 •

edited

Loading

manugupt1 commented Jan 25, 2023

manugupt1 commented Feb 5, 2023

fahedouch Feb 7, 2023

manugupt1 Feb 8, 2023

fahedouch Feb 15, 2023

manugupt1 Mar 8, 2023

fahedouch Feb 7, 2023

manugupt1 Feb 8, 2023

fahedouch Feb 15, 2023

fahedouch Feb 7, 2023 •

edited

Loading

fahedouch left a comment

AkihiroSuda Mar 14, 2023

AkihiroSuda Mar 14, 2023

manugupt1 Mar 16, 2023

AkihiroSuda Mar 14, 2023

AkihiroSuda Mar 14, 2023

manugupt1 Mar 16, 2023

manugupt1 commented Mar 16, 2023

manugupt1 commented Mar 19, 2023

fahedouch commented Mar 19, 2023

manugupt1 commented Mar 20, 2023 •

edited

Loading

		@@ -177,7 +175,8 @@ type imagePrintable struct {
		Name string // image name
		Size string // the size of the unpacked snapshots.

Add support for calculating virtual image size as well. #1918

Add support for calculating virtual image size as well. #1918

Conversation

manugupt1 commented Jan 24, 2023 • edited Loading

AkihiroSuda commented Jan 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manugupt1 commented Jan 24, 2023 • edited Loading

manugupt1 commented Jan 25, 2023

manugupt1 commented Feb 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fahedouch Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

fahedouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manugupt1 commented Mar 16, 2023

manugupt1 commented Mar 19, 2023

fahedouch commented Mar 19, 2023

manugupt1 commented Mar 20, 2023 • edited Loading

manugupt1 commented Jan 24, 2023 •

edited

Loading

manugupt1 commented Jan 24, 2023 •

edited

Loading

fahedouch Feb 7, 2023 •

edited

Loading

manugupt1 commented Mar 20, 2023 •

edited

Loading