diff --git a/integration/build_dependencies_test.go b/integration/build_dependencies_test.go index 0a700fb731c..1f4b9073b7f 100644 --- a/integration/build_dependencies_test.go +++ b/integration/build_dependencies_test.go @@ -96,7 +96,6 @@ func TestBuildDependenciesCache(t *testing.T) { // The test then triggers another build and verifies that the images in `rebuilt` were built // (e.g., the changed images and their dependents), and that the other images were found in the artifact cache. // It runs the profile `concurrency-0` which builds with maximum concurrency. - MarkIntegrationTest(t, CanRunWithoutGcp) tests := []struct { description string change []int @@ -137,6 +136,7 @@ func TestBuildDependenciesCache(t *testing.T) { for _, test := range tests { t.Run(test.description, func(t *testing.T) { + MarkIntegrationTest(t, CanRunWithoutGcp) // modify file `foo` to invalidate cache for target artifacts for _, i := range test.change { Run(t, fmt.Sprintf("testdata/build-dependencies/app%d", i), "sh", "-c", fmt.Sprintf("echo %s > foo", uuid.New().String())) diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 080c89764a8..371ffb104f2 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -42,14 +42,6 @@ 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 32d4df2fb20..c7544853841 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -68,29 +68,30 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string, plat 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() + pls := platforms.GetPlatforms(a.ImageName) + // TODO (gaghosh): allow `tryImport` when the Docker daemon starts supporting multiarch images + // See https://github.com/docker/buildx/issues/1220#issuecomment-1189996403 + if !cacheHit && !pls.IsMultiPlatform() { + var pl v1.Platform + if len(pls.Platforms) == 1 { + pl = util.ConvertToV1Platform(pls.Platforms[0]) + } + if entry, err = c.tryImport(ctx, a, tag, hash, pl); err != nil { + log.Entry(ctx).Debugf("Could not import artifact from Docker, building instead (%s)", err) + return needsBuilding{hash: hash} + } + } + if isLocal, err := c.isLocalImage(a.ImageName); err != nil { return failed{err} } else if isLocal { - c.cacheMutex.RLock() - entry, cacheHit := c.artifactCache[hash] - c.cacheMutex.RUnlock() - - // TODO (gaghosh): allow `tryImport` when the Docker daemon starts supporting multiarch images - // See https://github.com/docker/buildx/issues/1220#issuecomment-1189996403 - if !cacheHit && !pls.IsMultiPlatform() { - var pl v1.Platform - if len(pls.Platforms) == 1 { - pl = util.ConvertToV1Platform(pls.Platforms[0]) - } - if entry, err = c.tryImport(ctx, a, tag, hash, pl); err != nil { - log.Entry(ctx).Debugf("Could not import artifact from Docker, building instead (%s)", err) - return needsBuilding{hash: hash} - } - } return c.lookupLocal(ctx, hash, tag, entry) } - return c.lookupRemote(ctx, hash, tag, pls.Platforms) + return c.lookupRemote(ctx, hash, tag, pls.Platforms, entry) } func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDetails) cacheDetails { @@ -118,41 +119,25 @@ func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDe return needsBuilding{hash: hash} } -func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []specs.Platform) cacheDetails { - c.cacheMutex.RLock() - cachedEntry, cacheHit := c.artifactCache[hash] - c.cacheMutex.RUnlock() - +func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []specs.Platform, entry ImageDetails) cacheDetails { 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) + // Image exists remotely with the same tag and digest + if remoteDigest == entry.Digest { 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() - } } - if cachedEntry.HasDigest() { - // Image exists remotely with a different tag - 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 == cachedEntry.Digest { - return needsRemoteTagging{hash: hash, tag: tag, digest: cachedEntry.Digest, platforms: platforms} - } + // 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. + if remoteDigest, err := docker.RemoteDigest(fqn, c.cfg, nil); err == nil { + if remoteDigest == entry.Digest { + return needsRemoteTagging{hash: hash, tag: tag, digest: entry.Digest, platforms: platforms} } } // Image exists locally - if cachedEntry.HasID() && c.client != nil && c.client.ImageExists(ctx, cachedEntry.ID) { - return needsPushing{hash: hash, tag: tag, imageID: cachedEntry.ID} + if entry.ID != "" && c.client != nil && c.client.ImageExists(ctx, entry.ID) { + return needsPushing{hash: hash, tag: tag, imageID: entry.ID} } return needsBuilding{hash: hash} diff --git a/pkg/skaffold/build/cache/lookup_test.go b/pkg/skaffold/build/cache/lookup_test.go index 1358a3bdb6b..fa6ede704e4 100644 --- a/pkg/skaffold/build/cache/lookup_test.go +++ b/pkg/skaffold/build/cache/lookup_test.go @@ -146,13 +146,18 @@ func TestLookupRemote(t *testing.T) { hasher artifactHasher cache map[string]ImageDetails api *testutil.FakeAPIClient - tag string expected cacheDetails }{ + { + description: "miss", + hasher: mockHasher{"hash"}, + api: &testutil.FakeAPIClient{ErrImagePull: true}, + cache: map[string]ImageDetails{}, + expected: needsBuilding{hash: "hash"}, + }, { description: "hash failure", hasher: failingHasher{errors.New("BUG")}, - tag: "tag", expected: failed{err: errors.New("getting hash for artifact \"artifact\": BUG")}, }, { @@ -161,7 +166,6 @@ func TestLookupRemote(t *testing.T) { cache: map[string]ImageDetails{ "hash": {Digest: "digest"}, }, - tag: "tag", expected: found{hash: "hash"}, }, { @@ -170,8 +174,7 @@ func TestLookupRemote(t *testing.T) { cache: map[string]ImageDetails{ "hash": {Digest: "otherdigest"}, }, - tag: "fqn_tag", - expected: needsRemoteTagging{hash: "hash", tag: "fqn_tag", digest: "otherdigest"}, + expected: needsRemoteTagging{hash: "hash", tag: "tag", digest: "otherdigest"}, }, { description: "found locally", @@ -179,9 +182,8 @@ func TestLookupRemote(t *testing.T) { cache: map[string]ImageDetails{ "hash": {ID: "imageID"}, }, - api: (&testutil.FakeAPIClient{}).Add("no_remote_tag", "imageID"), - tag: "no_remote_tag", - expected: needsPushing{hash: "hash", tag: "no_remote_tag", imageID: "imageID"}, + api: (&testutil.FakeAPIClient{}).Add("tag", "imageID"), + expected: needsPushing{hash: "hash", tag: "tag", imageID: "imageID"}, }, { description: "not found", @@ -190,7 +192,6 @@ func TestLookupRemote(t *testing.T) { "hash": {ID: "imageID"}, }, api: &testutil.FakeAPIClient{}, - tag: "no_remote_tag", expected: needsBuilding{hash: "hash"}, }, } @@ -200,7 +201,7 @@ func TestLookupRemote(t *testing.T) { switch { case identifier == "tag": return "digest", nil - case identifier == "fqn_tag@otherdigest": + case identifier == "tag@otherdigest": return "otherdigest", nil default: return "", errors.New("unknown remote tag") @@ -215,13 +216,13 @@ func TestLookupRemote(t *testing.T) { cfg: &mockConfig{mode: config.RunModes.Build}, } t.Override(&newArtifactHasherFunc, func(_ graph.ArtifactGraph, _ DependencyLister, _ config.RunMode) artifactHasher { return test.hasher }) - details := cache.lookupArtifacts(context.Background(), map[string]string{"artifact": test.tag}, platform.Resolver{}, []*latest.Artifact{{ + details := cache.lookupArtifacts(context.Background(), map[string]string{"artifact": "tag"}, platform.Resolver{}, []*latest.Artifact{{ ImageName: "artifact", }}) // cmp.Diff cannot access unexported fields in *exec.Cmd, so use reflect.DeepEqual here directly if !reflect.DeepEqual(test.expected, details[0]) { - t.Errorf("Expected result different from actual result. Expected: \n\"%v\", \nActual: \n\"%v\"", test.expected, details[0]) + t.Errorf("Expected result different from actual result. Expected: \n%v, \nActual: \n%v", test.expected, details) } }) } diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index e3e391e141a..b66b9783aab 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -220,27 +220,22 @@ func TestCacheBuildRemote(t *testing.T) { "artifact1": {"dep1", "dep2"}, "artifact2": {"dep3"}, }) - tagToDigest := map[string]string{ - "artifact1:tag1": "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165", - "artifact2:tag2": "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539", - } // Mock Docker - api := &testutil.FakeAPIClient{} - for tag, digest := range tagToDigest { - api = api.Add(tag, digest) - } - - dockerDaemon := fakeLocalDaemon(api) + dockerDaemon := fakeLocalDaemon(&testutil.FakeAPIClient{}) t.Override(&docker.NewAPIClient, func(context.Context, docker.Config) (docker.LocalDaemon, error) { return dockerDaemon, nil }) t.Override(&docker.DefaultAuthHelper, stubAuth{}) t.Override(&docker.RemoteDigest, func(ref string, _ docker.Config, _ []specs.Platform) (string, error) { - if digest, ok := tagToDigest[ref]; ok { - return digest, nil + switch ref { + case "artifact1:tag1": + return "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165", nil + case "artifact2:tag2": + return "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539", nil + default: + return "", errors.New("unknown remote tag") } - return "", errors.New("unknown remote tag") }) // Mock args builder @@ -315,11 +310,7 @@ func TestCacheFindMissing(t *testing.T) { }) // Mock Docker - api := &testutil.FakeAPIClient{} - api = api.Add("artifact1:tag1", "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165") - api = api.Add("artifact2:tag2", "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539") - - dockerDaemon := fakeLocalDaemon(api) + dockerDaemon := fakeLocalDaemon(&testutil.FakeAPIClient{}) t.Override(&docker.NewAPIClient, func(context.Context, docker.Config) (docker.LocalDaemon, error) { return dockerDaemon, nil }) @@ -348,7 +339,7 @@ func TestCacheFindMissing(t *testing.T) { pipeline: latest.Pipeline{Build: latest.BuildConfig{BuildType: latest.BuildType{LocalBuild: &latest.LocalBuild{TryImportMissing: true}}}}, cacheFile: tmpDir.Path("cache"), } - artifactCache, err := NewCache(context.Background(), cfg, func(imageName string) (bool, error) { return true, nil }, deps, graph.ToArtifactGraph(artifacts), make(mockArtifactStore)) + artifactCache, err := NewCache(context.Background(), cfg, func(imageName string) (bool, error) { return false, nil }, deps, graph.ToArtifactGraph(artifacts), make(mockArtifactStore)) t.CheckNoError(err) // Because the artifacts are in the docker registry, we expect them to be imported correctly. diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go index 1237706b455..4adc8518f97 100644 --- a/pkg/skaffold/runner/new.go +++ b/pkg/skaffold/runner/new.go @@ -229,21 +229,14 @@ func setupTrigger(triggerName string, setIntent func(bool), setAutoTrigger func( func isImageLocal(runCtx *runcontext.RunContext, imageName string) (bool, error) { pipeline, found := runCtx.PipelineForImage(imageName) if !found { - log.Entry(context.TODO()).Debugf("Didn't find pipeline for image %s. Using default pipeline!", imageName) pipeline = runCtx.DefaultPipeline() } - 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) + if pipeline.Build.GoogleCloudBuild != nil || pipeline.Build.Cluster != nil { return false, nil } // if we're deploying to local Docker, all images must be local if pipeline.Deploy.DockerDeploy != nil { - log.Entry(context.TODO()).Debugf("Image %s is local because it has docker deploy", imageName) return true, nil }