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(lookupRemote): fixed lookup.go lookupRemote caching mechanism #9306

Closed
wants to merge 3 commits into from
Closed

fix(lookupRemote): fixed lookup.go lookupRemote caching mechanism #9306

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ccff1bf
fix(lookupRemote): fixed lookup.go lookupRemote caching mechanism
idsulik Feb 9, 2024
b85ab95
code style fix
idsulik Feb 9, 2024
b2f1273
fixed lookup.go
idsulik Feb 14, 2024
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
29 changes: 15 additions & 14 deletions pkg/skaffold/build/cache/lookup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,28 +131,29 @@ func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []

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.artifactCache[hash] = ImageDetails{Digest: remoteDigest}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think skaffold saves digest to cache by checking the remote image after build, the code here might not be necessary and if this works as the code indicates it's confusing

  • we find a remoteDigest with the target tag
  • in cache miss case, we save the digest, skaffold will rebuild and re-push, then the digest changes on remote but the digest on the cache is old one??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just checked it and found that we don't need to save the digest here because it'll be overwritten here pkg/skaffold/build/cache/details.go:121.
removed this part and pushed.

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}
if cacheHit {
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 locally
if cachedEntry.HasID() && c.client != nil && c.client.ImageExists(ctx, cachedEntry.ID) {
return needsPushing{hash: hash, tag: tag, imageID: cachedEntry.ID}
// Image exists locally
if cachedEntry.HasID() && c.client != nil && c.client.ImageExists(ctx, cachedEntry.ID) {
return needsPushing{hash: hash, tag: tag, imageID: cachedEntry.ID}
}
}

return needsBuilding{hash: hash}
Expand Down
87 changes: 58 additions & 29 deletions pkg/skaffold/build/cache/lookup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,70 +141,99 @@ func TestLookupLocal(t *testing.T) {
}

func TestLookupRemote(t *testing.T) {
commonRemoteDigestMap := map[string]string{
"tag": "digest",
"fqn_tag@otherdigest": "otherdigest",
}

tests := []struct {
description string
hasher artifactHasher
cache map[string]ImageDetails
api *testutil.FakeAPIClient
tag string
expected cacheDetails
description string
hasher artifactHasher
cache map[string]ImageDetails
remoteDigestMap map[string]string
api *testutil.FakeAPIClient
tag string
expected cacheDetails
}{
{
description: "hash failure",
hasher: failingHasher{errors.New("BUG")},
tag: "tag",
expected: failed{err: errors.New("getting hash for artifact \"artifact\": BUG")},
description: "hash failure",
hasher: failingHasher{errors.New("BUG")},
tag: "tag",
remoteDigestMap: commonRemoteDigestMap,
expected: failed{err: errors.New("getting hash for artifact \"artifact\": BUG")},
},
{
description: "hit",
description: "cache miss but remote found",
hasher: mockHasher{"hash"},
cache: map[string]ImageDetails{},
remoteDigestMap: map[string]string{
"tag": "digest",
"tag@digest": "digest",
},
tag: "tag",
expected: needsBuilding{hash: "hash"},
},
{
description: "cache hit and digests are the same",
hasher: mockHasher{"hash"},
cache: map[string]ImageDetails{
"hash": {Digest: "digest"},
},
tag: "tag",
expected: found{hash: "hash"},
remoteDigestMap: commonRemoteDigestMap,
tag: "tag",
expected: found{hash: "hash"},
},
{
description: "cache hit but digests are not the same, no remote or locally",
hasher: mockHasher{"hash"},
cache: map[string]ImageDetails{
"hash": {Digest: "otherdigest"},
},
remoteDigestMap: commonRemoteDigestMap,
tag: "tag",
expected: needsBuilding{hash: "hash"},
},
{
description: "hit with different tag",
description: "cache hit with different tag",
hasher: mockHasher{"hash"},
cache: map[string]ImageDetails{
"hash": {Digest: "otherdigest"},
},
tag: "fqn_tag",
expected: needsRemoteTagging{hash: "hash", tag: "fqn_tag", digest: "otherdigest"},
remoteDigestMap: commonRemoteDigestMap,
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("no_remote_tag", "imageID"),
tag: "no_remote_tag",
expected: needsPushing{hash: "hash", tag: "no_remote_tag", imageID: "imageID"},
remoteDigestMap: commonRemoteDigestMap,
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",
hasher: mockHasher{"hash"},
cache: map[string]ImageDetails{
"hash": {ID: "imageID"},
},
api: &testutil.FakeAPIClient{},
tag: "no_remote_tag",
expected: needsBuilding{hash: "hash"},
remoteDigestMap: commonRemoteDigestMap,
api: &testutil.FakeAPIClient{},
tag: "no_remote_tag",
expected: needsBuilding{hash: "hash"},
},
}
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
t.Override(&docker.RemoteDigest, func(identifier string, _ docker.Config, _ []specs.Platform) (string, error) {
switch {
case identifier == "tag":
return "digest", nil
case identifier == "fqn_tag@otherdigest":
return "otherdigest", nil
default:
return "", errors.New("unknown remote tag")
if digest, ok := test.remoteDigestMap[identifier]; ok {
return digest, nil
}

return "", errors.New("unknown remote tag")
})

cache := &cache{
Expand Down
Loading