fix(registry/storage/driver/s3-aws): use a consistent multipart chunk size #4424

uhthomas · 2024-07-30T23:46:32Z

Some S3 compatible object storage systems like R2 require that all multipart chunks are the same size. This was mostly true before, except the final chunk was larger than the requested chunk size which causes uploads to fail.

In addition, the two byte slices have been replaced with a single *bytes.Buffer and the surrounding code simplified significantly.

Fixes: #3873

uhthomas · 2024-07-30T23:47:18Z

@milosgajdos Would you be able to take a look?

uhthomas · 2024-07-31T00:06:07Z

So we're on the same page, the main issue was this:

distribution/registry/storage/driver/s3-aws/s3.go

Lines 1654 to 1660 in f0bd0f6

    
           buf := bytes.NewBuffer(w.ready.data) 
        
           if w.pending.Len() > 0 && w.pending.Len() < int(w.driver.ChunkSize) { 
        
           	if _, err := buf.Write(w.pending.data); err != nil { 
        
           		return err 
        
           	} 
        
           	w.pending.Clear() 
        
           }

The above would produce a huge final chunk (bigger than the specified chunk size) and would cause the upload to fail.

ianseyer · 2024-07-31T00:57:24Z

We have spun up an instance of harbor backed by R2 using this PR and have pushed small and large (4.5Gb) images with no issue and no chunksize specified. The success appears to be consistent, and was failing on an identical setup using 2.8.3

uhthomas · 2024-07-31T01:31:56Z

also @corhere @thaJeztah

uhthomas · 2024-07-31T02:05:30Z

I have done some reading, and understand this comment has concerns with simply not writing all the data which has been uploaded.

#3940 (comment)

I would need to find a more reliable way to test resumable uploads, but I agree I imagine there there is a discrepancy between what the registry says it has uploaded, versus what it actually has uploaded. Imagine the best thing to do here would be to ensure size matches exactly what data has actually been flushed. The remaining data which does not align with the chunk boundary is safe to discard as the client will just reupload it. Will wait for some thoughts and feedback, but I imagine this shouldn't be a difficult change to make.

uhthomas · 2024-07-31T02:25:54Z

I've pushed a change to address the above - essentially flush is now responsible for keeping track of the upload size, and will accurately reflect how much data has been written. Given the registry will return this in the Range header to the client, the client should correctly resume at the correct offset, which should be a chunk boundary :)

registry/storage/driver/s3-aws/s3.go

milosgajdos

I need to give this a proper look, I've only skimmed it. I also need to refresh my memory with the original issue, so might take a bit to review.

milosgajdos · 2024-07-31T14:07:50Z

registry/storage/driver/s3-aws/s3.go

-		// maxChunkSize large which fits in to int. The reason why
-		// we return int64 is to play nice with Go interfaces where
-		// the buffer implements io.ReaderFrom interface.
+	n, _ := w.buf.Write(p)


We should probably make a note here saying that error is always nil when writing to bytes.Buffer.

uhthomas · 2024-08-04T17:49:00Z

registry/storage/driver/s3-aws/s3.go


-			n, err := w.ready.ReadFrom(resp.Body)
-			if err != nil {
+			if _, err := io.Copy(w.buf, resp.Body); err != nil {


I had concerns about this - but the above condition guarantees(?) this data will be less than minChunkSize, which is "tiny" (5MB).

uhthomas · 2024-08-05T21:56:14Z

registry/storage/driver/s3-aws/s3.go

@@ -1389,8 +1332,7 @@ func (d *driver) newWriter(ctx context.Context, key, uploadID string, parts []*s
 		uploadID: uploadID,
 		parts:    parts,
 		size:     size,
-		ready:    d.NewBuffer(),
-		pending:  d.NewBuffer(),
+		buf:      d.pool.Get().(*bytes.Buffer),
 	}
 }



Question, unrelated to the changes here (sorry if distracting): Why/how could the parts ever become unordered? Shouldn't it be sufficient to sort the parts once when they are first fetched rather than when the upload is restarted or committed?

I think I'm missing some context for this 🤔

Vad1mo · 2024-08-12T12:47:47Z

This seems to solve the issue with Cloudflare, @tpoxa and I will test in on our feet and see if we find something.

BrammyS · 2024-09-26T07:40:36Z

I will also try and test this myself :)
Would really like this to be merged if everything is working fine.

uhthomas · 2024-10-08T12:12:17Z

Hope your tests went okay. If it's helpful, there is a prebuilt image here /~https://github.com/users/uhthomas/packages/container/package/distribution%2Fregistry

Vad1mo · 2024-10-11T11:53:02Z

@uhthomas @milosgajdos @corhere

Here is our feedback, after testing this feature on R2 for our service offering container-registry.com as my colleague @tpoxa was responsible for the similar PR #3940

We have moved GiBs of date to R2 to test is. Among the tests were our test images vad1mo/1gb-random-file, vad1mo/10gb-random-file that contain 1 GiB sized layer.

We didn't experience any issues or problems.
We also tested other S3 compatible backend ends, and not only with R2. No issues there.

So from our side the PR is good to go.

milosgajdos

I think the PR on the first look looks ok to me. I left some comments. I need to give it another pass at some point.

milosgajdos · 2024-10-11T14:43:40Z

docker-bake.hcl

-    "linux/arm/v6",
-    "linux/arm/v7",


Yeah, no. Let's not do this. We can't remove these. There are binaries have been released in this arch as well as official images /~https://github.com/distribution/distribution-library-image/blob/a943e89c3efe06134cd9a4b439203c5341082cbc/Dockerfile shipping them.

registry/storage/driver/s3-aws/s3.go

milosgajdos · 2024-10-11T14:45:28Z

registry/storage/driver/s3-aws/s3.go

@@ -1389,8 +1332,7 @@ func (d *driver) newWriter(ctx context.Context, key, uploadID string, parts []*s
 		uploadID: uploadID,
 		parts:    parts,
 		size:     size,
-		ready:    d.NewBuffer(),
-		pending:  d.NewBuffer(),
+		buf:      d.pool.Get().(*bytes.Buffer),
 	}
 }



I think I'm missing some context for this 🤔

milosgajdos

I think the changes here look ok to me. @uhthomas do you wanna address some of my comments? I'd like to do an RC soon and I think it'd be great if we could snuck this change in. Also, this needs a rebase now.

… size Some S3 compatible object storage systems like R2 require that all multipart chunks are the same size. This was mostly true before, except the final chunk was larger than the requested chunk size which causes uploads to fail. In addition, the two byte slices have been replaced with a single *bytes.Buffer and the surrounding code simplified significantly. Fixes: distribution#3873 Signed-off-by: Thomas Way <thomas@6f.io>

milosgajdos

LGTM

Jamstah

All looks kosher to me, thanks for the additional testing as well.

uhthomas · 2024-11-05T12:17:22Z

Thank you @milosgajdos, @Jamstah and everyone else who took the time to review and test. Really happy to see this finally get over the line!

- distribution/distribution#4424

github-actions bot added area/storage area/storage/s3 area/docs labels Jul 30, 2024

uhthomas force-pushed the 3873 branch from 1e6a8ab to 9543d4f Compare July 30, 2024 23:47

uhthomas force-pushed the 3873 branch from 9543d4f to 9b8a733 Compare July 30, 2024 23:51

uhthomas force-pushed the 3873 branch 2 times, most recently from 2580a22 to cb95fa1 Compare July 31, 2024 01:26

uhthomas force-pushed the 3873 branch from cb95fa1 to 7655597 Compare July 31, 2024 01:39

uhthomas mentioned this pull request Jul 31, 2024

Update S3 flush to not have last part greater than defined chunkSize #4327

Closed

uhthomas force-pushed the 3873 branch 2 times, most recently from 13b95d9 to b1e86de Compare July 31, 2024 02:24

This comment was marked as resolved.

Sign in to view

uhthomas force-pushed the 3873 branch 6 times, most recently from 1732bdc to e8b0e55 Compare July 31, 2024 14:00

milosgajdos requested a review from corhere July 31, 2024 14:05

github-advanced-security bot found potential problems Jul 31, 2024

View reviewed changes

registry/storage/driver/s3-aws/s3.go Fixed Show resolved Hide resolved

milosgajdos reviewed Jul 31, 2024

View reviewed changes

uhthomas force-pushed the 3873 branch 2 times, most recently from 8ea9ba6 to 6993235 Compare July 31, 2024 14:26

This comment was marked as resolved.

Sign in to view

uhthomas commented Aug 4, 2024

View reviewed changes

uhthomas force-pushed the 3873 branch from 4b66028 to d987d46 Compare August 4, 2024 18:15

uhthomas mentioned this pull request Aug 5, 2024

Fix S3 storage driver to support R2 Multipart upload #3940

Closed

uhthomas commented Aug 5, 2024

View reviewed changes

uhthomas mentioned this pull request Aug 6, 2024

chore(registry/storage/driver/s3-aws): refactor writer creation #4429

Open

milosgajdos reviewed Oct 11, 2024

View reviewed changes

milosgajdos reviewed Oct 23, 2024

View reviewed changes

uhthomas force-pushed the 3873 branch 4 times, most recently from de0a06c to 9ed1f97 Compare October 30, 2024 21:32

uhthomas force-pushed the 3873 branch from 9ed1f97 to 5ee5aaa Compare October 30, 2024 21:46

milosgajdos approved these changes Oct 31, 2024

View reviewed changes

milosgajdos requested review from Jamstah and thaJeztah October 31, 2024 23:42

Vad1mo approved these changes Nov 1, 2024

View reviewed changes

milosgajdos requested review from joaodrp and squizzi November 1, 2024 17:11

BrammyS approved these changes Nov 3, 2024

View reviewed changes

Jamstah approved these changes Nov 5, 2024

View reviewed changes

milosgajdos merged commit 099201a into distribution:main Nov 5, 2024
17 checks passed

Vad1mo mentioned this pull request Feb 4, 2025

Multipart upload issues with Cloudflare R2 #3873

Closed

wzshiming added a commit to DaoCloud/crproxy that referenced this pull request Feb 10, 2025

Manual pick

2dc3ee8

- distribution/distribution#4424

wzshiming added a commit to DaoCloud/crproxy that referenced this pull request Feb 10, 2025

Manual pick

c442826

- distribution/distribution#4424

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(registry/storage/driver/s3-aws): use a consistent multipart chunk size #4424

fix(registry/storage/driver/s3-aws): use a consistent multipart chunk size #4424

uhthomas commented Jul 30, 2024

uhthomas commented Jul 30, 2024

uhthomas commented Jul 31, 2024

ianseyer commented Jul 31, 2024 •

edited

Loading

uhthomas commented Jul 31, 2024 •

edited

Loading

uhthomas commented Jul 31, 2024 •

edited

Loading

uhthomas commented Jul 31, 2024 •

edited

Loading

This comment was marked as resolved.

milosgajdos left a comment

milosgajdos Jul 31, 2024

This comment was marked as resolved.

uhthomas Aug 4, 2024 •

edited

Loading

uhthomas Aug 5, 2024

milosgajdos Oct 11, 2024

Vad1mo commented Aug 12, 2024

BrammyS commented Sep 26, 2024

uhthomas commented Oct 8, 2024

Vad1mo commented Oct 11, 2024 •

edited

Loading

milosgajdos left a comment

milosgajdos Oct 11, 2024

Vad1mo Oct 23, 2024

milosgajdos Oct 11, 2024

milosgajdos left a comment •

edited

Loading

milosgajdos left a comment

Jamstah left a comment

uhthomas commented Nov 5, 2024

fix(registry/storage/driver/s3-aws): use a consistent multipart chunk size #4424

fix(registry/storage/driver/s3-aws): use a consistent multipart chunk size #4424

Conversation

uhthomas commented Jul 30, 2024

uhthomas commented Jul 30, 2024

uhthomas commented Jul 31, 2024

ianseyer commented Jul 31, 2024 • edited Loading

uhthomas commented Jul 31, 2024 • edited Loading

uhthomas commented Jul 31, 2024 • edited Loading

uhthomas commented Jul 31, 2024 • edited Loading

This comment was marked as resolved.

milosgajdos left a comment

Choose a reason for hiding this comment

milosgajdos Jul 31, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

uhthomas Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

uhthomas Aug 5, 2024

Choose a reason for hiding this comment

milosgajdos Oct 11, 2024

Choose a reason for hiding this comment

Vad1mo commented Aug 12, 2024

BrammyS commented Sep 26, 2024

uhthomas commented Oct 8, 2024

Vad1mo commented Oct 11, 2024 • edited Loading

milosgajdos left a comment

Choose a reason for hiding this comment

milosgajdos Oct 11, 2024

Choose a reason for hiding this comment

Vad1mo Oct 23, 2024

Choose a reason for hiding this comment

milosgajdos Oct 11, 2024

Choose a reason for hiding this comment

milosgajdos left a comment • edited Loading

Choose a reason for hiding this comment

milosgajdos left a comment

Choose a reason for hiding this comment

Jamstah left a comment

Choose a reason for hiding this comment

uhthomas commented Nov 5, 2024

ianseyer commented Jul 31, 2024 •

edited

Loading

uhthomas commented Jul 31, 2024 •

edited

Loading

uhthomas commented Jul 31, 2024 •

edited

Loading

uhthomas commented Jul 31, 2024 •

edited

Loading

uhthomas Aug 4, 2024 •

edited

Loading

Vad1mo commented Oct 11, 2024 •

edited

Loading

milosgajdos left a comment •

edited

Loading