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

[idsulik]

Fixes: #9248

Description
Hi! Within this PR #9278 I've fixed lookupRemote and it turned out that I've missed a case when the cache was missed but the image was found remotely (it'll be better to check the test case to understand it)

User facing changes (remove if N/A)
Before: when a tag wasn't found in the cache, but the image was found remotely by the tag, then we use needsRemoteTagging
After: with the same conditions it now uses needsBuilding instead

[codecov]

Codecov Report

Attention: 295 lines in your changes are missing coverage. Please review.

Comparison is base (290280e) 70.48% compared to head (b2f1273) 63.54%.
Report is 1114 commits behind head on main.

Files	Patch %	Lines
cmd/skaffold/app/cmd/exec.go	16.32%	40 Missing and 1 partial ⚠️
cmd/skaffold/app/cmd/filter.go	47.27%	22 Missing and 7 partials ⚠️
cmd/skaffold/app/cmd/lsp.go	28.12%	23 Missing ⚠️
cmd/skaffold/app/cmd/verify.go	23.33%	23 Missing ⚠️
cmd/skaffold/app/cmd/fix.go	51.16%	17 Missing and 4 partials ⚠️
cmd/skaffold/app/cmd/inspect_job_manifest_paths.go	60.00%	15 Missing and 1 partial ⚠️
cmd/skaffold/app/cmd/inspect_namespaces.go	50.00%	13 Missing and 1 partial ⚠️
...md/skaffold/app/cmd/inspect_config_dependencies.go	45.83%	12 Missing and 1 partial ⚠️
cmd/skaffold/app/cmd/lint.go	42.85%	12 Missing ⚠️
cmd/skaffold/app/cmd/inspect_build_env.go	60.71%	11 Missing ⚠️
... and 21 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9306      +/-   ##
==========================================
- Coverage   70.48%   63.54%   -6.94%     
==========================================
  Files         515      635     +120     
  Lines       23150    32799    +9649     
==========================================
+ Hits        16317    20843    +4526     
- Misses       5776    10347    +4571     
- Partials     1057     1609     +552     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[idsulik]

@ericzzzzzzz hi! Check it please, I think it'll be better to create patch release with these changes

[ericzzzzzzz]

Hi, @idsulik thank you for the fix, but I think our best option may be just roll back all changes related to cache lookup to the previous good version v2.9.0, and revisit #9177 to see if the request is valid, we definitely need to document this about how skaffold cache works and what are the expected behaviors, I'll put a short summary about how we end up here from #9177 later, and create another issue to document skaffold caching

[idsulik]

@ericzzzzzzz I've double-checked these changes and I use it daily, it works perfectly, so I think it's safe to merge and use it

[idsulik]

I missed one case in the previous PR and fixed it here + added a test for it.

I agree that we need some docs how the cache works because it was pretty hard to understand and keep in mind all the cases

[ericzzzzzzz]

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??

[idsulik]

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.

[ericzzzzzzz]

@ericzzzzzzz I've double-checked these changes and I use it daily, it works perfectly, so I think it's safe to merge and use it

if you believe you've made some improvements on the good working version -- v2.9.0, please them on the description. Thanks

I think there maybe some misunderstanding on the original issue,
When the image is remote and cached, skaffold tries to pull it locally and ignores the cache. #9177

• if an remote image is cached on previous build and digests match, skaffold should not build
• The pr https://github.com/GoogleContainerTools/skaffold/pull/9181/files and https://github.com/GoogleContainerTools/skaffold/pull/9211/files#diff-c29561cd5f488e560760ff508c85b0a485100b79f6971be9a1229278fb76b482 look for remote image with the target tag, if found it skips the build, regardless cache miss or found, it seems like @3droj7 just wants to use the image(tag) for deployment specified in the skaffold.yaml if the tag exists on remote. It seems to me that we're trying to solve a different issue.
• I don't want to keep fixing stuff forward and end up changing something not related issue, even if this is an improvement, we should create another issue for it. So I'm still thinking going back to the old state.

[idsulik]

@ericzzzzzzz

if an remote image is cached on previous build and digests match, skaffold should not build.

in this PR works in this way

[ericzzzzzzz]

@ericzzzzzzz

if an remote image is cached on previous build and digests match, skaffold should not build.

in this PR works in this way

thank you so much! I'll test it and give approve if everything works as expected.
wait, isn't v2.9.0 working as the same?

[idsulik]

I don't know about v2.9.0, if it works the same, here we have separated methods for/local remote at least, so we can look at it as refactoring

[ericzzzzzzz]

I don't know about v2.9.0, if it works the same, here we have separated methods for/local remote at least, so we can look at it as refactoring

I admit this is all my fault for not carefully reviewing the #9181 and #9211 for the requirements in #9177,

• the implementation in fix: Scope Issue with the 'entry' variable when looking up remote images and tests additions #9211 ignores the cache and just check if a image tag exists the remote, if exists use the remote image as the build result
• that caused the problem reported in lookupRemote function may always return found when the same tag is reused #9248 -- remote image lookup always returns found even cache misses or digest mismatch
• fix(lookupRemote): fixed lookup.go lookupRemote to compare remote and cached digests #9278 made an attempt to fix lookupRemote function may always return found when the same tag is reused #9248, but is misses the case where cache is not hit.
• This pr is making another patch to fix lookupRemote function may always return found when the same tag is reused #9248, but we have to stop here. Every pr after fix: Scope Issue with the 'entry' variable when looking up remote images and tests additions #9211 should have two goals
• fix the issue lookupRemote function may always return found when the same tag is reused #9248
• persist the functionality introduced in fix: Scope Issue with the 'entry' variable when looking up remote images and tests additions #9211, if the ask in When the image is remote and cached, skaffold tries to pull it locally and ignores the cache. #9177 is valid,
• we took When the image is remote and cached, skaffold tries to pull it locally and ignores the cache. #9177 as valid requirement but we missed the second goal in fix(lookupRemote): fixed lookup.go lookupRemote to compare remote and cached digests #9278 and end up here, lookup() method seems to behave the same as v2.9.0 after the pr merges, we should not merge a pr if we think When the image is remote and cached, skaffold tries to pull it locally and ignores the cache. #9177 is valid and the pr is not meant to solve it as we don't want to work on something then get a result of something else. If we believe When the image is remote and cached, skaffold tries to pull it locally and ignores the cache. #9177 is not valid, then all attempts for When the image is remote and cached, skaffold tries to pull it locally and ignores the cache. #9177, lookupRemote function may always return found when the same tag is reused #9248 should be reverted.
• As we're still having the remote cache lookup problem in lookupRemote function may always return found when the same tag is reused #9248, going back to the original good state is the best option we have.

Sorry, I have to close this and start to revert #9181, #9211, #9278