Merge pull request #2551 from mtrmac/oci-delete-refactor

Refactor image deletion in OCI
containers · Sep 9, 2024 · 1a02a3d · 1a02a3d
2 parents 3ad7998 + f957a9e
commit 1a02a3d
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 113 deletions.
diff --git a/oci/layout/oci_delete.go b/oci/layout/oci_delete.go
@@ -27,17 +27,8 @@ func (ref ociReference) DeleteImage(ctx context.Context, sys *types.SystemContex
 		return err
 	}
 
-	var blobsUsedByImage map[digest.Digest]int
-
-	switch descriptor.MediaType {
-	case imgspecv1.MediaTypeImageManifest:
-		blobsUsedByImage, err = ref.getBlobsUsedInSingleImage(&descriptor, sharedBlobsDir)
-	case imgspecv1.MediaTypeImageIndex:
-		blobsUsedByImage, err = ref.getBlobsUsedInImageIndex(&descriptor, sharedBlobsDir)
-	default:
-		return fmt.Errorf("unsupported mediaType in index: %q", descriptor.MediaType)
-	}
-	if err != nil {
+	blobsUsedByImage := make(map[digest.Digest]int)
+	if err := ref.countBlobsForDescriptor(blobsUsedByImage, &descriptor, sharedBlobsDir); err != nil {
 		return err
 	}
 
@@ -54,82 +45,48 @@ func (ref ociReference) DeleteImage(ctx context.Context, sys *types.SystemContex
 	return ref.deleteReferenceFromIndex(descriptorIndex)
 }
 
-func (ref ociReference) getBlobsUsedInSingleImage(descriptor *imgspecv1.Descriptor, sharedBlobsDir string) (map[digest.Digest]int, error) {
-	manifest, err := ref.getManifest(descriptor, sharedBlobsDir)
-	if err != nil {
-		return nil, err
-	}
-	blobsUsedInManifest := ref.getBlobsUsedInManifest(manifest)
-	blobsUsedInManifest[descriptor.Digest]++ // Add the current manifest to the list of blobs used by this reference
-
-	return blobsUsedInManifest, nil
-}
-
-func (ref ociReference) getBlobsUsedInImageIndex(descriptor *imgspecv1.Descriptor, sharedBlobsDir string) (map[digest.Digest]int, error) {
+// countBlobsForDescriptor updates dest with usage counts of blobs required for descriptor, INCLUDING descriptor itself.
+func (ref ociReference) countBlobsForDescriptor(dest map[digest.Digest]int, descriptor *imgspecv1.Descriptor, sharedBlobsDir string) error {
 	blobPath, err := ref.blobPath(descriptor.Digest, sharedBlobsDir)
 	if err != nil {
-		return nil, err
-	}
-	index, err := parseIndex(blobPath)
-	if err != nil {
-		return nil, err
+		return err
 	}
 
-	blobsUsedInImageRefIndex := make(map[digest.Digest]int)
-	err = ref.addBlobsUsedInIndex(blobsUsedInImageRefIndex, index, sharedBlobsDir)
-	if err != nil {
-		return nil, err
+	dest[descriptor.Digest]++
+	switch descriptor.MediaType {
+	case imgspecv1.MediaTypeImageManifest:
+		manifest, err := parseJSON[imgspecv1.Manifest](blobPath)
+		if err != nil {
+			return err
+		}
+		dest[manifest.Config.Digest]++
+		for _, layer := range manifest.Layers {
+			dest[layer.Digest]++
+		}
+	case imgspecv1.MediaTypeImageIndex:
+		index, err := parseIndex(blobPath)
+		if err != nil {
+			return err
+		}
+		if err := ref.countBlobsReferencedByIndex(dest, index, sharedBlobsDir); err != nil {
+			return err
+		}
+	default:
+		return fmt.Errorf("unsupported mediaType in index: %q", descriptor.MediaType)
 	}
-	blobsUsedInImageRefIndex[descriptor.Digest]++ // Add the nested index in the list of blobs used by this reference
-
-	return blobsUsedInImageRefIndex, nil
+	return nil
 }
 
-// Updates a map of digest with the usage count, so a blob that is referenced three times will have 3 in the map
-func (ref ociReference) addBlobsUsedInIndex(destination map[digest.Digest]int, index *imgspecv1.Index, sharedBlobsDir string) error {
+// countBlobsReferencedByIndex updates dest with usage counts of blobs required for index, EXCLUDING the index itself.
+func (ref ociReference) countBlobsReferencedByIndex(destination map[digest.Digest]int, index *imgspecv1.Index, sharedBlobsDir string) error {
 	for _, descriptor := range index.Manifests {
-		destination[descriptor.Digest]++
-		switch descriptor.MediaType {
-		case imgspecv1.MediaTypeImageManifest:
-			manifest, err := ref.getManifest(&descriptor, sharedBlobsDir)
-			if err != nil {
-				return err
-			}
-			for digest, count := range ref.getBlobsUsedInManifest(manifest) {
-				destination[digest] += count
-			}
-		case imgspecv1.MediaTypeImageIndex:
-			blobPath, err := ref.blobPath(descriptor.Digest, sharedBlobsDir)
-			if err != nil {
-				return err
-			}
-			index, err := parseIndex(blobPath)
-			if err != nil {
-				return err
-			}
-			err = ref.addBlobsUsedInIndex(destination, index, sharedBlobsDir)
-			if err != nil {
-				return err
-			}
-		default:
-			return fmt.Errorf("unsupported mediaType in index: %q", descriptor.MediaType)
+		if err := ref.countBlobsForDescriptor(destination, &descriptor, sharedBlobsDir); err != nil {
+			return err
 		}
 	}
-
 	return nil
 }
 
-func (ref ociReference) getBlobsUsedInManifest(manifest *imgspecv1.Manifest) map[digest.Digest]int {
-	blobsUsedInManifest := make(map[digest.Digest]int, 0)
-
-	blobsUsedInManifest[manifest.Config.Digest]++
-	for _, layer := range manifest.Layers {
-		blobsUsedInManifest[layer.Digest]++
-	}
-
-	return blobsUsedInManifest
-}
-
 // This takes in a map of the digest and their usage count in the manifest to be deleted
 // It will compare it to the digest usage in the root index, and return a set of the blobs that can be safely deleted
 func (ref ociReference) getBlobsToDelete(blobsUsedByDescriptorToDelete map[digest.Digest]int, sharedBlobsDir string) (*set.Set[digest.Digest], error) {
@@ -138,7 +95,7 @@ func (ref ociReference) getBlobsToDelete(blobsUsedByDescriptorToDelete map[diges
 		return nil, err
 	}
 	blobsUsedInRootIndex := make(map[digest.Digest]int)
-	err = ref.addBlobsUsedInIndex(blobsUsedInRootIndex, rootIndex, sharedBlobsDir)
+	err = ref.countBlobsReferencedByIndex(blobsUsedInRootIndex, rootIndex, sharedBlobsDir)
 	if err != nil {
 		return nil, err
 	}
@@ -224,17 +181,3 @@ func saveJSON(path string, content any) error {
 
 	return json.NewEncoder(file).Encode(content)
 }
-
-func (ref ociReference) getManifest(descriptor *imgspecv1.Descriptor, sharedBlobsDir string) (*imgspecv1.Manifest, error) {
-	manifestPath, err := ref.blobPath(descriptor.Digest, sharedBlobsDir)
-	if err != nil {
-		return nil, err
-	}
-
-	manifest, err := parseJSON[imgspecv1.Manifest](manifestPath)
-	if err != nil {
-		return nil, err
-	}
-
-	return manifest, nil
-}
diff --git a/oci/layout/oci_delete_test.go b/oci/layout/oci_delete_test.go
@@ -7,6 +7,7 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"slices"
 	"testing"
 
 	"github.com/containers/image/v5/types"
@@ -92,6 +93,14 @@ func TestReferenceDeleteImage_sharedBlobDir(t *testing.T) {
 	require.Equal(t, 0, len(index.Manifests))
 }
 
+func assertRefNameIsMissing(t *testing.T, index *imgspecv1.Index, refName string) {
+	if slices.ContainsFunc(index.Manifests, func(desc imgspecv1.Descriptor) bool {
+		return desc.Annotations[imgspecv1.AnnotationRefName] == refName
+	}) {
+		assert.Failf(t, "index still contains refName %q after deletion", refName)
+	}
+}
+
 func TestReferenceDeleteImage_multipleImages(t *testing.T) {
 	tmpDir := loadFixture(t, "delete_image_multiple_images")
 
@@ -118,14 +127,7 @@ func TestReferenceDeleteImage_multipleImages(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, 6, len(index.Manifests))
 	// .. Check that the image is not in the index anymore
-	for _, descriptor := range index.Manifests {
-		switch descriptor.Annotations[imgspecv1.AnnotationRefName] {
-		case "3.17.5":
-			assert.Fail(t, "image still present in the index after deletion")
-		default:
-			continue
-		}
-	}
+	assertRefNameIsMissing(t, index, "3.17.5")
 }
 
 func TestReferenceDeleteImage_multipleImages_blobsUsedByOtherImages(t *testing.T) {
@@ -154,14 +156,7 @@ func TestReferenceDeleteImage_multipleImages_blobsUsedByOtherImages(t *testing.T
 	require.NoError(t, err)
 	require.Equal(t, 6, len(index.Manifests))
 	// .. Check that the image is not in the index anymore
-	for _, descriptor := range index.Manifests {
-		switch descriptor.Annotations[imgspecv1.AnnotationRefName] {
-		case "1.0.0":
-			assert.Fail(t, "image still present in the index after deletion")
-		default:
-			continue
-		}
-	}
+	assertRefNameIsMissing(t, index, "1.0.0")
 }
 
 func TestReferenceDeleteImage_multipleImages_imageDoesNotExist(t *testing.T) {
@@ -214,14 +209,7 @@ func TestReferenceDeleteImage_multipleImages_nestedIndexImage(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, 6, len(index.Manifests))
 	// .. Check that the image is not in the index anymore
-	for _, descriptor := range index.Manifests {
-		switch descriptor.Annotations[imgspecv1.AnnotationRefName] {
-		case "3.16.7":
-			assert.Fail(t, "image still present in the index after deletion")
-		default:
-			continue
-		}
-	}
+	assertRefNameIsMissing(t, index, "3.16.7")
 }
 
 func TestReferenceDeleteImage_multipleImages_nestedIndexImage_refWithSameContent(t *testing.T) {