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.

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

c8d/list: Fix Total size calculation #48330

Merged
merged 3 commits into from
Aug 14, 2024
+101 −22
Merged

c8d/list: Fix Total size calculation #48330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
469c2ef
c8d/list: Fix `Total` size calculation
vvoland Aug 14, 2024
495fab8
c8d/list: Add test for total and content size
vvoland Aug 14, 2024
6bb6bef
c8d: Rename readConfig to readJSON
vvoland Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion daemon/containerd/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func (c cacheAdaptor) Get(id image.ID) (*image.Image, error) {
}

var config container.Config
if err := readConfig(ctx, c.is.content, configDesc, &config); err != nil {
if err := readJSON(ctx, c.is.content, configDesc, &config); err != nil {
if !errdefs.IsNotFound(err) {
log.G(ctx).WithFields(log.Fields{
"configDigest": dgst,
Expand Down
35 changes: 21 additions & 14 deletions daemon/containerd/image_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ func (i *ImageService) imageSummary(ctx context.Context, img images.Image, platf
} else {
mfstSummary.Size.Content = contentSize
totalSize += contentSize
mfstSummary.Size.Total = totalSize
mfstSummary.Size.Total += contentSize
}

isPseudo, err := img.IsPseudoImage(ctx)
Expand Down Expand Up @@ -322,27 +322,34 @@ func (i *ImageService) imageSummary(ctx context.Context, img images.Image, platf

chainIDs := identity.ChainIDs(dockerImage.RootFS.DiffIDs)

prevContentSize := contentSize
unpackedSize, contentSize, err := i.singlePlatformSize(ctx, img)
unpackedSize, imgContentSize, err := i.singlePlatformSize(ctx, img)
if err != nil {
logger.WithError(err).Warn("failed to determine platform specific size")
return nil
}

// If the image-specific content size calculation produces different result
// than the "generic" one, adjust the total size with the difference.
if prevContentSize != contentSize {
// Note: This shouldn't happen unless the implementation changes or the
// content is added/removed during the list operation.
if contentSize != imgContentSize {
logger.WithFields(log.Fields{
"prevSize": prevContentSize,
"contentSize": contentSize,
}).Debug("content size calculation mismatch")
"contentSize": contentSize,
"imgContentSize": imgContentSize,
}).Warn("content size calculation mismatch")

totalSize += contentSize - prevContentSize
mfstSummary.Size.Content = contentSize

// contentSize was already added to total, adjust it by the difference
// between the newly calculated size and the old size.
d := imgContentSize - contentSize
Copy link
Member

Choose a reason for hiding this comment

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

Any risk of contentSize being larger than imgContentSize here (so getting negative values?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly, and that's also fine here - if imgContentSize is larger then adding a negative will still adjust the total size correctly.

Copy link
Member

Choose a reason for hiding this comment

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

I had the exact same thought, and then realized it was fine either way 😅

Copy link
Member

Choose a reason for hiding this comment

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

Ha! Yes, it just crossed my mind, so thought I'd double check. Thanks both!

totalSize += d
mfstSummary.Size.Total += d
}

totalSize += unpackedSize
mfstSummary.Size.Total = totalSize
mfstSummary.ImageData.Size.Unpacked = unpackedSize
mfstSummary.Size.Total += unpackedSize
totalSize += unpackedSize

allChainsIDs = append(allChainsIDs, chainIDs...)

Expand Down Expand Up @@ -467,7 +474,7 @@ func (i *ImageService) singlePlatformImage(ctx context.Context, contentStore con
return nil, err
}
var cfg configLabels
if err := readConfig(ctx, contentStore, cfgDesc, &cfg); err != nil {
if err := readJSON(ctx, contentStore, cfgDesc, &cfg); err != nil {
return nil, err
}

Expand Down Expand Up @@ -669,7 +676,7 @@ func setupLabelFilter(ctx context.Context, store content.Store, fltrs filters.Ar
return nil, nil
}
var cfg configLabels
if err := readConfig(ctx, store, desc, &cfg); err != nil {
if err := readJSON(ctx, store, desc, &cfg); err != nil {
if errdefs.IsNotFound(err) {
return nil, nil
}
Expand Down Expand Up @@ -744,8 +751,8 @@ func computeSharedSize(chainIDs []digest.Digest, layers map[digest.Digest]int, s
return sharedSize, nil
}

// readConfig reads content pointed by the descriptor and unmarshals it into a specified output.
func readConfig(ctx context.Context, store content.Provider, desc ocispec.Descriptor, out interface{}) error {
// readJSON reads content pointed by the descriptor and unmarshals it into a specified output.
func readJSON(ctx context.Context, store content.Provider, desc ocispec.Descriptor, out interface{}) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps it could be extracted to the github.com/containerd/containerd/content package.

There's already content.ReadBlob , so I guess it could be named content.ReadJSONBlob.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, except our version also translates the cerrdefs.NotFound into errdefs.NotFound

Copy link
Member

Choose a reason for hiding this comment

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

Ah, except our version also translates the cerrdefs.NotFound into errdefs.NotFound

That problem should probably go away at some point; at least, Derek is working on unifying errdefs to make them interoperable.

data, err := content.ReadBlob(ctx, store, desc)
if err != nil {
err = errors.Wrapf(err, "failed to read config content")
Expand Down
70 changes: 70 additions & 0 deletions daemon/containerd/image_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,76 @@ func BenchmarkImageList(b *testing.B) {
}
}

func TestImageListCheckTotalSize(t *testing.T) {
ctx := namespaces.WithNamespace(context.TODO(), "testing")

blobsDir := t.TempDir()
cs := &blobsDirContentStore{blobs: filepath.Join(blobsDir, "blobs/sha256")}

twoplatform, mfstsDescs, err := specialimage.MultiPlatform(blobsDir, "test:latest", []ocispec.Platform{
{OS: "linux", Architecture: "arm64"},
{OS: "linux", Architecture: "amd64"},
})
assert.NilError(t, err)

ctx = logtest.WithT(ctx, t)
service := fakeImageService(t, ctx, cs)

_, err = service.images.Create(ctx, imagesFromIndex(twoplatform)[0])
assert.NilError(t, err)

all, err := service.Images(ctx, imagetypes.ListOptions{Manifests: true})
assert.NilError(t, err)

assert.Check(t, is.Len(all, 1))
assert.Check(t, is.Len(all[0].Manifests, 2))

// TODO: The test snapshotter doesn't do anything, so the size is always 0.
assert.Check(t, is.Equal(all[0].Manifests[0].ImageData.Size.Unpacked, int64(0)))
assert.Check(t, is.Equal(all[0].Manifests[1].ImageData.Size.Unpacked, int64(0)))

mfstArm64 := mfstsDescs[0]
mfstAmd64 := mfstsDescs[1]

indexSize := blobSize(t, ctx, cs, twoplatform.Manifests[0].Digest)
arm64ManifestSize := blobSize(t, ctx, cs, mfstArm64.Digest)
amd64ManifestSize := blobSize(t, ctx, cs, mfstAmd64.Digest)

var arm64Mfst, amd64Mfst ocispec.Manifest
assert.NilError(t, readJSON(ctx, cs, mfstArm64, &arm64Mfst))
assert.NilError(t, readJSON(ctx, cs, mfstAmd64, &amd64Mfst))

// MultiPlatform should produce a single layer. If these fail, the test needs to be adjusted.
assert.Assert(t, is.Len(arm64Mfst.Layers, 1))
assert.Assert(t, is.Len(amd64Mfst.Layers, 1))

arm64ConfigSize := blobSize(t, ctx, cs, arm64Mfst.Config.Digest)
amd64ConfigSize := blobSize(t, ctx, cs, amd64Mfst.Config.Digest)

arm64LayerSize := blobSize(t, ctx, cs, arm64Mfst.Layers[0].Digest)
amd64LayerSize := blobSize(t, ctx, cs, amd64Mfst.Layers[0].Digest)

allTotalSize := indexSize +
arm64ManifestSize + amd64ManifestSize +
arm64ConfigSize + amd64ConfigSize +
arm64LayerSize + amd64LayerSize

assert.Check(t, is.Equal(all[0].Size, allTotalSize-indexSize))

assert.Check(t, is.Equal(all[0].Manifests[0].Size.Content, arm64ManifestSize+arm64ConfigSize+arm64LayerSize))
assert.Check(t, is.Equal(all[0].Manifests[1].Size.Content, amd64ManifestSize+amd64ConfigSize+amd64LayerSize))

// TODO: This should also include the Size.Unpacked, but the test snapshotter doesn't do anything yet
assert.Check(t, is.Equal(all[0].Manifests[0].Size.Total, amd64ManifestSize+amd64ConfigSize+amd64LayerSize))
assert.Check(t, is.Equal(all[0].Manifests[1].Size.Total, amd64ManifestSize+amd64ConfigSize+amd64LayerSize))
}

func blobSize(t *testing.T, ctx context.Context, cs content.Store, dgst digest.Digest) int64 {
info, err := cs.Info(ctx, dgst)
assert.NilError(t, err)
return info.Size
}

func TestImageList(t *testing.T) {
ctx := namespaces.WithNamespace(context.TODO(), "testing")

Expand Down
2 changes: 1 addition & 1 deletion daemon/containerd/image_manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,5 +236,5 @@ func (m *ImageManifest) ReadConfig(ctx context.Context, outConfig interface{}) e
return err
}

return readConfig(ctx, m.ContentStore(), configDesc, outConfig)
return readJSON(ctx, m.ContentStore(), configDesc, outConfig)
}
2 changes: 1 addition & 1 deletion daemon/containerd/image_push_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func TestImagePushIndex(t *testing.T) {
imgSvc.defaultPlatformOverride = platforms.Only(defaultDaemonPlatform)
}

idx, err := specialimage.MultiPlatform(csDir, "multiplatform:latest", tc.indexPlatforms)
idx, _, err := specialimage.MultiPlatform(csDir, "multiplatform:latest", tc.indexPlatforms)
assert.NilError(t, err)

imgs := imagesFromIndex(idx)
Expand Down
3 changes: 2 additions & 1 deletion integration/image/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,8 @@ func TestAPIImagesListManifests(t *testing.T) {
{OS: "darwin", Architecture: "arm64"},
}
specialimage.Load(ctx, t, apiClient, func(dir string) (*ocispec.Index, error) {
return specialimage.MultiPlatform(dir, "multiplatform:latest", testPlatforms)
idx, _, err := specialimage.MultiPlatform(dir, "multiplatform:latest", testPlatforms)
return idx, err
})

t.Run("unsupported before 1.47", func(t *testing.T) {
Expand Down
9 changes: 5 additions & 4 deletions internal/testutils/specialimage/multiplatform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ import (
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
)

func MultiPlatform(dir string, imageRef string, imagePlatforms []ocispec.Platform) (*ocispec.Index, error) {
func MultiPlatform(dir string, imageRef string, imagePlatforms []ocispec.Platform) (*ocispec.Index, []ocispec.Descriptor, error) {
ref, err := reference.ParseNormalizedNamed(imageRef)
if err != nil {
return nil, err
return nil, nil, err
}

var descs []ocispec.Descriptor
Expand All @@ -19,14 +19,15 @@ func MultiPlatform(dir string, imageRef string, imagePlatforms []ocispec.Platfor
ps := platforms.Format(platform)
manifestDesc, err := oneLayerPlatformManifest(dir, platform, FileInLayer{Path: "bash", Content: []byte("layer-" + ps)})
if err != nil {
return nil, err
return nil, nil, err
}
descs = append(descs, manifestDesc)
}

return multiPlatformImage(dir, ref, ocispec.Index{
idx, err := multiPlatformImage(dir, ref, ocispec.Index{
Versioned: specs.Versioned{SchemaVersion: 2},
MediaType: ocispec.MediaTypeImageIndex,
Manifests: descs,
})
return idx, descs, err
}
Loading