Change destroy so it doesn't corrupt state

* Destroy will no longer try to delete the image metadata if deleting
  the image path fails.
* Destroy will always try to delete the image path, even if the
  volume driver fails to destroy the image.

[finishes #144345725]

Signed-off-by: Tiago Scolari <[email protected]>

groot/deleter.go

@@ -2,8 +2,11 @@ package groot
 
 import (
 	"fmt"
+	"os"
 	"time"
 
+	"github.com/pkg/errors"
+
 	"code.cloudfoundry.org/lager"
 )
 
@@ -28,12 +31,17 @@ func (d *Deleter) Delete(logger lager.Logger, id string) error {
 	logger.Info("starting")
 	defer logger.Info("ending")
 
-	err := d.imageCloner.Destroy(logger, id)
+	if err := d.imageCloner.Destroy(logger, id); err != nil {
+		return err
+	}
 
 	imageRefName := fmt.Sprintf(ImageReferenceFormat, id)
-	if derErr := d.dependencyManager.Deregister(imageRefName); derErr != nil {
-		logger.Error("failed-to-deregister-dependencies", derErr)
+	if err := d.dependencyManager.Deregister(imageRefName); err != nil {
+		if !os.IsNotExist(errors.Cause(err)) {
+			logger.Error("failed-to-deregister-dependencies", err)
+			return err
+		}
 	}
 
-	return err
+	return nil
 }

groot/deleter_test.go

@@ -2,6 +2,7 @@ package groot_test
 
 import (
 	"errors"
+	"os"
 
 	"code.cloudfoundry.org/grootfs/groot"
 	"code.cloudfoundry.org/grootfs/groot/grootfakes"
@@ -51,6 +52,11 @@ var _ = Describe("Deleter", func() {
 			It("returns an error", func() {
 				Expect(deleter.Delete(logger, "some-id")).To(MatchError(ContainSubstring("failed to destroy image")))
 			})
+
+			It("doesn't deregister the image", func() {
+				Expect(deleter.Delete(logger, "some-id")).To(HaveOccurred())
+				Expect(fakeDependencyManager.DeregisterCallCount()).To(Equal(0))
+			})
 		})
 
 		It("emits metrics for deletion", func() {
@@ -61,5 +67,25 @@ var _ = Describe("Deleter", func() {
 			Expect(name).To(Equal(groot.MetricImageDeletionTime))
 			Expect(start).NotTo(BeZero())
 		})
+
+		Context("when it fails to deregister an image", func() {
+			BeforeEach(func() {
+				fakeDependencyManager.DeregisterReturns(errors.New("failed"))
+			})
+
+			It("returns an error", func() {
+				Expect(deleter.Delete(logger, "some-id")).To(MatchError(ContainSubstring("failed")))
+			})
+
+			Context("when the image metadata doesn't exist", func() {
+				BeforeEach(func() {
+					fakeDependencyManager.DeregisterReturns(os.ErrNotExist)
+				})
+
+				It("doesn't return an error", func() {
+					Expect(deleter.Delete(logger, "some-id")).To(Succeed())
+				})
+			})
+		})
 	})
 })

integration/delete_test.go

@@ -4,9 +4,12 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"path/filepath"
+	"syscall"
 
 	"code.cloudfoundry.org/grootfs/groot"
 	"code.cloudfoundry.org/grootfs/integration"
+	"code.cloudfoundry.org/grootfs/store"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -101,6 +104,27 @@ var _ = Describe("Delete", func() {
 		})
 	})
 
+	Context("when it fails to delete the image", func() {
+		var mntPoint string
+
+		JustBeforeEach(func() {
+			mntPoint = filepath.Join(image.Path, "mnt")
+			Expect(os.Mkdir(mntPoint, 0700)).To(Succeed())
+			Expect(syscall.Mount(os.TempDir(), mntPoint, "none", syscall.MS_BIND, "")).To(Succeed())
+		})
+
+		AfterEach(func() {
+			Expect(syscall.Unmount(mntPoint, syscall.MNT_DETACH)).To(Succeed())
+		})
+
+		It("doesn't remove the metadata file", func() {
+			metadataPath := filepath.Join(StorePath, store.MetaDirName, "dependencies", "image:random-id.json")
+			Expect(metadataPath).To(BeAnExistingFile())
+			Expect(Runner.Delete(image.Path)).To(MatchError(ContainSubstring("deleting image path")))
+			Expect(metadataPath).To(BeAnExistingFile())
+		})
+	})
+
 	Context("when the id is not provided", func() {
 		It("fails", func() {
 			outBuffer := gbytes.NewBuffer()

integration/metrics_test.go

@@ -234,7 +234,7 @@ var _ = Describe("Metrics", func() {
 				}).Should(HaveLen(1))
 
 				Expect(*errors[0].Source).To(Equal("grootfs-error.delete"))
-				Expect(*errors[0].Message).To(ContainSubstring("destroying image"))
+				Expect(*errors[0].Message).To(ContainSubstring("deleting image"))
 			})
 
 			It("emits the fail count", func() {

store/image_cloner/image_cloner.go

@@ -138,11 +138,13 @@ func (b *ImageCloner) Destroy(logger lager.Logger, id string) error {
 	}
 
 	imagePath := b.imagePath(id)
-	if err := b.imageDriver.DestroyImage(logger, imagePath); err != nil {
-		return errorspkg.Wrap(err, "destroying image")
+	volDriverErr := b.imageDriver.DestroyImage(logger, imagePath)
+	if volDriverErr != nil {
+		logger.Error("destroying-image-failed", volDriverErr)
 	}
 
 	if err := b.deleteImageDir(imagePath); err != nil {
+		logger.Error("deleting-image-dir-failed", err, lager.Data{"volumeDriverError": volDriverErr})
 		return errorspkg.Wrap(err, "deleting image path")
 	}
 

store/image_cloner/image_cloner_linux_test.go

@@ -378,22 +378,47 @@ var _ = Describe("Image", func() {
 			})
 		})
 
-		It("removes the image", func() {
+		It("calls the image driver to remove the image", func() {
 			err := imageCloner.Destroy(logger, "some-id")
 			Expect(err).NotTo(HaveOccurred())
 
 			_, path := fakeImageDriver.DestroyImageArgsForCall(0)
 			Expect(path).To(Equal(imagePath))
 		})
 
-		Context("when removing the image fails", func() {
+		Context("when the image driver fails", func() {
 			BeforeEach(func() {
-				fakeImageDriver.DestroyImageReturns(errors.New("failed to remove image"))
+				fakeImageDriver.DestroyImageReturns(errors.New("failed"))
 			})
 
-			It("returns an error", func() {
+			It("doesnt fail", func() {
+				err := imageCloner.Destroy(logger, "some-id")
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("still tries to delete the image path", func() {
 				err := imageCloner.Destroy(logger, "some-id")
-				Expect(err).To(MatchError(ContainSubstring("failed to remove image")))
+				Expect(err).NotTo(HaveOccurred())
+				Expect(imagePath).NotTo(BeAnExistingFile())
+			})
+
+			Context("when removing the image path also fails", func() {
+				var mntPoint string
+
+				JustBeforeEach(func() {
+					mntPoint = filepath.Join(imagePath, "mnt")
+					Expect(os.Mkdir(mntPoint, 0700)).To(Succeed())
+					Expect(syscall.Mount(mntPoint, mntPoint, "none", syscall.MS_BIND, "")).To(Succeed())
+				})
+
+				AfterEach(func() {
+					Expect(syscall.Unmount(mntPoint, syscall.MNT_DETACH)).To(Succeed())
+				})
+
+				It("returns an error", func() {
+					err := imageCloner.Destroy(logger, "some-id")
+					Expect(err).To(MatchError(ContainSubstring("deleting image path")))
+				})
 			})
 		})
 	})