fix: Scope Issue with the 'entry' variable when looking up remote images and tests additions #9211
Conversation
…ty and add more tests to test the process of looking up remotely without building the images
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9211 +/- ##
==========================================
- Coverage 70.48% 63.64% -6.85%
==========================================
Files 515 632 +117
Lines 23150 32552 +9402
==========================================
+ Hits 16317 20717 +4400
- Misses 5776 10240 +4464
- Partials 1057 1595 +538 ☔ View full report in Codecov by Sentry. |
|
hi, @3droj7 |
|
@ericzzzzzzz What am I missing here? Are there some different logs from this PR that you can post here? |
… digest in the remote (but it shouldn't use the cache if the tagged image exists remotely)
|
@ericzzzzzzz It seems that the PR integration tests are passing after my last commit but the "kokoro ci build" still fails and I'm not allowed to see the logs. |
|
Hi @3droj7 the log is the same as I pasted before, I haven't got a chance to look into the detail, but it seems that you need to have a remote registry and your active k8s context should be a remote cluster. |
|
@ericzzzzzzz This is really weird because the same tests are passing in the Besides that, we're currently using the compiled version of Skaffold with this fix in our environments specifically because we have a remote registry and a remote cluster, and so far we didn't have any issue. What do you think is the difference between the integration tests environment and the kokoro CI build environment? |
|
Hi @3droj7 I've already posted the whole log 2/integration/TestBuildDependenciesCache
helper.go:260: Running [skaffold build --default-repo us-central1-docker.pkg.dev/k8s-skaffold/testing --cache-artifacts=true -p concurrency-0] in testdata/build-dependencies
helper.go:260: Ran [skaffold build --default-repo us-central1-docker.pkg.dev/k8s-skaffold/testing --cache-artifacts=true -p concurrency-0] in 3.238 seconds
time="2023-12-06T15:12:02Z" level=info msg="DOCKER_HOST env is not set, using the host from docker context." subtask=-1 task=DevLoop
build_dependencies_test.go:172: expected image 'us-central1-docker.pkg.dev/k8s-skaffold/testing/image1:latest' not present
build_dependencies_test.go:173: expected image 'us-central1-docker.pkg.dev/k8s-skaffold/testing/image2:latest' not present
build_dependencies_test.go:174: expected image 'us-central1-docker.pkg.dev/k8s-skaffold/testing/image3:latest' not present
build_dependencies_test.go:175: expected image 'us-central1-docker.pkg.dev/k8s-skaffold/testing/image4:latest' not presentThe internal platform uses a remote registry and a remote cluster, while github action uses minikube for testing, note that remote registry and cluster are long running service and are shared when multiple CI instances are running tests, github action CI testing environments are isolated, but the test failure might not be caused by resource conflicts, normally we see test failures caused by race condition are failed on some runs, on some of the testing machines, but this test is failing on every machine. |
|
Hey @ericzzzzzzz I've now had some time to review the issue, and it seems that the I believe the solution could be to bypass the initial Alternatively, we could change the dependency beforehand. This way, the tags would differ from those in the remote registry, ensuring that the images are always built locally. What do you think? |
|
hi @3droj7 , Thank you for the investigation! Yeah, let's bypass the test against a remote cluster, you can achieve this by moving to the beginning of the test funciton. |
| c.cacheMutex.Lock() | ||
| c.artifactCache[hash] = entry | ||
| c.cacheMutex.Unlock() | ||
| return found{hash: hash} |
There was a problem hiding this comment.
Hi @ericzzzzzzz @3droj7,
I have a concern about the lookupRemote function. It appears that the function may always return found when the same tag is reused, even if the digests are different.
To fix this issue, the lookupRemote function should check the digest of the image in the cache. If the digests are different, the function should return needsRemoteTagging.
Is this correct?
Thanks.
…mote images and tests additions (GoogleContainerTools#9211)" This reverts commit ffe769c.
* Revert "fix(lookupRemote): fixed lookup.go lookupRemote to compare remote and cached digests (#9278)" This reverts commit 9ff4546. * Revert "fix: Scope Issue with the 'entry' variable when looking up remote images and tests additions (#9211)" This reverts commit ffe769c. * Revert "fix: puling images when working with a remote repository (#9177) (#9181)" This reverts commit 9376dc0.
Related: #9177
Description
The variable "entry" was not overridden before triggering the lookupRemote method.
The reason is because the entry variable was redeclared within an if block, resulting in a new, locally scoped variable. This redeclaration prevented the updates made to entry within this block from being reflected outside of it.
The bug was introduced in my last PR (9181)... I had no idea that in go, the "if" statement can define a whole new scope
I've also added some tests, so it would test the scenario where we have tags in the remote which shouldn't be built and also fixed another test where it uses missingPull functionality on the remote