Conversation
| } | ||
| } | ||
|
|
||
| size, err := image.Size(ctx) |
There was a problem hiding this comment.
Question (as I'm not 100% sure); how does virtualSize and image.Size() differ?
I know in the past (before docker 1.10), virtualSize was needed to calculate the total size of the image based on image + parent image + parent image (etc), but since 1.10, it's just the sum of layers. And (IIRC) in moby we just set both virtualSize and size to the same value.
What does image.Size() return? Isn't that already the same as virtualSize?
There was a problem hiding this comment.
based on https://github.com/containerd/nerdctl/blob/b140bc1c7ace3f7c60fbbeae9dc16ec95687f4a1/cmd/nerdctl/images.go#L47 (API docs don't really help here)
Size returns the size of (compressed) blobs, i.e. image data transfert size
VirtualSize we compute here returns the size of uncompressed data (not considering some layers being shared between images)
There was a problem hiding this comment.
Ah, gotcha. Hmm.. yes, so we don't have a concept of compressed size in "old" moby, so yup.
(Perhaps something we could add in containerd as well, to have options for both compressed and uncompressed size of an image)
Thanks for explaining!
I think the filters should only affect "what's shown". If an image shares layers with another image that's not shown, it's still a shared layer (?), so |
daemon/containerd/service.go
Outdated
| if layers[chainID] == 1 { | ||
| continue | ||
| } | ||
| usage, err := snapshotter.Usage(ctx, chainID.String()) |
There was a problem hiding this comment.
Last question (probably ok for a follow-up); is this a heavy operation? Asking because it looks like we potentially call this multiple times for the same chainID (i.e., especially if some layers are shared many times).
If that's the case we could look at (temporarily) storing the results in a map and/or use sync/singleflight
There was a problem hiding this comment.
I have no idea, ctrd API is a black box to me
There was a problem hiding this comment.
From the docs of this function:
The running time of this call for active snapshots is dependent on implementation, but may be proportional to the size of the resource. Callers should take this into consideration.
I also found some quite recent blog post mentioning that btrfs snapshotter causes high CPU usage due to the regular collection of disk usage, which probably also uses the same code path as snapshotter.Usage - https://blog.cubieserver.de/2022/dont-use-containerd-with-the-btrfs-snapshotter/
So it probably would make sense to cache this because it can be slow.
There was a problem hiding this comment.
Thanks! I didn't dig deep yet before I wrote that comment, but saw that this function is part of the interface that's implemented by all snapshotters.
And based on @vvoland's comment, I fear that means "traversing the filesystem for all files in a snapshot", which can (depending on what's in each layer) a lot.
So, yes, we should at least cache it temporarily (docker image ls --size), to prevent calculating the size multiple times in a single invocation (that one should be relatively easy with a map[id]size), but possibly even consider storing it permanently (as layers should be immutable, so their size should never change). Not sure where to store it, and how to make sure that storage is purged when the layer is removed though.
I'm a bit fuzzy how we handled this in the existing (non-snapshotter) image/layer store, but perhaps we can have a look how we did it there.
daemon/containerd/service.go
Outdated
| type snapshotSizeFn func(ctx context.Context, d digest.Digest) (int64, error) | ||
|
|
||
| type snapshotSizer struct { | ||
| snapshotter snapshots.Snapshotter | ||
| cache map[digest.Digest]int64 | ||
| } | ||
|
|
||
| func (s *snapshotSizer) size(ctx context.Context, d digest.Digest) (int64, error) { | ||
| if s, ok := s.cache[d]; ok { | ||
| return s, nil | ||
| } | ||
| usage, err := snapshotter.Usage(ctx, d.String()) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| sizeCache[d] = usage.Size | ||
| return usage.Size, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
yes, pushed by mistake while refactoring :P
There was a problem hiding this comment.
I thought that was the case; I started looking at that code, wondering if / when we would reset the cache, and then realised "oh! but I don't think it's used" 😂
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
| if err != nil { | ||
| return 0, err | ||
| } | ||
| sizeCache[d] = usage.Size |
There was a problem hiding this comment.
👍 I think this should do "for now". We may want to create a ticket to optimise this more in-depth (concurrent use, perhaps contribute to containerd to persist this information as metadata, and make it generally available etc), but I don't think that's a blocker for the PoC.
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM (assuming green), thanks!
- What I did
compute "shared size" by counting snapshots usage while listing images
to be addressed: I'm not sure how this should interact with filters: shall we count as "shared" some layers that are excluded, based on selected filters, or are those metrics "within the scope of the filter" ?