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

Implement Alibaba Cloud OSS gateway support #5103

Merged
merged 4 commits into from Dec 19, 2017
Merged

Conversation

@timonwong
Copy link
Contributor

@timonwong timonwong commented Oct 23, 2017

Motivation and Context

Implement Alibaba Cloud (Aliyun) OSS gateway support.

API Documentation (very similar to AWS S3): https://www.alibabacloud.com/help/doc-detail/31947.htm

How Has This Been Tested?

Manually & Mint

Notes about failed mint tests:

  • Since OSS doesn't support underscores("_") in custom metadata headers, so aws-sdk-php will fail. (Workaround by sed -i 's/Param_1/Param-1/' /mint/run/core/aws-sdk-php/quick-tests.php)
  • minio-dotnet failed during put objects (tons of connect failed errors, may be caused by too many file descriptors)

Besides, in order to get mint tests pass, there is a check in CompleteMultipartUpload() which iterate uploaded parts, and error out if uploadedParts except last part sizing < 5MiB.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch 4 times, most recently from 4961a7f to 0398226 Oct 23, 2017
@timonwong timonwong changed the title WIP: Implement Aliyun OSS gateway support [WIP] Implement Aliyun OSS gateway support Oct 23, 2017
@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch from 0398226 to 181002a Oct 23, 2017
@codecov
Copy link

@codecov codecov bot commented Oct 23, 2017

Codecov Report

Merging #5103 into master will decrease coverage by 0.83%.
The diff coverage is 19.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5103      +/-   ##
==========================================
- Coverage    62.1%   61.26%   -0.84%     
==========================================
  Files         197      199       +2     
  Lines       28329    28900     +571     
==========================================
+ Hits        17593    17705     +112     
- Misses       9407     9863     +456     
- Partials     1329     1332       +3
Impacted Files Coverage Δ
cmd/gateway/oss/gateway-oss-anonymous.go 0% <0%> (ø)
cmd/gateway/oss/gateway-oss.go 20.21% <20.21%> (ø)
cmd/fs-v1-rwpool.go 64.34% <0%> (-2.61%) ⬇️
cmd/fs-v1-background-append.go 79.86% <0%> (+1.34%) ⬆️

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 8c08571...8611d3b. Read the comment docs.

@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch 2 times, most recently from 0df199c to bc6c3a4 Oct 23, 2017
@timonwong timonwong mentioned this pull request Oct 24, 2017
1 of 8 tasks complete
@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch from bc6c3a4 to 2ce9e88 Oct 28, 2017
return ossBackend
}

func (g *OSSGateway) NewGatewayLayer() (GatewayLayer, error) {

This comment has been minimized.

@houndci-bot

houndci-bot Oct 28, 2017

exported method OSSGateway.NewGatewayLayer should have comment or be unexported

host string
}

func (g *OSSGateway) Name() string {

This comment has been minimized.

@houndci-bot

houndci-bot Oct 28, 2017

exported method OSSGateway.Name should have comment or be unexported

startGateway(ctx, &OSSGateway{host})
}

type OSSGateway struct {

This comment has been minimized.

@houndci-bot

houndci-bot Oct 28, 2017

exported type OSSGateway should have comment or be unexported

@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch from 2ce9e88 to 1a3468d Oct 28, 2017
host string
}

// OSSGateway implements Gateway interface.

This comment has been minimized.

@houndci-bot

houndci-bot Oct 28, 2017

comment on exported method OSSGateway.Name should be of the form "Name ..."

@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch from 1a3468d to 6d05cdc Oct 28, 2017
@timonwong timonwong changed the title [WIP] Implement Aliyun OSS gateway support [WIP] Implement Alibaba Cloud OSS gateway support Oct 28, 2017
@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch 5 times, most recently from 5674a21 to 73be360 Oct 28, 2017
@timonwong timonwong changed the title [WIP] Implement Alibaba Cloud OSS gateway support Implement Alibaba Cloud OSS gateway support Oct 28, 2017
@harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Dec 6, 2017

@timonwong i have rebased upon your work timonwong#1 to point to latest master, made relevant changes. Please merge the PR, we can then plan on merging this PR.

timonwong and others added 2 commits Oct 24, 2017
@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch from e5f3661 to 19f2885 Dec 6, 2017
@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Dec 6, 2017

@harshavardhana Thanks :) And besides, I just did a rebase due to CI failure after merging...

@@ -0,0 +1,177 @@
package oss

This comment has been minimized.

@harshavardhana

harshavardhana Dec 6, 2017
Member

can you add license header here @timonwong ? just like other gateways?

This comment has been minimized.

@timonwong

timonwong Dec 6, 2017
Author Contributor

@@ -0,0 +1,847 @@
package oss

This comment has been minimized.

@harshavardhana

harshavardhana Dec 6, 2017
Member

license header.

@harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Dec 12, 2017

@timonwong you can run only specific tests by docker run minio/mint:edge minio-go minio-py mc minio-java aws-sdk-go aws-sdk-ruby

@harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Dec 12, 2017

it is okay that these fail and we can catch up in perhaps future versions, just wanted to see how ready is this support . If things are failing perhaps you can write down which all tests fail we can review and deal with this in case by case basis.

@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Dec 12, 2017

Current condition:

  • aws-sdk-php failed with custom metadata header with underscores (after patching without underscore, it passes)
  • minio-dotnet failed
  • s3cmd Failed in test_put_object, throws "MD5 Sums don't match" warnings (etag mismatch, OSS returns uppercase string, however, md5.hexdigest() in Python returns lowercase string).

All other tests passes.

(1/5) Running minio-go tests ... done in 19 minutes and 23 seconds
(2/5) Running minio-java tests ... done in 43 seconds
(3/5) Running minio-js tests ... done in 1 minutes and 28 seconds
(4/5) Running minio-py tests ... done in 2 minutes and 31 seconds
(5/5) Running s3cmd tests ... FAILED in 10 seconds
{
  "name": "s3cmd",
  "duration": "7431",
  "function": "test_put_object",
  "status": "FAIL",
  "error": "WARNING: MD5 Sums don't match!\nWARNING: Retrying upload of /mint/data/datafile-1-MB\nWARNING: MD5 Sums don't match!\nWARNING: Retrying upload of /mint/data/datafile-1-MB\nWARNING: MD5 Sums don't match!\nWARNING: Retrying upload of /mint/data/datafile-1-MB\nWARNING: MD5 Sums don't match!\nWARNING: Retrying upload of /mint/data/datafile-1-MB\nWARNING: MD5 Sums don't match!\nWARNING: Retrying upload of /mint/data/datafile-1-MB\nWARNING: MD5 Sums don't match!\nWARNING: Too many failures. Giving up on '/mint/data/datafile-1-MB'\nERROR: Upload of '/mint/data/datafile-1-MB' failed too many times (Last reason: )"
}
@harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Dec 12, 2017

For md5 you can simply add -1 at the end here . We do that with all gateways which do not support direct md5sum.

@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch 4 times, most recently from e2565a9 to 99932bd Dec 12, 2017
@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Dec 12, 2017

For md5 you can simply add -1 at the end here . We do that with all gateways which do not support direct md5sum.

Actually OSS did have MD5 Etag, but it's upper-cased.
In s3cmd, it calculate md5 hash of uploaded content itself (https://github.com/s3tools/s3cmd/blob/6d8e621738f4b37a4bbc0b5b28d1c0717264f7d8/S3/S3.py#L1548).

So I should just use minio.ToS3ETag?

@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch from 99932bd to 29086b3 Dec 12, 2017
@harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Dec 12, 2017

Yes @timonwong just use that .it should be fine

@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Dec 12, 2017

Ok, so now s3cmd passes :)

@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch from 29086b3 to e150b2a Dec 12, 2017
@harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Dec 12, 2017

Ok, so now s3cmd passes :)

So other than aws-sdk-php all tests pass?? @timonwong

@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Dec 12, 2017

So other than aws-sdk-php all tests pass?? @timonwong

@harshavardhana And minio-dotnet failed due to tons of "Couldn't connect to server" errors.

@harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Dec 12, 2017

@harshavardhana And minio-dotnet failed due to tons of "Couldn't connect to server" errors.

That is nice to know @timonwong LGTM we can take this change in.

@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Dec 13, 2017

Please just wait for a moment, I need to investigate some weird behavior in CompleteMultipartUpload

@timonwong timonwong force-pushed the timonwong:add-aliyun-oss branch from e150b2a to 8611d3b Dec 13, 2017
@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Dec 13, 2017

Mint tests verified:

  • aws-sdk-go
  • aws-sdk-php
  • aws-sdk-ruby
docker run --rm -e SERVER_ENDPOINT=docker.for.mac.localhost:9000 -e ACCESS_KEY=$MINIO_ACCESS_KEY -e SECRET_KEY=$MINIO_SECRET_KEY -e ENABLE_HTTPS=0 mint2 aws-sdk-go aws-sdk-php aws-sdk-ruby
Running with
SERVER_ENDPOINT: docker.for.mac.localhost:9000
ACCESS_KEY:      ***REDACTED***
SECRET_KEY:      ***REDACTED***
ENABLE_HTTPS:    0
SERVER_REGION:   us-east-1
MINT_DATA_DIR:   /mint/data
MINT_MODE:       core

To get logs, run 'sudo docker cp f16f3755d3d3:/mint/log /tmp/mint-logs'
(1/3) Running aws-sdk-go tests ... done in 5 seconds
(2/3) Running aws-sdk-php tests ... done in 1 minutes and 9 seconds
(3/3) Running aws-sdk-ruby tests ... done in 48 seconds

All tests ran successfully
  • minio-java
  • minio-js
  • minio-py
docker run --rm -e SERVER_ENDPOINT=docker.for.mac.localhost:9000 -e ACCESS_KEY=$MINIO_ACCESS_KEY -e SECRET_KEY=$MINIO_SECRET_KEY -e ENABLE_HTTPS=0 mint2 minio-java minio-js minio-py
Running with
SERVER_ENDPOINT: docker.for.mac.localhost:9000
ACCESS_KEY:      ***REDACTED***
SECRET_KEY:      ***REDACTED***
ENABLE_HTTPS:    0
SERVER_REGION:   us-east-1
MINT_DATA_DIR:   /mint/data
MINT_MODE:       core

To get logs, run 'sudo docker cp 0552eda8334a:/mint/log /tmp/mint-logs'
(1/3) Running minio-java tests ... done in 56 seconds
(2/3) Running minio-js tests ... done in 2 minutes and 16 seconds
(3/3) Running minio-py tests ... done in 3 minutes and 52 seconds

All tests ran successfully
  • s3cmd
docker run --rm -e SERVER_ENDPOINT=docker.for.mac.localhost:9000 -e ACCESS_KEY=$MINIO_ACCESS_KEY -e SECRET_KEY=$MINIO_SECRET_KEY -e ENABLE_HTTPS=0 mint2 s3cmd
Running with
SERVER_ENDPOINT: docker.for.mac.localhost:9000
ACCESS_KEY:      ***REDACTED***
SECRET_KEY:      ***REDACTED***
ENABLE_HTTPS:    0
SERVER_REGION:   us-east-1
MINT_DATA_DIR:   /mint/data
MINT_MODE:       core

To get logs, run 'sudo docker cp 28a6e865a68b:/mint/log /tmp/mint-logs'
(1/1) Running s3cmd tests ... done in 7 minutes and 33 seconds

All tests ran successfully
  • awscli
  • mc
docker run --rm -e SERVER_ENDPOINT=docker.for.mac.localhost:9000 -e ACCESS_KEY=$MINIO_ACCESS_KEY -e SECRET_KEY=$MINIO_SECRET_KEY -e ENABLE_HTTPS=0 mint2 awscli mc
Running with
SERVER_ENDPOINT: docker.for.mac.localhost:9000
ACCESS_KEY:      ***REDACTED***
SECRET_KEY:      ***REDACTED***
ENABLE_HTTPS:    0
SERVER_REGION:   us-east-1
MINT_DATA_DIR:   /mint/data
MINT_MODE:       core

To get logs, run 'sudo docker cp fbe4a313d265:/mint/log /tmp/mint-logs'
(1/2) Running awscli tests ... done in 4 minutes and 49 seconds
(2/2) Running mc tests ... done in 11 minutes and 3 seconds

All tests ran successfully
Copy link
Member

@krishnasrinivas krishnasrinivas left a comment

Did not test it as I don't have alibaba account details. I had reviewed the code which looks good. If the mint is passing then it should be good to go.

@nitisht nitisht merged commit 84fc78d into minio:master Dec 19, 2017
2 of 4 checks passed
2 of 4 checks passed
codecov/patch 19.78% of diff hit (target 62.1%)
Details
codecov/project 61.26% (-0.84%) compared to 8c08571
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@timonwong timonwong deleted the timonwong:add-aliyun-oss branch Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.