Skip to content

Commit

Permalink
fix: puling images when working with a remote repository (GoogleConta…
Browse files Browse the repository at this point in the history 
…inerTools#9177) (GoogleContainerTools#9181)

* fix: puling images when working with a remote repository (GoogleContainerTools#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 <daniel@cybellum.com>
  • Loading branch information
@3droj7 @cydan33
3droj7 and cydan33 committed Nov 30, 2023
1 parent d49be7a commit 9376dc0
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 13 deletions.
37 changes: 27 additions & 10 deletions pkg/skaffold/build/cache/lookup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -120,16 +133,20 @@ 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}
}
}

// 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}
}
Expand Down
14 changes: 11 additions & 3 deletions pkg/skaffold/build/cache/retrieve_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Expand Down Expand Up @@ -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
})
Expand Down Expand Up @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions pkg/skaffold/runner/new.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down

0 comments on commit 9376dc0

Please sign in to comment.