Skip to content

Commit

Permalink
fix: Scope Issue with the 'entry' variable when looking up remote ima…
Browse files Browse the repository at this point in the history 
…ges and tests additions (GoogleContainerTools#9211)

* fix: new retrieved entry is not used in the lookup remote functionality and add more tests to test the process of looking up remotely without building the images

* fix: In case there's an entry in the cache it might not find the same digest in the remote (but it shouldn't use the cache if the tagged image exists remotely)

* fix: build dependency test will be bypassed against a remote cluster

---------

Co-authored-by: daniel <[email protected]>
  • Loading branch information
@3droj7 @cydan33
3droj7 and cydan33 authored Jan 2, 2024
1 parent bead9c8 commit ffe769c
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 69 deletions.
2 changes: 1 addition & 1 deletion integration/build_dependencies_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ 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
Expand Down Expand Up @@ -136,7 +137,6 @@ 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()))
Expand Down
74 changes: 35 additions & 39 deletions pkg/skaffold/build/cache/lookup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,16 @@ 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 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 {
Expand All @@ -91,20 +90,7 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string, plat
}
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)
return c.lookupRemote(ctx, hash, tag, pls.Platforms)
}

func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDetails) cacheDetails {
Expand Down Expand Up @@ -132,29 +118,39 @@ 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, 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
func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []specs.Platform) cacheDetails {
var cacheHit bool
entry := ImageDetails{}

if digest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil {
log.Entry(ctx).Debugf("Found %s remote", tag)
if remoteDigest == entry.Digest {
return found{hash: hash}
}
entry.Digest = digest

c.cacheMutex.Lock()
c.artifactCache[hash] = entry
c.cacheMutex.Unlock()
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}
c.cacheMutex.RLock()
entry, cacheHit = c.artifactCache[hash]
c.cacheMutex.RUnlock()

if cacheHit {
// 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}
}
}
}

// 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 entry.ID != "" && c.client != nil && c.client.ImageExists(ctx, entry.ID) {
return needsPushing{hash: hash, tag: tag, imageID: entry.ID}
}
}

return needsBuilding{hash: hash}
Expand Down
25 changes: 12 additions & 13 deletions pkg/skaffold/build/cache/lookup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,18 +146,13 @@ 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")},
},
{
Expand All @@ -166,6 +161,7 @@ func TestLookupRemote(t *testing.T) {
cache: map[string]ImageDetails{
"hash": {Digest: "digest"},
},
tag: "tag",
expected: found{hash: "hash"},
},
{
Expand All @@ -174,16 +170,18 @@ func TestLookupRemote(t *testing.T) {
cache: map[string]ImageDetails{
"hash": {Digest: "otherdigest"},
},
expected: needsRemoteTagging{hash: "hash", tag: "tag", digest: "otherdigest"},
tag: "fqn_tag",
expected: needsRemoteTagging{hash: "hash", tag: "fqn_tag", digest: "otherdigest"},
},
{
description: "found locally",
hasher: mockHasher{"hash"},
cache: map[string]ImageDetails{
"hash": {ID: "imageID"},
},
api: (&testutil.FakeAPIClient{}).Add("tag", "imageID"),
expected: needsPushing{hash: "hash", tag: "tag", imageID: "imageID"},
api: (&testutil.FakeAPIClient{}).Add("no_remote_tag", "imageID"),
tag: "no_remote_tag",
expected: needsPushing{hash: "hash", tag: "no_remote_tag", imageID: "imageID"},
},
{
description: "not found",
Expand All @@ -192,6 +190,7 @@ func TestLookupRemote(t *testing.T) {
"hash": {ID: "imageID"},
},
api: &testutil.FakeAPIClient{},
tag: "no_remote_tag",
expected: needsBuilding{hash: "hash"},
},
}
Expand All @@ -201,7 +200,7 @@ func TestLookupRemote(t *testing.T) {
switch {
case identifier == "tag":
return "digest", nil
case identifier == "tag@otherdigest":
case identifier == "fqn_tag@otherdigest":
return "otherdigest", nil
default:
return "", errors.New("unknown remote tag")
Expand All @@ -216,13 +215,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": "tag"}, platform.Resolver{}, []*latest.Artifact{{
details := cache.lookupArtifacts(context.Background(), map[string]string{"artifact": test.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)
t.Errorf("Expected result different from actual result. Expected: \n\"%v\", \nActual: \n\"%v\"", test.expected, details[0])
}
})
}
Expand Down
49 changes: 33 additions & 16 deletions pkg/skaffold/build/cache/retrieve_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,40 +206,49 @@ 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",
"artifact1": "artifact1:tag1",
"artifact2": "artifact2:tag2",
"exist_artifact1": "exist_artifact1:tag1",
"exist_artifact2": "exist_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"},
"artifact1": {"dep1", "dep2"},
"artifact2": {"dep3"},
"exist_artifact1": {"dep4"},
"exist_artifact2": {"dep5"},
})
tagToDigest := map[string]string{
"exist_artifact1:tag1": "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165",
"exist_artifact2:tag2": "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539",
}

// Mock Docker
api := &testutil.FakeAPIClient{}
api = api.Add("artifact1:tag1", "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165")
api = api.Add("artifact2:tag2", "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539")
for tag, digest := range tagToDigest {
api = api.Add(tag, digest)
}

dockerDaemon := fakeLocalDaemon(api)
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) {
switch ref {
case "artifact1:tag1":
return "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165", nil
case "artifact2:tag2":
return "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539", nil
default:
return "", errors.New("unknown remote tag")
if digest, ok := tagToDigest[ref]; ok {
return digest, nil
}
return "", errors.New("unknown remote tag")
})

// Mock args builder
Expand All @@ -264,29 +273,37 @@ func TestCacheBuildRemote(t *testing.T) {

t.CheckNoError(err)
t.CheckDeepEqual(2, len(builder.built))
t.CheckDeepEqual(2, len(bRes))
t.CheckDeepEqual(4, 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(2, len(bRes))
t.CheckDeepEqual(4, 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(2, len(bRes))
t.CheckDeepEqual(4, len(bRes))
t.CheckDeepEqual("artifact1", bRes[0].ImageName)
t.CheckDeepEqual("artifact2", bRes[1].ImageName)
})
Expand Down

0 comments on commit ffe769c

Please sign in to comment.