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: add patch for registry layers larger than 10G with S3 backend #16322

Merged
merged 3 commits into from
Jun 2, 2022

Conversation

franznemeth
Copy link
Contributor

@franznemeth franznemeth commented Feb 2, 2022

This PR fixes an issue if you use S3 as a storage backend, where you cannot push layers larger than 10GB in size.

The registry fails with the following message:

harbor-registry-7fd5cd687b-xz4kc registry time="2022-02-01T10:11:34.289971246Z" level=error msg="upload resumed at wrong offest: 5242880000 != 7615513624" auth.user.name="harbor_registry_user" go.version=go1.15.6 http.request.host=registry.example.org http.request.id=<removed> http.request.method=PUT http.request.remoteaddr=<removed> http.request.uri="/v2/<removed>" http.request.useragent="docker/19.03.5 go/go1.12.12 git-commit/2ee0c57608 os/windows arch/amd64 UpstreamClient(Docker-Client/19.03.5 \(windows\))" vars.name="<removed>" vars.uuid=<removed>

This problem was fixed upstream in the distribution/distribution repository through distribution/distribution#2815 but never included in a release which is why I am adding it as a patch to the registry version that harbor is building.

If you want me to adapt https://github.com/goharbor/harbor/blob/main/Makefile#L111 as well please tell me.

Signed-off-by: Franz Nemeth franz.nemeth@dynatrace.com

Closes #15719

Signed-off-by: Franz Nemeth <franz.nemeth@dynatrace.com>
@stonezdj
Copy link
Contributor

stonezdj commented Feb 8, 2022

It seems that the distribution v2.8 already integrate this PR and we could bump it up to v2.8 instead of patch.

@stonezdj stonezdj closed this Feb 8, 2022
@franznemeth
Copy link
Contributor Author

So that means waiting for #16190 instead of this fix?

@Vad1mo
Copy link
Member

Vad1mo commented Mar 3, 2022

I think we need to reopen that or do you see other results @franznemeth?

/usr/bin/registry_DO_NOT_USE_GC -v    
/usr/bin/registry_DO_NOT_USE_GC github.com/docker/distribution v2.8.0

I tried to push

  • docker pull vad1mo/10gb-random-file:multilayer 10 layer a 1GB result: WORKS 🎉
  • docker pull vad1mo/10gb-random-file:latest 1 layer a 10 GB result: FAIL 🔥

here the repo with sample files: https://hub.docker.com/r/vad1mo/10gb-random-file/tags

@franznemeth
Copy link
Contributor Author

No I've had the exact same results. At the time of testing there was only a beta release of 2.8.0 so I was assuming that it would be fixed in the release.
I have since used my patch in our deployment since this is a real world problem blocking us from using certain images in our environment (e.g. windows images with large application installations like visual studio).

I would suggest reopening the PR

@Vad1mo
Copy link
Member

Vad1mo commented Mar 4, 2022

Ok, In the meantime I figured out regarding the upstream docker distribution schedule.

The patch distribution/distribution#2815 was merged into main but did not make it into the 2.8.0 and 2.8.1 release. It seems like this will be part of registry 3.x.

@stonezdj can you please reopen this ticket or even better can we accept it.

Consequence
If we don't accept this patch, we have to wait months or years for registry/distribution 3.x. Until that happens, Harbor will fail on layers >10GB?

@stonezdj stonezdj reopened this Mar 4, 2022
@Vad1mo Vad1mo requested a review from stonezdj April 5, 2022 06:46
@Vad1mo Vad1mo assigned Vad1mo and stonezdj and unassigned Vad1mo Apr 5, 2022
@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #16322 (7ccba0f) into main (5afd110) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16322      +/-   ##
==========================================
- Coverage   67.28%   67.28%   -0.01%     
==========================================
  Files         970      970              
  Lines       81267    81267              
  Branches     2550     2550              
==========================================
- Hits        54684    54681       -3     
- Misses      22878    22881       +3     
  Partials     3705     3705              
Flag Coverage Δ
unittests 67.28% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...es/vulnerability/vulnerability-config.component.ts 58.51% <0.00%> (-4.45%) ⬇️
src/lib/cache/util.go 89.47% <0.00%> (+15.78%) ⬆️

@Vad1mo Vad1mo removed the request for review from wy65701436 June 1, 2022 16:42
@Vad1mo Vad1mo added the release-note/enhancement Label to mark PR to be added under release notes as enhancement label Jun 1, 2022
@OrlinVasilev OrlinVasilev changed the title fix: add patch for registry layers larger than 10G with S3 backend fix: add patch for registry layers larger than 10G with S3 backend new Jun 2, 2022
@OrlinVasilev OrlinVasilev changed the title fix: add patch for registry layers larger than 10G with S3 backend new fix: add patch for registry layers larger than 10G with S3 backend Jun 2, 2022
@OrlinVasilev
Copy link
Member

@franznemeth can you rebase please and push!

@Vad1mo
Copy link
Member

Vad1mo commented Jun 2, 2022

@franznemeth, would you mind pushing again so that the CI runs again. Somehow we can't rerun it ourself 😞

@franznemeth franznemeth requested a review from a team as a code owner June 2, 2022 12:40
@Vad1mo Vad1mo merged commit 7c2e591 into goharbor:main Jun 2, 2022
@kmlebedev kmlebedev mentioned this pull request Aug 25, 2022
5 tasks
sluetze pushed a commit to sluetze/harbor that referenced this pull request Oct 29, 2022
…oharbor#16322)

add patch for registry layers larger than 10G with S3 backend

Signed-off-by: Franz Nemeth <franz.nemeth@dynatrace.com>
@franznemeth franznemeth deleted the fix/support-large-s3-layers branch April 24, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/registry env/storage/s3 release-note/enhancement Label to mark PR to be added under release notes as enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uploading image with large layers fails after push
4 participants