Introduce RWMutex to prevent concurrent read/write during to cache du…

…ring lookup

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

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
 }

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
 }

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

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