From 9376dc047aded2adb188f599267fbb829a327dfd Mon Sep 17 00:00:00 2001 From: xord37 Date: Thu, 30 Nov 2023 19:02:01 +0200 Subject: [PATCH] fix: puling images when working with a remote repository (#9177) (#9181) * fix: puling images when working with a remote repository (#9177) If the skaffold is configured to use the cluster and not the local images, it will not pull the images locally. If everything is cached it will move to deployment stage instead of unnecessarily downloading the images. * Fixed the tests --------- Co-authored-by: daniel --- pkg/skaffold/build/cache/lookup.go | 37 +++++++++++++++++------ pkg/skaffold/build/cache/retrieve_test.go | 14 +++++++-- pkg/skaffold/runner/new.go | 3 ++ 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index c7544853841..5af76ae654c 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -75,22 +75,35 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string, plat 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 { + 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) } + if !cacheHit { + entry := ImageDetails{} + if digest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil { + log.Entry(ctx).Debugf("Added digest for %s to cache entry", tag) + entry.Digest = digest + entry.ID = "" + } + log.Entry(ctx).Debugf("remote digest Error %s", err) + + c.cacheMutex.Lock() + c.artifactCache[hash] = entry + c.cacheMutex.Unlock() + } return c.lookupRemote(ctx, hash, tag, pls.Platforms, entry) } @@ -120,8 +133,10 @@ 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, entry ImageDetails) cacheDetails { + log.Entry(ctx).Debugf("Looking up %s tag when in entry it's %s", tag, entry.Digest) if remoteDigest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil { // Image exists remotely with the same tag and digest + log.Entry(ctx).Debugf("Found %s remote", tag) if remoteDigest == entry.Digest { return found{hash: hash} } @@ -129,7 +144,9 @@ func (c *cache) lookupRemote(ctx context.Context, hash, tag string, 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. + log.Entry(ctx).Debugf("Looking up %s tag with the full fqn %s", tag, entry.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} } diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index b66b9783aab..703f1162967 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -222,7 +222,11 @@ func TestCacheBuildRemote(t *testing.T) { }) // Mock Docker - dockerDaemon := fakeLocalDaemon(&testutil.FakeAPIClient{}) + api := &testutil.FakeAPIClient{} + api = api.Add("artifact1:tag1", "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165") + api = api.Add("artifact2:tag2", "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539") + + dockerDaemon := fakeLocalDaemon(api) t.Override(&docker.NewAPIClient, func(context.Context, docker.Config) (docker.LocalDaemon, error) { return dockerDaemon, nil }) @@ -310,7 +314,11 @@ func TestCacheFindMissing(t *testing.T) { }) // Mock Docker - dockerDaemon := fakeLocalDaemon(&testutil.FakeAPIClient{}) + api := &testutil.FakeAPIClient{} + api = api.Add("artifact1:tag1", "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165") + api = api.Add("artifact2:tag2", "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539") + + dockerDaemon := fakeLocalDaemon(api) t.Override(&docker.NewAPIClient, func(context.Context, docker.Config) (docker.LocalDaemon, error) { return dockerDaemon, nil }) @@ -339,7 +347,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 false, nil }, deps, graph.ToArtifactGraph(artifacts), make(mockArtifactStore)) + artifactCache, err := NewCache(context.Background(), cfg, func(imageName string) (bool, error) { return true, 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 4adc8518f97..747fdf0ffca 100644 --- a/pkg/skaffold/runner/new.go +++ b/pkg/skaffold/runner/new.go @@ -229,14 +229,17 @@ 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 || pipeline.Build.Cluster != nil { + log.Entry(context.TODO()).Debugf("Image %s is remote because it has GoogleCloudBuild or pipeline.Build.Cluster", imageName) 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 }