diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index e9dbe72f6a0..e40994e2668 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -21,6 +21,7 @@ import ( "fmt" "io/ioutil" "path/filepath" + "sync" homedir "github.com/mitchellh/go-homedir" "github.com/sirupsen/logrus" @@ -45,6 +46,7 @@ type ArtifactCache map[string]ImageDetails // cache holds any data necessary for accessing the cache type cache struct { artifactCache ArtifactCache + cacheMutex sync.RWMutex client docker.LocalDaemon insecureRegistries map[string]bool cacheFile string @@ -81,6 +83,7 @@ func NewCache(runCtx *runcontext.RunContext, imagesAreLocal bool, tryImportMissi return &cache{ artifactCache: artifactCache, + cacheMutex: sync.RWMutex{}, client: client, insecureRegistries: runCtx.InsecureRegistries, cacheFile: cacheFile, diff --git a/pkg/skaffold/build/cache/details.go b/pkg/skaffold/build/cache/details.go index 6bb880ca7b7..97de56e74c5 100644 --- a/pkg/skaffold/build/cache/details.go +++ b/pkg/skaffold/build/cache/details.go @@ -111,6 +111,8 @@ func (d needsPushing) Push(ctx context.Context, out io.Writer, c *cache) error { } // Update cache + c.cacheMutex.Lock() + defer c.cacheMutex.Unlock() e := c.artifactCache[d.hash] e.Digest = digest c.artifactCache[d.hash] = e diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index caa9d6b6679..b68e3f76017 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -53,7 +53,9 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string) cach return failed{err: fmt.Errorf("getting hash for artifact %q: %s", a.ImageName, err)} } + c.cacheMutex.RLock() entry, cacheHit := c.artifactCache[hash] + c.cacheMutex.RUnlock() if !cacheHit { if entry, err = c.tryImport(ctx, a, tag, hash); err != nil { logrus.Debugf("Could not import artifact from Docker, building instead (%s)", err) @@ -145,7 +147,8 @@ func (c *cache) tryImport(ctx context.Context, a *latest.Artifact, tag string, h logrus.Debugf("Added digest for %s to cache entry", tag) entry.Digest = digest } - + c.cacheMutex.Lock() + defer c.cacheMutex.Unlock() c.artifactCache[hash] = entry return entry, nil } diff --git a/pkg/skaffold/build/cache/retrieve.go b/pkg/skaffold/build/cache/retrieve.go index 25546c73090..bf807389ac4 100644 --- a/pkg/skaffold/build/cache/retrieve.go +++ b/pkg/skaffold/build/cache/retrieve.go @@ -89,7 +89,10 @@ func (c *cache) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, ar } // Image is already built + c.cacheMutex.RLock() entry := c.artifactCache[result.Hash()] + c.cacheMutex.RUnlock() + tag := tags[artifact.ImageName] var uniqueTag string @@ -166,7 +169,9 @@ func (c *cache) addArtifacts(ctx context.Context, bRes []build.Artifact, hashByN entry.ID = imageID } + c.cacheMutex.Lock() c.artifactCache[hashByName[a.ImageName]] = entry + c.cacheMutex.Unlock() } return nil diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index 6adeb1bba75..8754cf73bd5 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -252,3 +252,64 @@ func TestCacheBuildRemote(t *testing.T) { t.CheckDeepEqual("artifact2", bRes[1].ImageName) }) } + +func TestCacheFindMissing(t *testing.T) { + testutil.Run(t, "", func(t *testutil.T) { + tmpDir := t.NewTempDir(). + Write("dep1", "content1"). + Write("dep2", "content2"). + Write("dep3", "content3"). + Chdir() + + runCtx := &runcontext.RunContext{ + Opts: config.SkaffoldOptions{ + CacheArtifacts: true, + CacheFile: tmpDir.Path("cache"), + }, + } + tags := map[string]string{ + "artifact1": "artifact1:tag1", + "artifact2": "artifact2:tag2", + } + artifacts := []*latest.Artifact{ + {ImageName: "artifact1", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}}, + {ImageName: "artifact2", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}}, + } + deps := depLister(map[string][]string{ + "artifact1": {"dep1", "dep2"}, + "artifact2": {"dep3"}, + }) + + // Mock Docker + dockerDaemon := docker.NewLocalDaemon(&testutil.FakeAPIClient{}, nil, false, nil) + t.Override(&docker.NewAPIClient, func(*runcontext.RunContext) (docker.LocalDaemon, error) { + return dockerDaemon, nil + }) + t.Override(&docker.DefaultAuthHelper, stubAuth{}) + t.Override(&docker.RemoteDigest, func(ref string, _ map[string]bool) (string, error) { + switch ref { + case "artifact1:tag1": + return "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165", nil + case "artifact2:tag2": + return "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539", nil + default: + return "", errors.New("unknown remote tag") + } + }) + + // Create cache + artifactCache, err := NewCache(runCtx, false, true, deps) + t.CheckNoError(err) + + // Because the artifacts are in the docker registry, we expect them to be imported correctly. + builder := &mockBuilder{dockerDaemon: dockerDaemon, push: true} + bRes, err := artifactCache.Build(context.Background(), ioutil.Discard, tags, artifacts, builder.BuildAndTest) + + t.CheckNoError(err) + t.CheckDeepEqual(0, len(builder.built)) + t.CheckDeepEqual(2, len(bRes)) + // Artifacts should always be returned in their original order + t.CheckDeepEqual("artifact1", bRes[0].ImageName) + t.CheckDeepEqual("artifact2", bRes[1].ImageName) + }) +}