Merge pull request moby#48330 from vvoland/c8d-list-multiplatform-fix…

…size

c8d/list: Fix `Total` size calculation

daemon/containerd/cache.go

@@ -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,

daemon/containerd/image_list.go

@@ -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)
@@ -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
+			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...)
 
@@ -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
 	}
 
@@ -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
 				}
@@ -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 {
 	data, err := content.ReadBlob(ctx, store, desc)
 	if err != nil {
 		err = errors.Wrapf(err, "failed to read config content")

daemon/containerd/image_list_test.go

@@ -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")
 

daemon/containerd/image_manifest.go

@@ -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)
 }

daemon/containerd/image_push_test.go

@@ -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)

integration/image/list_test.go

@@ -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) {

internal/testutils/specialimage/multiplatform.go

@@ -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
@@ -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
 }