From df0264229733d654ae0f43466e760dae936b12e7 Mon Sep 17 00:00:00 2001 From: ericzzzzzzz <102683393+ericzzzzzzz@users.noreply.github.com> Date: Wed, 7 Feb 2024 14:18:42 -0500 Subject: [PATCH] fix(lookupRemote): cherry-pick cache remote fix (#9301) * fix(lookupRemote): fixed lookup.go lookupRemote to compare remote and cached digests (#9278) * fix(lookupRemote): fixed lookup.go lookupRemote to compare remote and cached digests * fixes * fixed lookup.go * fixes * feat: schema * lint --------- Co-authored-by: idsulik <3595194+idsulik@users.noreply.github.com> --- deploy/skaffold/Dockerfile.deps | 2 +- deploy/skaffold/Dockerfile.deps.slim | 2 +- hack/versions/pkg/schema/check.go | 2 +- pkg/skaffold/build/cache/cache.go | 8 ++++ pkg/skaffold/build/cache/lookup.go | 46 ++++++++++++----------- pkg/skaffold/build/cache/retrieve_test.go | 34 +++++------------ pkg/skaffold/runner/new.go | 8 +++- 7 files changed, 50 insertions(+), 52 deletions(-) diff --git a/deploy/skaffold/Dockerfile.deps b/deploy/skaffold/Dockerfile.deps index 00181f47fb6..882c2546e0f 100644 --- a/deploy/skaffold/Dockerfile.deps +++ b/deploy/skaffold/Dockerfile.deps @@ -163,5 +163,5 @@ RUN apt-get update && apt-get install --no-install-recommends --no-install-sugge jq \ apt-transport-https && \ rm -rf /var/lib/apt/lists/* -COPY --from=golang:1.21.0 /usr/local/go /usr/local/go +COPY --from=golang:1.21.6 /usr/local/go /usr/local/go ENV PATH /usr/local/go/bin:/root/go/bin:$PATH diff --git a/deploy/skaffold/Dockerfile.deps.slim b/deploy/skaffold/Dockerfile.deps.slim index 0a364469408..93360877727 100644 --- a/deploy/skaffold/Dockerfile.deps.slim +++ b/deploy/skaffold/Dockerfile.deps.slim @@ -104,5 +104,5 @@ RUN apt-get update && apt-get install --no-install-recommends --no-install-sugge jq \ apt-transport-https && \ rm -rf /var/lib/apt/lists/* -COPY --from=golang:1.21.0 /usr/local/go /usr/local/go +COPY --from=golang:1.21.6 /usr/local/go /usr/local/go ENV PATH /usr/local/go/bin:/root/go/bin:$PATH diff --git a/hack/versions/pkg/schema/check.go b/hack/versions/pkg/schema/check.go index ecdcf8562ab..7ba07a58507 100644 --- a/hack/versions/pkg/schema/check.go +++ b/hack/versions/pkg/schema/check.go @@ -28,7 +28,7 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/hack/versions/pkg/diff" ) -const baseRef = "origin/main" +const baseRef = "origin/release/v2.10" func RunSchemaCheckOnChangedFiles() error { git, err := newGit(baseRef) diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 371ffb104f2..080c89764a8 100644 --- 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 index e07f87f07cb..32d4df2fb20 100644 --- 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 index a412d4d1f50..e3e391e141a 100644 --- 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 index 747fdf0ffca..1237706b455 100644 --- 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 }