diff --git a/groot/deleter.go b/groot/deleter.go index d1a9fe52c..abbdf0346 100644 --- a/groot/deleter.go +++ b/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 } diff --git a/groot/deleter_test.go b/groot/deleter_test.go index ffaf3da50..a2e3f8dd7 100644 --- a/groot/deleter_test.go +++ b/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()) + }) + }) + }) }) }) diff --git a/integration/delete_test.go b/integration/delete_test.go index b03de7bb9..1b68c88e3 100644 --- a/integration/delete_test.go +++ b/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() diff --git a/integration/metrics_test.go b/integration/metrics_test.go index b7d431ba0..de6b654e9 100644 --- a/integration/metrics_test.go +++ b/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() { diff --git a/store/image_cloner/image_cloner.go b/store/image_cloner/image_cloner.go index e79de0ee9..187759aa9 100644 --- a/store/image_cloner/image_cloner.go +++ b/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") } diff --git a/store/image_cloner/image_cloner_linux_test.go b/store/image_cloner/image_cloner_linux_test.go index bee454a50..22fd0e16a 100644 --- a/store/image_cloner/image_cloner_linux_test.go +++ b/store/image_cloner/image_cloner_linux_test.go @@ -378,7 +378,7 @@ 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()) @@ -386,14 +386,39 @@ var _ = Describe("Image", func() { 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"))) + }) }) }) })