fix(lookupRemote): fixed lookup.go lookupRemote to compare remote and…

… cached digests (GoogleContainerTools#9278) * fix(lookupRemote): fixed lookup.go lookupRemote to compare remote and cached digests * fixes * fixed lookup.go * fixes
ericzzzzzzz · Jan 31, 2024 · 9ff4546 · 9ff4546
1 parent 6ea9aeb
commit 9ff4546
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 49 deletions.
diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go
@@ -42,6 +42,14 @@ type ImageDetails struct {
 	ID     string `yaml:"id,omitempty"`
 }
 
+func (d ImageDetails) HasID() bool {
+	return d.ID != ""
+}
+
+func (d ImageDetails) HasDigest() bool {
+	return d.Digest != ""
+}
+
 // ArtifactCache is a map of [artifact dependencies hash : ImageDetails]
 type ArtifactCache map[string]ImageDetails
 

diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go
@@ -119,38 +119,40 @@ func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDe
 }
 
 func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []specs.Platform) cacheDetails {
-	var cacheHit bool
-	entry := ImageDetails{}
+	c.cacheMutex.RLock()
+	cachedEntry, cacheHit := c.artifactCache[hash]
+	c.cacheMutex.RUnlock()
 
-	if digest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil {
-		log.Entry(ctx).Debugf("Found %s remote", tag)
-		entry.Digest = digest
+	if remoteDigest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil {
+		if cacheHit && remoteDigest == cachedEntry.Digest {
+			log.Entry(ctx).Debugf("Found %s remote with the same digest", tag)
+			return found{hash: hash}
+		}
 
-		c.cacheMutex.Lock()
-		c.artifactCache[hash] = entry
-		c.cacheMutex.Unlock()
-		return found{hash: hash}
+		if !cacheHit {
+			log.Entry(ctx).Debugf("Added digest for %s to cache entry", tag)
+			cachedEntry.Digest = remoteDigest
+			c.cacheMutex.Lock()
+			c.artifactCache[hash] = cachedEntry
+			c.cacheMutex.Unlock()
+		}
 	}
 
-	c.cacheMutex.RLock()
-	entry, cacheHit = c.artifactCache[hash]
-	c.cacheMutex.RUnlock()
-
-	if cacheHit {
+	if cachedEntry.HasDigest() {
 		// Image exists remotely with a different tag
-		fqn := tag + "@" + entry.Digest // Actual tag will be ignored but we need the registry and the digest part of it.
-		log.Entry(ctx).Debugf("Looking up %s tag with the full fqn %s", tag, entry.Digest)
+		fqn := tag + "@" + cachedEntry.Digest // Actual tag will be ignored but we need the registry and the digest part of it.
+		log.Entry(ctx).Debugf("Looking up %s tag with the full fqn %s", tag, cachedEntry.Digest)
 		if remoteDigest, err := docker.RemoteDigest(fqn, c.cfg, nil); err == nil {
 			log.Entry(ctx).Debugf("Found %s with the full fqn", tag)
-			if remoteDigest == entry.Digest {
-				return needsRemoteTagging{hash: hash, tag: tag, digest: entry.Digest, platforms: platforms}
+			if remoteDigest == cachedEntry.Digest {
+				return needsRemoteTagging{hash: hash, tag: tag, digest: cachedEntry.Digest, platforms: platforms}
 			}
 		}
+	}
 
-		// Image exists locally
-		if entry.ID != "" && c.client != nil && c.client.ImageExists(ctx, entry.ID) {
-			return needsPushing{hash: hash, tag: tag, imageID: entry.ID}
-		}
+	// Image exists locally
+	if cachedEntry.HasID() && c.client != nil && c.client.ImageExists(ctx, cachedEntry.ID) {
+		return needsPushing{hash: hash, tag: tag, imageID: cachedEntry.ID}
 	}
 
 	return needsBuilding{hash: hash}

diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go
@@ -206,31 +206,23 @@ func TestCacheBuildRemote(t *testing.T) {
 			Write("dep1", "content1").
 			Write("dep2", "content2").
 			Write("dep3", "content3").
-			Write("dep4", "content4").
-			Write("dep5", "content5").
 			Chdir()
 
 		tags := map[string]string{
-			"artifact1":       "artifact1:tag1",
-			"artifact2":       "artifact2:tag2",
-			"exist_artifact1": "exist_artifact1:tag1",
-			"exist_artifact2": "exist_artifact2:tag2",
+			"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{}}},
-			{ImageName: "exist_artifact1", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}},
-			{ImageName: "exist_artifact2", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}},
 		}
 		deps := depLister(map[string][]string{
-			"artifact1":       {"dep1", "dep2"},
-			"artifact2":       {"dep3"},
-			"exist_artifact1": {"dep4"},
-			"exist_artifact2": {"dep5"},
+			"artifact1": {"dep1", "dep2"},
+			"artifact2": {"dep3"},
 		})
 		tagToDigest := map[string]string{
-			"exist_artifact1:tag1": "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165",
-			"exist_artifact2:tag2": "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539",
+			"artifact1:tag1": "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165",
+			"artifact2:tag2": "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539",
 		}
 
 		// Mock Docker
@@ -273,37 +265,29 @@ func TestCacheBuildRemote(t *testing.T) {
 
 		t.CheckNoError(err)
 		t.CheckDeepEqual(2, len(builder.built))
-		t.CheckDeepEqual(4, len(bRes))
+		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)
 
-		// Add the other tags to the remote cache
-		tagToDigest["artifact1:tag1"] = tagToDigest["exist_artifact1:tag1"]
-		tagToDigest["artifact2:tag2"] = tagToDigest["exist_artifact2:tag2"]
-		api.Add("artifact1:tag1", tagToDigest["artifact1:tag1"])
-		api.Add("artifact2:tag2", tagToDigest["artifact2:tag2"])
-
 		// Second build: both artifacts are read from cache
 		builder = &mockBuilder{dockerDaemon: dockerDaemon, push: true, cache: artifactCache}
 		bRes, err = artifactCache.Build(context.Background(), io.Discard, tags, artifacts, platform.Resolver{}, builder.Build)
 
 		t.CheckNoError(err)
 		t.CheckEmpty(builder.built)
-		t.CheckDeepEqual(4, len(bRes))
+		t.CheckDeepEqual(2, len(bRes))
 		t.CheckDeepEqual("artifact1", bRes[0].ImageName)
 		t.CheckDeepEqual("artifact2", bRes[1].ImageName)
 
 		// Third build: change one artifact's dependencies
 		tmpDir.Write("dep1", "new content")
-		tags["artifact1"] = "artifact1:new_tag1"
-
 		builder = &mockBuilder{dockerDaemon: dockerDaemon, push: true, cache: artifactCache}
 		bRes, err = artifactCache.Build(context.Background(), io.Discard, tags, artifacts, platform.Resolver{}, builder.Build)
 
 		t.CheckNoError(err)
 		t.CheckDeepEqual(1, len(builder.built))
-		t.CheckDeepEqual(4, len(bRes))
+		t.CheckDeepEqual(2, len(bRes))
 		t.CheckDeepEqual("artifact1", bRes[0].ImageName)
 		t.CheckDeepEqual("artifact2", bRes[1].ImageName)
 	})

diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go
@@ -232,8 +232,12 @@ func isImageLocal(runCtx *runcontext.RunContext, imageName string) (bool, error)
 		log.Entry(context.TODO()).Debugf("Didn't find pipeline for image %s. Using default pipeline!", imageName)
 		pipeline = runCtx.DefaultPipeline()
 	}
-	if pipeline.Build.GoogleCloudBuild != nil || pipeline.Build.Cluster != nil {
-		log.Entry(context.TODO()).Debugf("Image %s is remote because it has GoogleCloudBuild or pipeline.Build.Cluster", imageName)
+	if pipeline.Build.GoogleCloudBuild != nil {
+		log.Entry(context.TODO()).Debugf("Image %s is remote because it has pipeline.Build.GoogleCloudBuild", imageName)
+		return false, nil
+	}
+	if pipeline.Build.Cluster != nil {
+		log.Entry(context.TODO()).Debugf("Image %s is remote because it has pipeline.Build.Cluster", imageName)
 		return false, nil
 	}