From 72b09f23b44d31067925c12aad8e44c9b487cde5 Mon Sep 17 00:00:00 2001 From: David Trudgian Date: Fri, 6 Mar 2020 12:33:03 -0600 Subject: [PATCH] Handle existing old-style cache entry directories (#5098) Fixes: #5097 Also moves defer statements after error checks when getting a cacheEntry to avoid panics on tmpCleanup with a nil cacheEntry. --- e2e/cache/cache.go | 4 +- e2e/cache/regressions.go | 73 +++++++++++++++++++++++++++++ internal/pkg/cache/cache.go | 32 +++++++++---- internal/pkg/cache/entry.go | 3 +- internal/pkg/client/library/pull.go | 2 +- internal/pkg/client/net/pull.go | 2 +- internal/pkg/client/oci/pull.go | 2 +- internal/pkg/client/oras/pull.go | 2 +- internal/pkg/client/shub/pull.go | 2 +- 9 files changed, 105 insertions(+), 17 deletions(-) create mode 100644 e2e/cache/regressions.go diff --git a/e2e/cache/cache.go b/e2e/cache/cache.go index a3b65a52f3..4d16a428bc 100644 --- a/e2e/cache/cache.go +++ b/e2e/cache/cache.go @@ -10,12 +10,11 @@ import ( "path/filepath" "testing" - "github.com/sylabs/singularity/internal/pkg/util/fs" - "github.com/sylabs/scs-library-client/client" "github.com/sylabs/singularity/e2e/internal/e2e" "github.com/sylabs/singularity/e2e/internal/testhelper" "github.com/sylabs/singularity/internal/pkg/cache" + "github.com/sylabs/singularity/internal/pkg/util/fs" ) type cacheTests struct { @@ -282,5 +281,6 @@ func E2ETests(env e2e.TestEnv) testhelper.Tests { return testhelper.Tests{ "interactive commands": np(c.testInteractiveCacheCmds), "non-interactive commands": np(c.testNoninteractiveCacheCmds), + "issue5097": np(c.issue5097), } } diff --git a/e2e/cache/regressions.go b/e2e/cache/regressions.go new file mode 100644 index 0000000000..ee41d9e779 --- /dev/null +++ b/e2e/cache/regressions.go @@ -0,0 +1,73 @@ +// Copyright (c) 2020, Sylabs Inc. All rights reserved. +// This software is licensed under a 3-clause BSD license. Please consult the +// LICENSE.md file distributed with the sources of this project regarding your +// rights to use or distribute this software. + +package cache + +import ( + "os" + "path" + "path/filepath" + "testing" + + "github.com/sylabs/scs-library-client/client" + "github.com/sylabs/singularity/e2e/internal/e2e" + "github.com/sylabs/singularity/internal/pkg/util/fs" +) + +// issue5097 - need to handle an existing directory entry present in the cache +// from older singularity versions. +func (c cacheTests) issue5097(t *testing.T) { + imgCacheDir, cleanCache := e2e.MakeCacheDir(t, c.env.TestDir) + defer cleanCache(t) + c.env.ImgCacheDir = imgCacheDir + + tempDir, imgStoreCleanup := e2e.MakeTempDir(t, "", "", "image store") + defer imgStoreCleanup(t) + imagePath := filepath.Join(tempDir, imgName) + + // Pull through the cache - will give us a new style file in the cache + c.env.RunSingularity( + t, + e2e.WithProfile(e2e.UserProfile), + e2e.WithCommand("pull"), + e2e.WithArgs([]string{"--force", imagePath, imgURL}...), + e2e.ExpectExit(0), + ) + + // Replace the cache entry with a directory, containing the image, + // like in older versions of singularity + hash, err := client.ImageHash(imagePath) + if err != nil { + t.Fatalf("Could not calculate hash of test image: %v", err) + } + cachePath := path.Join(imgCacheDir, "cache", "library", hash) + err = os.Remove(cachePath) + if err != nil { + t.Fatalf("Could not remove cached image '%s': %v", cachePath, err) + } + err = os.Mkdir(cachePath, 0700) + if err != nil { + t.Fatalf("Could not create directory '%s': %v", cachePath, err) + } + err = fs.CopyFile(imagePath, path.Join(cachePath, hash), 0700) + if err != nil { + t.Fatalf("Could not copy file to directory '%s': %v", cachePath, err) + } + + // Pull through the cache - it should work as we now remove the directory and + // re-pull a file into the cache + c.env.RunSingularity( + t, + e2e.WithProfile(e2e.UserProfile), + e2e.WithCommand("pull"), + e2e.WithArgs([]string{"--force", imagePath, imgURL}...), + e2e.ExpectExit(0), + ) + + if !fs.IsFile(cachePath) { + t.Fatalf("Cache entry '%s' is not a file", cachePath) + } + +} diff --git a/internal/pkg/cache/cache.go b/internal/pkg/cache/cache.go index c98f7c4cbf..25762050b8 100644 --- a/internal/pkg/cache/cache.go +++ b/internal/pkg/cache/cache.go @@ -111,15 +111,30 @@ func (h *Handle) GetEntry(cacheType string, hash string) (e *Entry, err error) { cacheDir, err := h.GetFileCacheDir(cacheType) if err != nil { - return nil, fmt.Errorf("cannot get cache entry: %v", err) + return nil, fmt.Errorf("cannot get '%s' cache directory: %v", cacheType, err) } e.Path = filepath.Join(cacheDir, hash) - _, err = os.Stat(e.Path) - // If there is no existing dir return an entry with a TmpPath for the caller + // If there is a directory it's from an older version of Singularity + // We need to remove it as we work with single files per hash only now + if fs.IsDir(e.Path) { + sylog.Debugf("Removing old cache directory: %s", e.Path) + err := os.RemoveAll(e.Path) + // Allow IsNotExist in case a concurrent process already removed it + if err != nil && !os.IsNotExist(err) { + return nil, fmt.Errorf("could not remove old cache directory '%s': %v", e.Path, err) + } + } + + // If there is no existing file return an entry with a TmpPath for the caller // to use and then Finalize - if err != nil && os.IsNotExist(err) { + pathExists, err := fs.PathExists(e.Path) + if err != nil { + return nil, fmt.Errorf("could not check for cache entry '%s': %v", e.Path, err) + } + + if !pathExists { e.Exists = false f, err := fs.MakeTmpFile(cacheDir, "tmp_", 0700) if err != nil { @@ -132,12 +147,13 @@ func (h *Handle) GetEntry(cacheType string, hash string) (e *Entry, err error) { e.TmpPath = f.Name() return e, nil } - // Other error in Stat - if err != nil { - return nil, err + + // Double check that there isn't something else weird there + if !fs.IsFile(e.Path) { + return nil, fmt.Errorf("path '%s' exists but is not a file", e.Path) } - // It exists. Caller can use the Path directly + // It exists in the cache and it's a file. Caller can use the Path directly e.Exists = true return e, nil } diff --git a/internal/pkg/cache/entry.go b/internal/pkg/cache/entry.go index c4884e6784..13686472d7 100644 --- a/internal/pkg/cache/entry.go +++ b/internal/pkg/cache/entry.go @@ -10,9 +10,8 @@ import ( "fmt" "os" - "github.com/sylabs/singularity/internal/pkg/util/fs" - "github.com/sylabs/singularity/internal/pkg/sylog" + "github.com/sylabs/singularity/internal/pkg/util/fs" ) // Entry is a structure representing an entry in the cache. An entry is a file under the diff --git a/internal/pkg/client/library/pull.go b/internal/pkg/client/library/pull.go index 072239b456..4b5c77bfb0 100644 --- a/internal/pkg/client/library/pull.go +++ b/internal/pkg/client/library/pull.go @@ -53,10 +53,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom string } else { cacheEntry, err := imgCache.GetEntry(cache.LibraryCacheType, libraryImage.Hash) - defer cacheEntry.CleanTmp() if err != nil { return "", fmt.Errorf("unable to check if %v exists in cache: %v", libraryImage.Hash, err) } + defer cacheEntry.CleanTmp() if !cacheEntry.Exists { sylog.Infof("Downloading library image") diff --git a/internal/pkg/client/net/pull.go b/internal/pkg/client/net/pull.go index d6b436ea8b..3b54f0d626 100644 --- a/internal/pkg/client/net/pull.go +++ b/internal/pkg/client/net/pull.go @@ -139,10 +139,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom string } else { cacheEntry, err := imgCache.GetEntry(cache.NetCacheType, hash) - defer cacheEntry.CleanTmp() if err != nil { return "", fmt.Errorf("unable to check if %v exists in cache: %v", hash, err) } + defer cacheEntry.CleanTmp() if !cacheEntry.Exists { sylog.Infof("Downloading network image") diff --git a/internal/pkg/client/oci/pull.go b/internal/pkg/client/oci/pull.go index e4aea5210c..f21be97509 100644 --- a/internal/pkg/client/oci/pull.go +++ b/internal/pkg/client/oci/pull.go @@ -40,10 +40,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom, tmpDi } else { cacheEntry, err := imgCache.GetEntry(cache.OciTempCacheType, hash) - defer cacheEntry.CleanTmp() if err != nil { return "", fmt.Errorf("unable to check if %v exists in cache: %v", hash, err) } + defer cacheEntry.CleanTmp() if !cacheEntry.Exists { sylog.Infof("Converting OCI blobs to SIF format") diff --git a/internal/pkg/client/oras/pull.go b/internal/pkg/client/oras/pull.go index 95328eb982..c1917cc043 100644 --- a/internal/pkg/client/oras/pull.go +++ b/internal/pkg/client/oras/pull.go @@ -32,10 +32,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom string } else { cacheEntry, err := imgCache.GetEntry(cache.OrasCacheType, hash) - defer cacheEntry.CleanTmp() if err != nil { return "", fmt.Errorf("unable to check if %v exists in cache: %v", hash, err) } + defer cacheEntry.CleanTmp() if !cacheEntry.Exists { sylog.Infof("Downloading oras image") diff --git a/internal/pkg/client/shub/pull.go b/internal/pkg/client/shub/pull.go index ceabb6ef6c..b70c0cb974 100644 --- a/internal/pkg/client/shub/pull.go +++ b/internal/pkg/client/shub/pull.go @@ -141,10 +141,10 @@ func pull(ctx context.Context, imgCache *cache.Handle, directTo, pullFrom string imagePath = directTo } else { cacheEntry, err := imgCache.GetEntry(cache.ShubCacheType, manifest.Commit) - defer cacheEntry.CleanTmp() if err != nil { return "", fmt.Errorf("unable to check if %v exists in cache: %v", manifest.Commit, err) } + defer cacheEntry.CleanTmp() if !cacheEntry.Exists { sylog.Infof("Downloading shub image")