From c1616af2772881da9de0a421c0dbb9e6301dd5ef Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Wed, 18 Oct 2017 10:47:03 -0700 Subject: [PATCH] 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 ;). Aleksa also mentioned that non-umoci consumers might currently be relying on umoci to clean up their abandoned cruft [1]. But we both agree that non-umoci consumers should be cleaning up after themselves and/or providing their own "cleanup anything abandoned by previous runs of my tool" functionality, and should not be relying on umoci to do that for them [2,3]. 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 [4]. 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://github.com/openSUSE/umoci/pull/198#issuecomment-337928272 [3]: https://github.com/openSUSE/umoci/pull/198#issuecomment-337937858 [4]: https://golang.org/ref/spec#Defer_statements Signed-off-by: W. Trevor King --- CHANGELOG.md | 4 +++ oci/cas/dir/dir.go | 63 ++++++++++++++++++++--------------------- oci/cas/dir/dir_test.go | 32 ++++++++++++++------- 3 files changed, 56 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49ec2bc35..749dd1d21 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/oci/cas/dir/dir.go b/oci/cas/dir/dir.go index 6906852ea..8b83e4e57 100644 --- a/oci/cas/dir/dir.go +++ b/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 } diff --git a/oci/cas/dir/dir_test.go b/oci/cas/dir/dir_test.go index f44695406..29b143137 100644 --- a/oci/cas/dir/dir_test.go +++ b/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) + } } }