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.

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(sync): fixed skipping docker images when they already synced #1521

Merged

Conversation

eusebiu-constantin-petu-dbk
Copy link
Collaborator

@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk commented Jun 15, 2023

What type of PR is this?
bug

Which issue does this PR fix:
#1490

What does this PR do / Why do we need it:
currently docker images are downloaded every time, because comparison between remote and local is done before conversion of docker images to oci.
convert docker manifests to oci manifests before syncing, compare with local one and skip if it's already synced

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?
no

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk force-pushed the sync_docker_digests branch 2 times, most recently from 0b49a95 to c10f7dd Compare June 15, 2023 12:04
@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk changed the title Sync docker digests fix(sync): fixed skipping docker images when they already synced Jun 15, 2023
@eusebiu-constantin-petu-dbk
Copy link
Collaborator Author

need to add tests

@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Merging #1521 (40f8675) into main (aab1496) will increase coverage by 0.00%.
The diff coverage is 91.50%.

@@           Coverage Diff            @@
##             main    #1521    +/-   ##
========================================
  Coverage   91.19%   91.19%            
========================================
  Files         114      114            
  Lines       22757    22862   +105     
========================================
+ Hits        20753    20849    +96     
- Misses       1495     1501     +6     
- Partials      509      512     +3     
Impacted Files Coverage Δ
pkg/extensions/sync/utils.go 95.33% <90.32%> (-4.67%) ⬇️
pkg/api/authz.go 96.49% <100.00%> (+0.01%) ⬆️
pkg/extensions/sync/remote.go 82.75% <100.00%> (+2.49%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pmirshad
Copy link

Thank you for doing this. I was debugging the same behavior as #1490 when mirroring from Amazon ECR (on-demand). Came across your PR as I was hacking in this support.

@eusebiu-constantin-petu-dbk eusebiu-constantin-petu-dbk force-pushed the sync_docker_digests branch 5 times, most recently from 20d6240 to af93db2 Compare June 21, 2023 17:04
before syncing an image we first check if it's already present in our storage
to do that we get the manifest from remote and compare it with the local one
but in the case of syncing docker images, because the conversion to OCI format is done while
syncing, we get a docker manifest before conversion, so sync detects that local manifest and
remote one are different, so it starts syncing again.

to overcome this, convert remote docker manifests to OCI manifests and then compare.

Signed-off-by: Petu Eusebiu <[email protected]>
@eusebiu-constantin-petu-dbk
Copy link
Collaborator Author

Thank you for doing this. I was debugging the same behavior as #1490 when mirroring from Amazon ECR (on-demand). Came across your PR as I was hacking in this support.

Hello @pmirshad do you want to independently test this patch?

@rchincha rchincha linked an issue Jun 21, 2023 that may be closed by this pull request
@rchincha rchincha added this to the v2.0.0 milestone Jun 21, 2023
Copy link
Contributor

@rchincha rchincha left a comment

Choose a reason for hiding this comment

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

lgtm

@rchincha rchincha merged commit 377aff1 into project-zot:main Jun 21, 2023
@pmirshad
Copy link

Hello @pmirshad do you want to independently test this patch?

I had patched in your changes over the latest main and was running it for the past two days. Works as expected. Log snippet from today.

{"level":"info","repository":"alpine","reference":"3.13","goroutine":817,"caller":"zotregistry.io/zot/pkg/api/routes.go:1736","time":"2023-06-22T10:34:49.656152094Z","message":"trying to get updated image by syncing on demand"}
{"level":"info","goroutine":819,"caller":"zotregistry.io/zot/pkg/extensions/sync/service.go:433","time":"2023-06-22T10:34:49.656326359Z","message":"getting available client"}
{"level":"info","remote":"https://<ecr-endpoint>","repo":"alpine","reference":"3.13","goroutine":819,"caller":"zotregistry.io/zot/pkg/extensions/sync/service.go:244","time":"2023-06-22T10:34:49.672344378Z","message":"sync: syncing image"}
{"level":"info","image":"<ecr-endpoint>/alpine:3.13","goroutine":819,"caller":"zotregistry.io/zot/pkg/extensions/sync/service.go:418","time":"2023-06-22T10:34:50.129544448Z","message":"skipping image because it's already synced"} <<<<
{"level":"info","image":"<ecr-endpoint>/alpine:3.13","goroutine":819,"caller":"zotregistry.io/zot/pkg/extensions/sync/service.go:421","time":"2023-06-22T10:34:50.129757627Z","message":"sync: finished syncing image"}
...
{"level":"info","module":"http","clientIP":"<ip>","method":"HEAD","path":"/v2/alpine/manifests/3.13?ns=<ecr-endpoint>","statusCode":200,"latency":"0s","bodySize":0,"headers":{"Accept":["application/vnd.docker.distribution.manifest.v2+json, application/vnd.docker.distribution.manifest.list.v2+json, application/vnd.oci.image.manifest.v1+json, application/vnd.oci.image.index.v1+json, */*"],"User-Agent":["containerd/1.4.13"]},"goroutine":817,"caller":"zotregistry.io/zot/pkg/api/session.go:131","time":"2023-06-22T10:34:50.33479259Z","message":"HTTP API"}
{"level":"info","module":"http","clientIP":"<ip>","method":"GET","path":"/v2/alpine/manifests/sha256:2fefe8f7522d4db9e4a67f2efa41fcad31cd6544c21b787c5dacde72c1734563?ns=<ecr-endpoint>","statusCode":200,"latency":"0s","bodySize":405,"headers":{"Accept":["application/vnd.oci.image.manifest.v1+json, */*"],"Accept-Encoding":["gzip"],"User-Agent":["containerd/1.4.13"]},"goroutine":843,"caller":"zotregistry.io/zot/pkg/api/session.go:131","time":"2023-06-22T10:34:50.349163883Z","message":"HTTP API"}
{"level":"info","module":"http","clientIP":"<ip>","method":"GET","path":"/v2/alpine/blobs/sha256:76ae9b81aa35ceec6534c5202c06e7a207dd10b073602d0a7479d1da03422a80?ns=<ecr-endpoint>","statusCode":200,"latency":"0s","bodySize":585,"headers":{"Accept":["application/vnd.oci.image.config.v1+json, */*"],"Accept-Encoding":["gzip"],"User-Agent":["containerd/1.4.13"]},"goroutine":843,"caller":"zotregistry.io/zot/pkg/api/session.go:131","time":"2023-06-22T10:34:50.369382419Z","message":"HTTP API"}
{"level":"info","module":"http","clientIP":"<ip>","method":"GET","path":"/v2/alpine/blobs/sha256:4e9f2cdf438714c2c4533e28c6c41a89cc6c1b46cf77e54c488db30ca4f5b6f3?ns=<ecr-endpoint>","statusCode":200,"latency":"0s","bodySize":2814079,"headers":{"Accept":["application/vnd.oci.image.layer.v1.tar+gzip, */*"],"Accept-Encoding":["gzip"],"User-Agent":["containerd/1.4.13"]},"goroutine":843,"caller":"zotregistry.io/zot/pkg/api/session.go:131","time":"2023-06-22T10:34:50.406012831Z","message":"HTTP API"}

Thanks again 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Requested images are downloaded every time
4 participants