-
Notifications
You must be signed in to change notification settings - Fork 39
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
Enable chunked uploads #150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good for testing - could you please add / re-enable a test for chunked? You'll also need to figure out how to sign the DCO, which is a requirement for CNCF projects.
adb4c55
to
123e39d
Compare
Signed-off-by: Brian Cook <[email protected]> Signed-off-by: Isabella do Amaral <[email protected]>
123e39d
to
9ab5537
Compare
@isinyaaa please see my previous review comment - we need explicit tests for the chunked upload. |
Signed-off-by: Isabella do Amaral <[email protected]>
9ab5537
to
804ccb1
Compare
Nice! Let's run these tests now. |
804ccb1
to
bb59445
Compare
Oops, I apologize for the dumb mistake, updated now @vsoch . |
bb59445
to
038a509
Compare
@vsoch can you reapprove tests? I ran the linter locally to make sure it's working, sorry for the trouble |
@vsoch not sure what went wrong with those tests... maybe some issue with the generated file size? from the raw logs I can't spot any problems, neither locally. |
Here is what I see:
I would try to reproduce locally. |
038a509
to
ce6e216
Compare
Signed-off-by: Isabella do Amaral <[email protected]>
ce6e216
to
0956242
Compare
@vsoch as expected, while testing on my fork I found the problem lies when working with those very large files on GHA (workflow run on my modified main). It worked when I reduced the test file size to be a couple times the default chunk size, wdyt? |
Should the chunk size perhaps be smaller then? |
I don't really think that's a problem, up to you. The issue was in creating a 15GB test file in github actions. I think the worker didn't have this much space to spare or something. I reduced the test file to be 4x the default chunk size (4x 16MB), that's all. |
Ah ok. Please keep the tests in GitHub the same as what you are doing (and what is working) locally and let's try making more space on the builder - these first three lines before the tests to cleanup and add space should be sufficient: But you can add more as needed. |
Signed-off-by: Isabella do Amaral <[email protected]>
@vsoch updated! thanks for the pointer, I had no idea the default images could be so huge! Though the first three lines didn't suffice as we actually need 15GB for the file + 15GB for the upload. |
Hey, @vsoch, can we merge this yet? |
Yes - we are close! Can you please bump the version in oras/version.py and make a note about the change in CHANGELOG.md? That should be the final bit we need. |
Signed-off-by: Isabella do Amaral <[email protected]>
updated, wdyt? |
TL;DR: Rebases #137. Also fixes out-of-date
_state_
parameter onsession_url
, which caused a 404 when resuming/completing uploads.We wanted to use oras for larger uploads (say, ML model files) at containers/omlmd, but I wasn't able to make them work with standard uploads. I noticed #137, rebased it, addressed some of your comments (not sure how to address all of them, though). But I still couldn't make it work with https://hub.docker.com/_/registry. So I started debbuging to find out there's a
_state_
parameter being passed around, and I assume it must be updated on the https://distribution.github.io/distribution/spec/api/#completed-upload PUT request to reflect the last state reported by the server. I tested this with 20GB-ish files.I wonder if this could help with bringing back chunked file support by default, although I'm not really sure in which aspect that kind of support is "flaky" or (as I experienced) poorly documented.