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

This allows the use of other non-flocking consumers (e.g. other CAS
implementations) in the same base directory.  The '.umoci-*' approach
was suggested by Aleksa [1] as a way to keep Clean from stepping on
other consumers' toes.  I'd suggesting using pruneExpire for
ModTime-based backstopping, but Aleksa was concerned about leaking
temporary directories from crashed umoci runs, primarily for
disk-space concerns [1].  The .umoci-* approach allows such abandoned
cruft to be immediately cleaned, without worrying about clobbering
non-umoci consumers and extentions (unless they use .umoci-*
filenames, in which case they deserve it ;).

I've gone with .umoci-* instead of Aleksa's .umoci-tmpdir, because I
think the . prefix is a sufficient hint that these directories are
umoci-specific scratch space.

Removal errors (because of insufficient permissions, etc.) seemed like
they should be non-fatal so we could continue to remove other cruft.
However, I didn't want to silently ignore the failed removal.  In this
commit, I log those errors with a "warning" level.

The new cleanFile method gives us tighter scoping for the defer lock
release / file close, because those fire on function-exit [2].  With
the old code, we'd keep the removed directory file-descriptors and
associated flocks while Clean processed further directories.  With the
new code, we release those resources immediately after we finish
processing the directory in question.

[1]: https://github.com/openSUSE/umoci/pull/198#issuecomment-337889528
[2]: https://golang.org/ref/spec#Defer_statements

Signed-off-by: W. Trevor King <wking@tremily.us>

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 (%s)", 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)
+		}
 	}
 }