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

closes #2224 re-vendor the latest Azure Storage SDK for better performance #2247

Merged
merged 1 commit into from Apr 27, 2017
Merged

closes #2224 re-vendor the latest Azure Storage SDK for better performance #2247

merged 1 commit into from Apr 27, 2017

Conversation

yuwaMSFT2
Copy link
Contributor

closes #2224 re-vendor the latest Azure Storage SDK for better performance

Signed-off-by: Yu Wang yuwa@microsoft.com

…rmance

Signed-off-by: Yu Wang <yuwa@microsoft.com>
@codecov
Copy link

codecov bot commented Apr 14, 2017

Codecov Report

Merging #2247 into master will decrease coverage by 10.07%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2247       +/-   ##
===========================================
- Coverage   61.61%   51.53%   -10.08%     
===========================================
  Files         125      125               
  Lines       11395    11398        +3     
===========================================
- Hits         7021     5874     -1147     
- Misses       3489     4779     +1290     
+ Partials      885      745      -140
Impacted Files Coverage Δ
registry/storage/driver/azure/azure.go 0.78% <0%> (-0.01%) ⬇️
registry/storage/driver/gcs/gcs.go 0.4% <0%> (-68.94%) ⬇️
registry/storage/driver/oss/oss.go 0.56% <0%> (-57.23%) ⬇️
registry/storage/driver/s3-aws/s3.go 4.59% <0%> (-56.33%) ⬇️
registry/storage/driver/s3-goamz/s3.go 0.5% <0%> (-50.88%) ⬇️
registry/client/transport/transport.go 69.69% <0%> (-9.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3e06c6...ac05d14. Read the comment docs.

@yuwaMSFT2
Copy link
Contributor Author

@stevvooe Could you please take a look? It's just re-vendor latest Azure GO SDK. Thanks!

@stevvooe
Copy link
Collaborator

LGTM

@dmcgowan PTAL

@dmp42 What do we need to do for testing here?

@@ -86,7 +86,8 @@ func New(accountName, accountKey, container, realm string) (*Driver, error) {
blobClient := api.GetBlobService()

// Create registry container
if _, err = blobClient.CreateContainerIfNotExists(container, azure.ContainerAccessTypePrivate); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this azure.ContainerAccessTypePrivate used before and no longer used? Is it deprecated now, is it the default, or was it just not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the default. The header is optional when create container; without the header set, the container will be private. The new SDK doesn't provide the option to set the header. To change the permission, need to call Container.SetPermissions.

@dmcgowan dmcgowan added this to the Registry/2.7 milestone Apr 24, 2017
@dmcgowan
Copy link
Collaborator

Looks like we currently don't have an Azure configuration for testing on our CI.
We would need...
AZURE_STORAGE_ACCOUNT_NAME
AZURE_STORAGE_ACCOUNT_KEY
AZURE_STORAGE_CONTAINER
AZURE_STORAGE_REALM

@dmp42 if we have an account somewhere to use I will update our CI

@dmcgowan
Copy link
Collaborator

LGTM

@yuwaMSFT2
Copy link
Contributor Author

@stevvooe @dmcgowan @dmp42
I ran the driver test manually. There was only one failure because that current Azure driver doesn't support PutContent of more than 64MB. This was an old change:
#2088

Since it's not related to this re-vendor change, could you please help to merge?

_FAIL: /home/azureuser/oss/docker/src/github.com/docker/distribution/registry/storage/driver/testsuites/testsuites.go:852: DriverSuite.TestConcurrentStreamReads

/home/azureuser/oss/docker/src/github.com/docker/distribution/registry/storage/driver/testsuites/testsuites.go:866:
c.Assert(err, check.IsNil)
... value driver.Error = driver.Error{DriverName:"azure", Enclosed:(*errors.errorString)(0xc42b09ecb0)} ("azure: uploading 134217728 bytes with PutContent is not supported; limit: 67108864 bytes")

OOPS: 33 passed, 1 FAILED
--- FAIL: Test (689.32s)_

@dmcgowan dmcgowan merged commit 1d78247 into distribution:master Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

re-vendor the latest Azure Storage SDK for better performance
4 participants