fit get status issue by wy65701436 · Pull Request #3755 · distribution/distribution

wy65701436 · 2022-10-23T15:46:55Z

1, return the right upload offset for client when asks.
2, do not call ResumeBlobUpload on getting status.
3, return 416 rather than 404 on failed to patch chunk blob.
4, add the missing upload close

Signed-off-by: Wang Yan wangyan@vmware.com

codecov-commenter · 2022-10-23T15:51:52Z

Codecov Report

Base: 57.06% // Head: 57.05% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (1c38c74) compared to base (fb21888).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3755      +/-   ##
==========================================
- Coverage   57.06%   57.05%   -0.01%     
==========================================
  Files         105      105              
  Lines       10806    10818      +12     
==========================================
+ Hits         6166     6172       +6     
- Misses       3952     3956       +4     
- Partials      688      690       +2

Impacted Files	Coverage Δ
registry/handlers/blobupload.go	`47.56% <66.66%> (+0.10%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

sudo-bmitch · 2022-10-23T19:48:31Z

Looking through the logic of what happens if the connection drops and a client retries an upload of the same blob to the same location (including the state), where would that fail? I'm not sure if the state is checked earlier, but if not, it looks like the following would cancel the upload:

distribution/registry/handlers/blobupload.go

Lines 314 to 321 in d3d6014

    
           if size := upload.Size(); size != buh.State.Offset { 
        
           	defer upload.Close() 
        
           	dcontext.GetLogger(ctx).Errorf("upload resumed at wrong offset: %d != %d", size, buh.State.Offset) 
        
           	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { 
        
           		buh.Errors = append(buh.Errors, v2.ErrorCodeBlobUploadInvalid.WithDetail(err)) 
        
           		upload.Cancel(buh) 
        
           	}) 
        
           }

Shouldn't that return a 416 with an updated Range and Location header instead, and not cancel the upload?

wy65701436 · 2022-10-24T06:08:15Z

In fact, according to this specification, https://github.com/distribution/distribution/blob/main/docs/spec/api.md#get-blob-upload, the client should have a way to get the upload offset through this API.

However, according to the handler dispatcher, any get method with a UUID will be processed first by ResumeBlobUpload, which doesn't make much sense.

1, http servers should not cancel uploads while processing get methods, no matter what the case.
2, think about this situation: the distribution http server restarts during the phase of writing the upload block, but some of it is successfully written to disk. At this point, the client will just retry to upload the same block to the same location. But actually the location has changed and it should be the end + the size of the partial block (successfully written). But the client has no way to know that and has to call the get-blob-upload API to return the new location(offset).

sudo-bmitch · 2022-10-24T11:41:07Z

The path I'm thinking of is:

PATCH, partial write
Retry PATCH, 416 response
Possible fallback to GET/HEAD for state
PATCH with new offset/state

I think you're implementing:

PATCH, partial write
GET/HEAD on any error to get the new offset/state
PATCH with new offset/state

The risk I'm seeing with the retry is:

PATCH, partial write
Retry PATCH, 404 response, upload canceled
All further requests fail

I can adjust the PATCH logic in my code to avoid any retries, but I think 416 would be a more appropriate response than a 404 to the retry.

milosgajdos · 2022-10-24T16:16:05Z

I think returning 416 on PATCH retry seems reasonable 🤔 In fact the spec does [sort of] cover that case

If the server cannot accept the chunk, a 416 Requested Range Not Satisfiable response will be returned and will include a Range header indicating the current status:

That being said, this PR is headed in the right direction, just feel we should expand it?

wy65701436 · 2022-10-24T16:24:53Z

I think returning 416 on PATCH retry seems reasonable 🤔 In fact the spec does [sort of] cover that case

If the server cannot accept the chunk, a 416 Requested Range Not Satisfiable response will be returned and will include a Range header indicating the current status:

That being said, this PR is headed in the right direction, just feel we should expand it?

I agree that we should disable upload.Cancel(buh) in the error handling of ResumeBlobUpload. However, I'm not sure if we need to update the response code from 404 to 416, which would be a breaking change for the client?

sudo-bmitch · 2022-10-24T17:38:48Z

That being said, this PR is headed in the right direction, just feel we should expand it?

Yes, the PR certainly fixes an issue that needs to be fixed. So either expand or a separate PR would both be good.

I picked 416 because it's defined in the spec and I think GHCR does this already.

milosgajdos · 2022-10-24T19:03:33Z

I picked 416 because it's defined in the spec and I think GHCR does this already.

Personally, I'd also go for 416. Imho, that's the correct behaviour (according to spec, anyways) -- I see nothing wrong with nudging the existing clients towards this path.

wy65701436 · 2022-10-25T12:19:28Z

@milosgajdos @sudo-bmitch I've updated the code according to the about conversation, can you help to review it?

sudo-bmitch · 2022-10-25T23:23:45Z

Overall design looks right to me, but I'm not familiar enough with the code base. I'll make an intentionally broken client to do some testing.

sudo-bmitch

Gave it a quick test and the changes LGTM.

wy65701436 · 2022-10-26T15:21:57Z

ping @milosgajdos @thaJeztah @deleteriousEffect

registry/handlers/blobupload.go

1, return the right upload offset for client when asks. 2, do not call ResumeBlobUpload on getting status. 3, return 416 rather than 404 on failed to patch chunk blob. 4, add the missing upload close Signed-off-by: Wang Yan <wangyan@vmware.com>

milosgajdos

LGTM! @deleteriousEffect PTAL

wy65701436 requested review from milosgajdos and thaJeztah October 23, 2022 15:47

sudo-bmitch mentioned this pull request Oct 23, 2022

[Issue] Getting "Not Found" while pushing to DockerHub after a sleep for backoff regclient/regclient#323

Closed

milosgajdos requested a review from deleteriousEffect October 24, 2022 10:30

sudo-bmitch approved these changes Oct 26, 2022

View reviewed changes

milosgajdos reviewed Oct 26, 2022

View reviewed changes

registry/handlers/blobupload.go Outdated Show resolved Hide resolved

fit get status issue

bad5dcb

1, return the right upload offset for client when asks. 2, do not call ResumeBlobUpload on getting status. 3, return 416 rather than 404 on failed to patch chunk blob. 4, add the missing upload close Signed-off-by: Wang Yan <wangyan@vmware.com>

milosgajdos approved these changes Oct 26, 2022

View reviewed changes

wy65701436 requested a review from heww October 28, 2022 07:07

heww approved these changes Oct 28, 2022

View reviewed changes

milosgajdos merged commit 7f9f86c into distribution:main Oct 28, 2022

sudo-bmitch mentioned this pull request Nov 4, 2022

unable to reuse reader regclient/regclient#331

Closed

sudo-bmitch mentioned this pull request Nov 26, 2022

Add a patch status to recover failed requests opencontainers/distribution-spec#366

Merged

Conversation

wy65701436 commented Oct 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sudo-bmitch commented Oct 23, 2022

Uh oh!

wy65701436 commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sudo-bmitch commented Oct 24, 2022

Uh oh!

milosgajdos commented Oct 24, 2022

Uh oh!

wy65701436 commented Oct 24, 2022

Uh oh!

sudo-bmitch commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milosgajdos commented Oct 24, 2022

Uh oh!

wy65701436 commented Oct 25, 2022

Uh oh!

sudo-bmitch commented Oct 25, 2022

Uh oh!

sudo-bmitch left a comment

Choose a reason for hiding this comment

Uh oh!

wy65701436 commented Oct 26, 2022

Uh oh!

Uh oh!

milosgajdos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wy65701436 commented Oct 23, 2022 •

edited

Loading

codecov-commenter commented Oct 23, 2022 •

edited

Loading

wy65701436 commented Oct 24, 2022 •

edited

Loading

sudo-bmitch commented Oct 24, 2022 •

edited

Loading