Add new parameter accelerate to S3 storage driver. #3181
Conversation
|
If anyone interested, we published a docker image with this patch applied on top of the 2.7 branch, and we are using it in production since friday to fix slow downloads speeds when S3 bucket is in a different region than clients pulling. |
registry/storage/driver/s3-aws/s3.go
Outdated
| @@ -427,6 +446,10 @@ func New(params DriverParameters) (*Driver, error) { | |||
| awsConfig.WithEndpoint(params.RegionEndpoint) | |||
| } | |||
|
|
|||
| if params.S3Accelerate { | |||
There was a problem hiding this comment.
Minor comment: just use awsConfig.WithS3UseAccelerate(params.S3Accelerate)? It can be concise.
|
and it's better to revert the change of document as well. https://github.com/docker/docker.github.io/pull/10993/files |
|
@pimuzzo looks like CI is failing because the second commit is missing a DCO sign-off in the commit message https://github.com/docker/distribution/blob/master/CONTRIBUTING.md#sign-your-work |
|
@thaJeztah. Is there an ETA for merging this PR? |
|
@pimuzzo this patch looks great. Would you mind rebasing again, please? |
|
@milosgajdos PR rebased @wy65701436 @thaJeztah Fixed review and signed commits |
pimuzzo
changed the title
Codecov Report
@@ Coverage Diff @@
## main #3181 +/- ##
==========================================
- Coverage 56.12% 55.84% -0.29%
==========================================
Files 105 102 -3
Lines 7431 7222 -209
==========================================
- Hits 4171 4033 -138
+ Misses 2615 2552 -63
+ Partials 645 637 -8
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@pimuzzo can you please update the S3 driver docs https://github.com/docker/docker.github.io/blob/master/registry/storage-drivers/s3.md as the comment on the original PR suggest?
|
@milosgajdos please take a look to https://github.com/docker/docker.github.io/pull/11140/files and leave me some comment on what you would change, it looks good to me |
|
@milosgajdos 🙏🏻 |
|
@pimuzzo can you rebase one more time. This LGTM and I'd like to give this 👍 so we can merge this in. |
|
@pimuzzo can you sign your commits, please? |
|
@milosgajdos I changed my workstation, please give me some time to fix all the things :D |
If s3accelerate is set to true then we turn on S3 Transfer Acceleration via the AWS SDK. It defaults to false since this is an opt-in feature on the S3 bucket. Signed-off-by: Kirat Singh <kirat.singh@wsq.io> Signed-off-by: Simone Locci <simonelocci88@gmail.com>
Signed-off-by: Simone Locci <simonelocci88@gmail.com>
Signed-off-by: Simone Locci <simonelocci88@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #3181 +/- ##
=======================================
Coverage 56.33% 56.33%
=======================================
Files 101 101
Lines 7313 7313
=======================================
Hits 4120 4120
Misses 2536 2536
Partials 657 657 Continue to review full report at Codecov.
|
|
@milosgajdos I did it 👯♂️ |
Rebase of #2166 due to inactivity
Original PR comment:
I've added an optional parameter to enable S3 Transfer acceleration for the S3 storage driver. Thank you very much for considering the change.
If s3accelerate is set to true then we turn on S3 Transfer Acceleration via the AWS SDK. It defaults to false since this is an opt-in feature on the S3 bucket.
Signed-off-by: Kirat Singh kirat.singh@wsq.io