diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 749ebf4f3b2..0f15d941268 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 diff --git a/pkg/skaffold/build/cache/details.go b/pkg/skaffold/build/cache/details.go index 6bb880ca7b7..512dc930cef 100644 --- a/pkg/skaffold/build/cache/details.go +++ b/pkg/skaffold/build/cache/details.go @@ -111,8 +111,10 @@ func (d needsPushing) Push(ctx context.Context, out io.Writer, c *cache) error { } // Update cache + c.cacheMutex.Lock() e := c.artifactCache[d.hash] e.Digest = digest c.artifactCache[d.hash] = e + c.cacheMutex.Unlock() return nil } diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index caa9d6b6679..98b3274a256 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) @@ -146,6 +148,8 @@ func (c *cache) tryImport(ctx context.Context, a *latest.Artifact, tag string, h entry.Digest = digest } + c.cacheMutex.Lock() c.artifactCache[hash] = entry + c.cacheMutex.Unlock() return entry, nil } diff --git a/pkg/skaffold/build/cache/retrieve.go b/pkg/skaffold/build/cache/retrieve.go index 25546c73090..380a7cf8a3e 100644 --- a/pkg/skaffold/build/cache/retrieve.go +++ b/pkg/skaffold/build/cache/retrieve.go @@ -89,7 +89,9 @@ 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 +168,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 c5f7fe6372f..fafb7bd3076 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -261,6 +261,69 @@ func TestCacheBuildRemote(t *testing.T) { }) } +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() + + 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 := fakeLocalDaemon(&testutil.FakeAPIClient{}) + t.Override(&docker.NewAPIClient, func(docker.Config) (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") + } + }) + + // Mock args builder + t.Override(&docker.EvalBuildArgs, func(mode config.RunMode, workspace string, a *latest.DockerArtifact) (map[string]*string, error) { + return a.BuildArgs, nil + }) + + // Create cache + cfg := &mockConfig{ + cacheFile: tmpDir.Path("cache"), + } + artifactCache, err := NewCache(cfg, 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: false} + 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) + }) +} + type mockConfig struct { runcontext.RunContext // Embedded to provide the default values. cacheFile string