Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: puling images when working with a remote repository (#9177) #9181

Merged
merged 3 commits into from
Nov 30, 2023
Merged

fix: puling images when working with a remote repository (#9177) #9181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d5b92f
fix: puling images when working with a remote repository (#9177)
cydan33 Nov 18, 2023
c75b65d
Fixed the tests
cydan33 Nov 27, 2023
5b15710
Merge branch 'main' of github.com:GoogleContainerTools/skaffold into …
cydan33 Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions pkg/skaffold/build/cache/lookup.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading