merge branch 'pr-198'

  oci/cas/dir: Only Clean .umoci-* directories

LGTMs: @cyphar
Closes #198

CHANGELOG.md

@@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   responsibility of the caller (which was quite difficult if you were
   unprivileged). This is a breaking change, but is in the error path so it's
   not critical. openSUSE/umoci#174 openSUSE/umoci#187
+- `umoci gc` now will no longer remove unknown files and directories that
+  aren't `flock(2)`ed, thus ensuring that any possible OCI image-spec
+  extensions or other users of an image being operated on will no longer
+  break.  openSUSE/umoci#198
 
 ### Added
 - `umoci repack` now supports `--refresh-bundle` which will update the

oci/cas/dir/dir.go

@@ -24,6 +24,7 @@ import (
 	"os"
 	"path/filepath"
 
+	"github.com/apex/log"
 	"github.com/openSUSE/umoci/oci/cas"
 	"github.com/opencontainers/go-digest"
 	imeta "github.com/opencontainers/image-spec/specs-go"
@@ -77,7 +78,7 @@ type dirEngine struct {
 
 func (e *dirEngine) ensureTempDir() error {
 	if e.temp == "" {
-		tempDir, err := ioutil.TempDir(e.path, "tmp-")
+		tempDir, err := ioutil.TempDir(e.path, ".umoci-")
 		if err != nil {
 			return errors.Wrap(err, "create tempdir")
 		}
@@ -297,46 +298,42 @@ func (e *dirEngine) ListBlobs(ctx context.Context) ([]digest.Digest, error) {
 // (this includes temporary files and directories not reachable from the CAS
 // interface). This MUST NOT remove any blobs or references in the store.
 func (e *dirEngine) Clean(ctx context.Context) error {
-	// Effectively we are going to remove every directory except the standard
-	// directories, unless they have a lock already.
-	fh, err := os.Open(e.path)
+	// Remove every .umoci directory that isn't flocked.
+	matches, err := filepath.Glob(filepath.Join(e.path, ".umoci-*"))
 	if err != nil {
-		return errors.Wrap(err, "open imagedir")
+		return errors.Wrap(err, "glob .umoci-*")
 	}
-	defer fh.Close()
-
-	children, err := fh.Readdir(-1)
-	if err != nil {
-		return errors.Wrap(err, "readdir imagedir")
+	for _, path := range matches {
+		err = e.cleanPath(ctx, path)
+		if err != nil && err != filepath.SkipDir {
+			return err
+		}
 	}
 
-	for _, child := range children {
-		// Skip any children that are expected to exist.
-		switch child.Name() {
-		case blobDirectory, indexFile, layoutFile:
-			continue
-		}
+	return nil
+}
 
-		// Try to get a lock on the directory.
-		path := filepath.Join(e.path, child.Name())
-		cfh, err := os.Open(path)
-		if err != nil {
-			// Ignore errors because it might've been deleted underneath us.
-			continue
-		}
-		defer cfh.Close()
+func (e *dirEngine) cleanPath(ctx context.Context, path string) error {
+	cfh, err := os.Open(path)
+	if err != nil && !os.IsNotExist(err) {
+		return errors.Wrap(err, "open for locking")
+	}
+	defer cfh.Close()
 
-		if err := unix.Flock(int(cfh.Fd()), unix.LOCK_EX|unix.LOCK_NB); err != nil {
-			// If we fail to get a flock(2) then it's probably already locked,
-			// so we shouldn't touch it.
-			continue
-		}
-		defer unix.Flock(int(cfh.Fd()), unix.LOCK_UN)
+	if err := unix.Flock(int(cfh.Fd()), unix.LOCK_EX|unix.LOCK_NB); err != nil {
+		// If we fail to get a flock(2) then it's probably already locked,
+		// so we shouldn't touch it.
+		return filepath.SkipDir
+	}
+	defer unix.Flock(int(cfh.Fd()), unix.LOCK_UN)
 
-		if err := os.RemoveAll(path); err != nil {
-			return errors.Wrap(err, "remove garbage path")
-		}
+	if err := os.RemoveAll(path); os.IsNotExist(err) {
+		return nil // somebody else beat us to it
+	} else if err != nil {
+		log.Warnf("failed to remove %s: %v", path, err)
+		return filepath.SkipDir
 	}
+	log.Debugf("cleaned %s", path)
 
 	return nil
 }

oci/cas/dir/dir_test.go

@@ -236,8 +236,13 @@ func TestEngineGCLocking(t *testing.T) {
 		t.Errorf("engine doesn't have a tempdir after putting a blob!")
 	}
 
-	// Create tempdir to make sure things work.
-	removedDir, err := ioutil.TempDir(image, "testdir")
+	// Create umoci and other directories and files to make sure things work.
+	umociTestDir, err := ioutil.TempDir(image, ".umoci-dead-")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	otherTestDir, err := ioutil.TempDir(image, "other-")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -253,15 +258,22 @@ func TestEngineGCLocking(t *testing.T) {
 		t.Fatalf("unexpected error while GCing image: %+v", err)
 	}
 
-	// Make sure that engine.temp is still around.
-	if _, err := os.Lstat(engine.(*dirEngine).temp); err != nil {
-		t.Errorf("expected active direngine.temp to still exist after GC: %+v", err)
+	for _, path := range []string{
+		engine.(*dirEngine).temp,
+		otherTestDir,
+	} {
+		if _, err := os.Lstat(path); err != nil {
+			t.Errorf("expected %s to still exist after GC: %+v", path, err)
+		}
 	}
 
-	// Make sure that removedDir is still around
-	if _, err := os.Lstat(removedDir); err == nil {
-		t.Errorf("expected inactive temporary dir to not exist after GC")
-	} else if !os.IsNotExist(errors.Cause(err)) {
-		t.Errorf("expected IsNotExist for temporary dir after GC: %+v", err)
+	for _, path := range []string{
+		umociTestDir,
+	} {
+		if _, err := os.Lstat(path); err == nil {
+			t.Errorf("expected %s to not exist after GC", path)
+		} else if !os.IsNotExist(errors.Cause(err)) {
+			t.Errorf("expected IsNotExist for %s after GC: %+v", path, err)
+		}
 	}
 }