Stop deleting cache images

acceptance/exporter_test.go

@@ -344,7 +344,9 @@ func testExporterFunc(platformAPI string) func(t *testing.T, when spec.G, it spe
 
 							// Retrieve the cache image from the ephemeral registry
 							h.Run(t, exec.Command("docker", "pull", cacheImageName))
-							subject, err := cache.NewImageCacheFromName(cacheImageName, authn.DefaultKeychain, cmd.DefaultLogger)
+							logger := cmd.DefaultLogger
+
+							subject, err := cache.NewImageCacheFromName(cacheImageName, authn.DefaultKeychain, logger, cache.NewImageDeleter(cache.NewImageComparer(), logger, api.MustParse(platformAPI).LessThan("0.13")))
 							h.AssertNil(t, err)
 
 							//Assert the cache image was created with an empty layer

analyzer.go

@@ -151,7 +151,7 @@ func (f *AnalyzerFactory) setBuildpacks(analyzer *Analyzer, group buildpack.Grou
 
 func (f *AnalyzerFactory) setCache(analyzer *Analyzer, imageRef string, dir string) error {
 	var err error
-	analyzer.Cache, err = f.cacheHandler.InitCache(imageRef, dir)
+	analyzer.Cache, err = f.cacheHandler.InitCache(imageRef, dir, f.platformAPI.LessThan("0.13"))
 	return err
 }
 

analyzer_test.go

@@ -367,7 +367,7 @@ func testAnalyzerFactory(t *testing.T, when spec.G, it spec.S) {
 				fakeAPIVerifier.EXPECT().VerifyBuildpackAPI(buildpack.KindBuildpack, "some-buildpack-id@some-buildpack-version", "0.2", logger)
 
 				t.Log("processes cache")
-				fakeCacheHandler.EXPECT().InitCache("some-cache-image-ref", "some-legacy-cache-dir")
+				fakeCacheHandler.EXPECT().InitCache("some-cache-image-ref", "some-legacy-cache-dir", true)
 
 				t.Log("processes previous image")
 				fakeImageHandler.EXPECT().InitImage("some-previous-image-ref").Return(previousImage, nil)
@@ -421,7 +421,7 @@ func testAnalyzerFactory(t *testing.T, when spec.G, it spec.S) {
 					fakeAPIVerifier.EXPECT().VerifyBuildpackAPI(buildpack.KindBuildpack, "some-buildpack-id@some-buildpack-version", "0.2", logger)
 
 					t.Log("processes cache")
-					fakeCacheHandler.EXPECT().InitCache("some-cache-image-ref", "some-legacy-cache-dir")
+					fakeCacheHandler.EXPECT().InitCache("some-cache-image-ref", "some-legacy-cache-dir", true)
 
 					t.Log("processes previous image")
 					fakeImageHandler.EXPECT().InitImage("some-previous-image-ref").Return(previousImage, nil)
@@ -454,7 +454,7 @@ func testAnalyzerFactory(t *testing.T, when spec.G, it spec.S) {
 
 			when("buildpack group is provided", func() {
 				it("uses the provided group", func() {
-					fakeCacheHandler.EXPECT().InitCache(gomock.Any(), gomock.Any())
+					fakeCacheHandler.EXPECT().InitCache(gomock.Any(), gomock.Any(), gomock.Any())
 					fakeImageHandler.EXPECT().InitImage(gomock.Any())
 					fakeImageHandler.EXPECT().Kind().Return(image.RemoteKind)
 					fakeImageHandler.EXPECT().InitImage(gomock.Any())

cache/image_cache.go

@@ -19,21 +19,25 @@ import (
 const MetadataLabel = "io.buildpacks.lifecycle.cache.metadata"
 
 type ImageCache struct {
-	committed bool
-	origImage imgutil.Image
-	newImage  imgutil.Image
-	logger    log.Logger
+	committed    bool
+	origImage    imgutil.Image
+	newImage     imgutil.Image
+	logger       log.Logger
+	imageDeleter ImageDeleter
 }
 
-func NewImageCache(origImage imgutil.Image, newImage imgutil.Image, logger log.Logger) *ImageCache {
+// NewImageCache creates a new ImageCache instance
+func NewImageCache(origImage imgutil.Image, newImage imgutil.Image, logger log.Logger, imageDeleter ImageDeleter) *ImageCache {
 	return &ImageCache{
-		origImage: origImage,
-		newImage:  newImage,
-		logger:    logger,
+		origImage:    origImage,
+		newImage:     newImage,
+		logger:       logger,
+		imageDeleter: imageDeleter,
 	}
 }
 
-func NewImageCacheFromName(name string, keychain authn.Keychain, logger log.Logger) (*ImageCache, error) {
+// NewImageCacheFromName creates a new ImageCache from the name that has been provided
+func NewImageCacheFromName(name string, keychain authn.Keychain, logger log.Logger, imageDeleter ImageDeleter) (*ImageCache, error) {
 	origImage, err := remote.NewImage(
 		name,
 		keychain,
@@ -54,7 +58,7 @@ func NewImageCacheFromName(name string, keychain authn.Keychain, logger log.Logg
 		return nil, fmt.Errorf("creating new cache image %q: %v", name, err)
 	}
 
-	return NewImageCache(origImage, emptyImage, logger), nil
+	return NewImageCache(origImage, emptyImage, logger, imageDeleter), nil
 }
 
 func (c *ImageCache) Exists() bool {
@@ -111,36 +115,17 @@ func (c *ImageCache) Commit() error {
 		return errCacheCommitted
 	}
 
-	// Check if the cache image exists prior to saving the new cache at that same location
-	origImgExists := c.origImage.Found()
-
 	if err := c.newImage.Save(); err != nil {
 		return errors.Wrapf(err, "saving image '%s'", c.newImage.Name())
 	}
 	c.committed = true
 
-	if origImgExists {
-		// Deleting the original image is for cleanup only and should not fail the commit.
-		if err := c.DeleteOrigImage(); err != nil {
-			c.logger.Warnf("Unable to delete previous cache image: %v", err.Error())
-		}
+	// Check if the cache image exists prior to saving the new cache at that same location
+	if c.origImage.Found() {
+		c.imageDeleter.DeleteOrigImageIfDifferentFromNewImage(c.origImage, c.newImage)
 	}
+
 	c.origImage = c.newImage
 
 	return nil
 }
-
-func (c *ImageCache) DeleteOrigImage() error {
-	origIdentifier, err := c.origImage.Identifier()
-	if err != nil {
-		return errors.Wrap(err, "getting identifier for original image")
-	}
-	newIdentifier, err := c.newImage.Identifier()
-	if err != nil {
-		return errors.Wrap(err, "getting identifier for new image")
-	}
-	if origIdentifier.String() == newIdentifier.String() {
-		return nil
-	}
-	return c.origImage.Delete()
-}

cache/image_cache_test.go

@@ -7,10 +7,12 @@ import (
 	"path/filepath"
 	"testing"
 
-	"github.com/buildpacks/imgutil"
+	cacheMock "github.com/buildpacks/lifecycle/testmock/cache"
+
+	"github.com/golang/mock/gomock"
+
 	"github.com/buildpacks/imgutil/fakes"
 	"github.com/buildpacks/imgutil/local"
-	"github.com/pkg/errors"
 	"github.com/sclevine/spec"
 	"github.com/sclevine/spec/report"
 
@@ -45,8 +47,13 @@ func testImageCache(t *testing.T, when spec.G, it spec.S) {
 
 		fakeOriginalImage = fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeOriginalImage"})
 		fakeNewImage = fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeImage"})
+		mockController := gomock.NewController(t)
+		fakeImageDeleter := cacheMock.NewMockImageDeleter(mockController)
+		fakeImageComparer := cacheMock.NewMockImageComparer(mockController)
+		fakeImageComparer.EXPECT().ImagesEq(gomock.Any(), gomock.Any()).AnyTimes().Return(false, nil)
+		fakeImageDeleter.EXPECT().DeleteOrigImageIfDifferentFromNewImage(gomock.Any(), gomock.Any()).AnyTimes()
 		testLogger = cmd.DefaultLogger
-		subject = cache.NewImageCache(fakeOriginalImage, fakeNewImage, testLogger)
+		subject = cache.NewImageCache(fakeOriginalImage, fakeNewImage, testLogger, fakeImageDeleter)
 
 		testLayerTarPath = filepath.Join(tmpDir, "some-layer.tar")
 		h.AssertNil(t, os.WriteFile(testLayerTarPath, []byte("dummy data"), 0600))
@@ -291,74 +298,5 @@ func testImageCache(t *testing.T, when spec.G, it spec.S) {
 				h.AssertError(t, err, "cache cannot be modified after commit")
 			})
 		})
-
-		when("attempting to delete original image gives non nil error", func() {
-			var (
-				fakeErrorImage *fakeErrorImage
-				mockLogger     *MockLogger
-			)
-
-			it.Before(func() {
-				fakeOriginalImage = fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeOrigImage"})
-				fakeErrorImage = newFakeImageErrIdentifier(fakeOriginalImage)
-				fakeNewImage = fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeNewImage"})
-				mockLogger = &MockLogger{Logger: testLogger}
-				subject = cache.NewImageCache(fakeErrorImage, fakeNewImage, mockLogger)
-			})
-			it("should log error as warning", func() {
-				err := subject.Commit()
-				h.AssertNil(t, err)
-				h.AssertEq(t, mockLogger.Calls, 1)
-			})
-		})
-
-		when("with #DeleteOrigImage", func() {
-			when("original and new image are different", func() {
-				it.Before(func() {
-					fakeOriginalImage = fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeOrigImage"})
-					fakeNewImage = fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeNewImage"})
-					subject = cache.NewImageCache(fakeOriginalImage, fakeNewImage, testLogger)
-				})
-				it("should delete original image", func() {
-					err := subject.DeleteOrigImage()
-					h.AssertNil(t, err)
-					h.AssertEq(t, fakeOriginalImage.Found(), false)
-				})
-			})
-
-			when("original and new image are the same", func() {
-				it.Before(func() {
-					fakeOriginalImage = fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeOrigImage"})
-					fakeNewImage = fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeOrigImage"})
-					subject = cache.NewImageCache(fakeOriginalImage, fakeNewImage, testLogger)
-				})
-				it("should not delete original image", func() {
-					err := subject.DeleteOrigImage()
-					h.AssertNil(t, err)
-					h.AssertEq(t, fakeOriginalImage.Found(), true)
-				})
-			})
-		})
 	})
 }
-
-type fakeErrorImage struct {
-	imgutil.Image
-}
-
-func newFakeImageErrIdentifier(origImage imgutil.Image) *fakeErrorImage {
-	return &fakeErrorImage{Image: origImage}
-}
-
-func (f *fakeErrorImage) Identifier() (imgutil.Identifier, error) {
-	return nil, errors.New("error deleting original image")
-}
-
-type MockLogger struct {
-	log.Logger
-	Calls int
-}
-
-func (l *MockLogger) Warnf(fmt string, v ...interface{}) {
-	l.Calls++
-}

cache/image_comparer.go

@@ -0,0 +1,40 @@
+package cache
+
+import (
+	"github.com/buildpacks/imgutil"
+	"github.com/pkg/errors"
+)
+
+//go:generate mockgen -package testmockcache -destination ../testmock/cache/image_comparer.go github.com/buildpacks/lifecycle/cache ImageComparer
+
+// ImageComparer provides a way to compare images
+type ImageComparer interface {
+	ImagesEq(orig imgutil.Image, new imgutil.Image) (bool, error)
+}
+
+// ImageComparerImpl implements the ImageComparer interface
+type ImageComparerImpl struct{}
+
+// NewImageComparer instantiate ImageComparerImpl
+func NewImageComparer() *ImageComparerImpl {
+	return &ImageComparerImpl{}
+}
+
+// ImagesEq checks if the origin and the new images are the same
+func (c *ImageComparerImpl) ImagesEq(origImage imgutil.Image, newImage imgutil.Image) (bool, error) {
+	origIdentifier, err := origImage.Identifier()
+	if err != nil {
+		return false, errors.Wrap(err, "getting identifier for original image")
+	}
+
+	newIdentifier, err := newImage.Identifier()
+	if err != nil {
+		return false, errors.Wrap(err, "getting identifier for new image")
+	}
+
+	if origIdentifier.String() == newIdentifier.String() {
+		return true, nil
+	}
+
+	return false, nil
+}

cache/image_comparer_test.go

@@ -0,0 +1,57 @@
+package cache
+
+import (
+	"testing"
+
+	"github.com/buildpacks/imgutil/fakes"
+	"github.com/buildpacks/imgutil/local"
+	"github.com/sclevine/spec"
+	"github.com/sclevine/spec/report"
+
+	h "github.com/buildpacks/lifecycle/testhelpers"
+)
+
+func TestImageComparer(t *testing.T) {
+	spec.Run(t, "ImageComparer", testImageComparer, spec.Parallel(), spec.Report(report.Terminal{}))
+}
+
+func testImageComparer(t *testing.T, when spec.G, it spec.S) {
+	var (
+		imageComparer ImageComparer
+	)
+
+	it.Before(func() {
+		imageComparer = NewImageComparer()
+	})
+
+	when("Comparing two images: orig and new", func() {
+		it("checks if the images have the same identifier", func() {
+			fakeOldImage := fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeOldImage"})
+			fakeNewImage := fakes.NewImage("fake-new-image", "", local.IDIdentifier{ImageID: "fakeNewImage"})
+
+			result, _ := imageComparer.ImagesEq(fakeOldImage, fakeNewImage)
+
+			h.AssertEq(t, result, false)
+		})
+
+		it("returns an error if it's impossible to get the original image identifier", func() {
+			fakeOriginalImage := fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeOriginalImage"})
+			fakeNewImage := fakes.NewImage("fake-new-image", "", local.IDIdentifier{ImageID: "fakeNewImage"})
+			fakeErrorImage := newFakeImageErrIdentifier(fakeOriginalImage, "original")
+
+			_, err := imageComparer.ImagesEq(fakeErrorImage, fakeNewImage)
+
+			h.AssertError(t, err, "getting identifier for original image")
+		})
+
+		it("returns an error if it's impossible to get the new image identifier", func() {
+			fakeOriginalImage := fakes.NewImage("fake-image", "", local.IDIdentifier{ImageID: "fakeOriginalImage"})
+			fakeNewImage := fakes.NewImage("fake-new-image", "", local.IDIdentifier{ImageID: "fakeNewImage"})
+			fakeErrorImage := newFakeImageErrIdentifier(fakeNewImage, "new")
+
+			_, err := imageComparer.ImagesEq(fakeOriginalImage, fakeErrorImage)
+
+			h.AssertError(t, err, "getting identifier for new image")
+		})
+	})
+}

cache/image_deleter.go

@@ -0,0 +1,50 @@
+// Package cache provides functionalities around the cache
+package cache
+
+import (
+	"github.com/buildpacks/imgutil"
+
+	"github.com/buildpacks/lifecycle/log"
+)
+
+//go:generate mockgen -package testmockcache -destination ../testmock/cache/image_deleter.go github.com/buildpacks/lifecycle/cache ImageDeleter
+
+// ImageDeleter defines the methods available to delete and compare cached images
+type ImageDeleter interface {
+	DeleteOrigImageIfDifferentFromNewImage(origImage, newImage imgutil.Image)
+}
+
+// ImageDeleterImpl is a component to manage cache image deletion
+type ImageDeleterImpl struct {
+	logger          log.Logger
+	deletionEnabled bool
+	comparer        ImageComparer
+}
+
+// NewImageDeleter creates a new ImageDeleter implementation
+func NewImageDeleter(comparer ImageComparer, logger log.Logger, deletionEnabled bool) *ImageDeleterImpl {
+	return &ImageDeleterImpl{comparer: comparer, logger: logger, deletionEnabled: deletionEnabled}
+}
+
+// DeleteOrigImageIfDifferentFromNewImage compares the two images, and it tries to delete it if they are not the same
+func (c *ImageDeleterImpl) DeleteOrigImageIfDifferentFromNewImage(origImage, newImage imgutil.Image) {
+	if c.deletionEnabled {
+		same, err := c.comparer.ImagesEq(origImage, newImage)
+		if err != nil {
+			c.logger.Warnf("Unable to compare the image: %v", err.Error())
+		}
+
+		if !same {
+			go c.deleteImage(origImage)
+		}
+	}
+}
+
+// deleteImage deletes an image
+func (c *ImageDeleterImpl) deleteImage(image imgutil.Image) {
+	if c.deletionEnabled {
+		if err := image.Delete(); err != nil {
+			c.logger.Warnf("Unable to delete cache image: %v", err.Error())
+		}
+	}
+}