objstore : implement Aliyun OSS #1573

wujinhu · 2019-09-26T06:55:20Z

Open this PR as discussed in #1234 ， will update soon.

wujinhu · 2019-09-26T08:17:43Z

@bwplotka It seems a little difficult to make commits signed off, there is a lot of conflicts when I run the rebase command. I think it will be easier if I revert all those commits and submit a new one. And I will reference #1234 in the new one. What do you think of this?

woodliu · 2019-10-14T06:54:18Z

@wujinhu What is the status now? This guy shaulboozhiao has deleted the code he had...
We are going to use Ali oss to support thanos, but can't find the supported code.

wujinhu · 2019-10-14T09:27:58Z

@woodliu Sorry for the delay, will update this week.

woodliu · 2019-10-14T09:29:25Z

@wujinhu What is the status now? This guy shaulboozhiao has deleted the code he had...
We are going to use Ali oss to support thanos, but can't find the supported code.

I have try and merge the history code from your repo, it works, there's not much files changed, it seems that merge manually is a better way.

@woodliu Sorry for the delay, will update this week.

I have try and merge the history code from your repo, it works, there's not much files changed, it seems that merge manually is a better way.

bwplotka · 2019-10-17T10:35:16Z

Any luck? (: Feel free to squash the original commits into one.

wujinhu · 2019-10-18T06:14:38Z

revert commits and apply patch!

wujinhu · 2019-10-18T06:54:21Z

Sth is wrong with the patch, I am fixing!

wujinhu · 2019-10-18T08:53:38Z

@bwplotka @jojohappy Please help review this PR, thanks!😆

pkg/objstore/oss/oss.go

povilasv · 2019-10-18T08:59:51Z

pkg/objstore/oss/oss.go

+	bucket *alioss.Bucket
+}
+
+func NewTestBucket(t testing.TB) (objstore.Bucket, func(), error) {


I think this code should be in *_test.go files

Nope, this is the idiom for now. This is because _test.go files even exported methods are not exported between packages.

On the other hand this method is really the same across objectstores so we might want to refactor it to one (:

povilasv · 2019-10-18T09:03:54Z

pkg/objstore/oss/oss.go

+	return NewTestBucketFromConfig(t, c, false)
+}
+
+func calculateChunks(name string, r io.Reader) (int, int64, error) {


I think we shouldn't do this type castin, let's just take an interface which has io.Reader & Size() methods instead of supporting only os.File & strings.Reader types.

I am a little ambiguous about this. We take S3 as an example, could you please give me comments in more details. Thanks!

I think we can do this in another PR. As this would require quite a lot of refactoring.

If we change the interface in https://github.com/thanos-io/thanos/blob/master/pkg/objstore/objstore.go#L26
from:

Upload(ctx context.Context, name string, r io.Reader) error

to

type ReadSizer interface{ io.Reader Size() int64 } Upload(ctx context.Context, name string, r ReadSizer) error

We would avoid this hack entirely.

@bwplotka what do you think?

I guess there is no way to have this working without knowing the size beforehand? 🤔

We could change interface, but the S3 example is bad - we should never rely on size as it block streaming case. However the truth is there is no case we need streaming currently for write (:

The other option is to actually read all the bytes into memory and then we would know the size, which sucks for memory requirements.

I would prefer to have ReadSizer, it actually helps upload case, as the uploader client can split the object into chunks, make it parallel, etc.

Well that's the thing, for streaming purposes you buffer let's say 128KB then if you buffer more you know you need multipart. While you traverse the file you stream parts until end of the reader (: I think that's what GCS is doing for example. Essentially you can implement this without knowing size beforehand.

In fact this will be required if we would like to have this: #1673

Yeah you are right. Actually in this case we shouldn't need a Size() method, we can just do what you said and upload parts the way you described.

Let's move this discussion here: #678

pkg/objstore/oss/oss.go

povilasv · 2019-10-18T09:10:22Z

pkg/objstore/oss/oss.go

+	return nil
+}
+
+func NewTestBucketFromConfig(t testing.TB, c Config, reuseBucket bool) (objstore.Bucket, func(), error) {


I think this code should be in *_test.go files

@povilasv could we refactor obj testing in another PR?

pkg/objstore/oss/oss.go

Signed-off-by: wujinhu <[email protected]>

wujinhu · 2019-10-21T06:39:56Z

@povilasv @jojohappy Thanks for your comments, they are very helpful, and I updated the PR.

bwplotka

Nice, generally LGTM, some minor comments only. Very clean code, thanks!

It's sad OSS SDK does not have multipart logic without file size. It indeed gives another reason to change our interface and add size to it. Not for this PR for sure though.

Can you confirm the acceptance e2e is successfully working with this provider? (

thanos/pkg/objstore/objtesting/acceptance_e2e_test.go

Line 18 in 5d107d1

func TestObjStore_AcceptanceTest_e2e(t *testing.T) {

)

bwplotka · 2019-10-22T10:38:34Z

pkg/objstore/oss/oss.go

+	bucket *alioss.Bucket
+}
+
+func NewTestBucket(t testing.TB) (objstore.Bucket, func(), error) {


Nope, this is the idiom for now. This is because _test.go files even exported methods are not exported between packages.

On the other hand this method is really the same across objectstores so we might want to refactor it to one (:

Makefile

bwplotka · 2019-10-22T10:41:27Z

docs/storage.md

@@ -60,6 +60,7 @@ Current object storage client implementations:
 | [Azure Storage Account](./storage.md#azure) | Stable  (production usage) | yes       | @vglafirov   |
 | [OpenStack Swift](./storage.md#openstack-swift)      | Beta  (working PoCs, testing usage)               | no        | @sudhi-vm   |
 | [Tencent COS](./storage.md#tencent-cos)          | Beta  (testing usage)                   | no        | @jojohappy          |
+| [AliYun OSS](./storage.md#aliyun-oss)           | Beta  (testing usage)                   | no        | @shaulboozhiao,@wujinhu      |


bwplotka · 2019-10-22T10:41:46Z

docs/storage.md

@@ -336,3 +337,20 @@ config:
 ```

 Set the flags `--objstore.config-file` to reference to the configuration file.
+
+##  AliYun OSS Configuration
+In order to use AliYun OSS object storage, you should first create a bucket with proper Storage Class , ACLs and get the access key on the AliYun cloud. Go to [https://www.alibabacloud.com/product/oss](https://www.alibabacloud.com/product/oss) for more detail.


What proper Storage Class means?

It is the same meaning with S3 https://aws.amazon.com/s3/storage-classes/?nc1=h_ls
It defines the storage type of data.

pkg/objstore/oss/oss.go

bwplotka · 2019-10-22T10:49:04Z

pkg/objstore/oss/oss.go

+	return NewTestBucketFromConfig(t, c, false)
+}
+
+func calculateChunks(name string, r io.Reader) (int, int64, error) {


Yea ok it looks like for multipart need to do on our own: https://github.com/aliyun/aliyun-oss-go-sdk/blob/d0a2d03230103776946b757cbc05aeecb523d9c9/oss/multipart_test.go#L180

pkg/objstore/oss/oss.go

Signed-off-by: wujinhu <[email protected]>

wujinhu · 2019-10-23T02:36:13Z

=== RUN TestObjStore_AcceptanceTest_e2e
--- PASS: TestObjStore_AcceptanceTest_e2e (1.68s)
=== RUN TestObjStore_AcceptanceTest_e2e/inmem
--- PASS: TestObjStore_AcceptanceTest_e2e/inmem (0.00s)
foreach.go:49: THANOS_SKIP_GCS_TESTS envvar present. Skipping test against GCS.
foreach.go:70: THANOS_SKIP_S3_AWS_TESTS envvar present. Skipping test against S3 AWS.
foreach.go:86: THANOS_SKIP_AZURE_TESTS envvar present. Skipping test against Azure.
foreach.go:102: THANOS_SKIP_SWIFT_TESTS envvar present. Skipping test against swift.
foreach.go:118: THANOS_SKIP_TENCENT_COS_TESTS envvar present. Skipping test against Tencent COS.
=== RUN TestObjStore_AcceptanceTest_e2e/AliYun_oss
--- PASS: TestObjStore_AcceptanceTest_e2e/AliYun_oss (0.63s)
PASS

Process finished with exit code 0

wujinhu · 2019-10-23T02:39:44Z

=== RUN TestBucketStore_e2e
--- PASS: TestBucketStore_e2e (26.03s)
=== RUN TestBucketStore_e2e/inmem
--- PASS: TestBucketStore_e2e/inmem (10.17s)
bucket_e2e_test.go:398: Test with no index cache
bucket_e2e_test.go:372: Run 0
bucket_e2e_test.go:372: Run 1
bucket_e2e_test.go:372: Run 2
bucket_e2e_test.go:372: Run 3
bucket_e2e_test.go:372: Run 4
bucket_e2e_test.go:372: Run 5
bucket_e2e_test.go:372: Run 6
bucket_e2e_test.go:372: Run 7
bucket_e2e_test.go:372: Run 8
bucket_e2e_test.go:372: Run 9
bucket_e2e_test.go:372: Run 10
bucket_e2e_test.go:402: Test with large, sufficient index cache
bucket_e2e_test.go:372: Run 0
bucket_e2e_test.go:372: Run 1
bucket_e2e_test.go:372: Run 2
bucket_e2e_test.go:372: Run 3
bucket_e2e_test.go:372: Run 4
bucket_e2e_test.go:372: Run 5
bucket_e2e_test.go:372: Run 6
bucket_e2e_test.go:372: Run 7
bucket_e2e_test.go:372: Run 8
bucket_e2e_test.go:372: Run 9
bucket_e2e_test.go:372: Run 10
bucket_e2e_test.go:411: Test with small index cache
bucket_e2e_test.go:372: Run 0
bucket_e2e_test.go:372: Run 1
bucket_e2e_test.go:372: Run 2
bucket_e2e_test.go:372: Run 3
bucket_e2e_test.go:372: Run 4
bucket_e2e_test.go:372: Run 5
bucket_e2e_test.go:372: Run 6
bucket_e2e_test.go:372: Run 7
bucket_e2e_test.go:372: Run 8
bucket_e2e_test.go:372: Run 9
bucket_e2e_test.go:372: Run 10
foreach.go:49: THANOS_SKIP_GCS_TESTS envvar present. Skipping test against GCS.
foreach.go:70: THANOS_SKIP_S3_AWS_TESTS envvar present. Skipping test against S3 AWS.
foreach.go:86: THANOS_SKIP_AZURE_TESTS envvar present. Skipping test against Azure.
foreach.go:102: THANOS_SKIP_SWIFT_TESTS envvar present. Skipping test against swift.
foreach.go:118: THANOS_SKIP_TENCENT_COS_TESTS envvar present. Skipping test against Tencent COS.
=== RUN TestBucketStore_e2e/AliYun_oss
--- PASS: TestBucketStore_e2e/AliYun_oss (14.41s)
bucket_e2e_test.go:398: Test with no index cache
bucket_e2e_test.go:372: Run 0
bucket_e2e_test.go:372: Run 1
bucket_e2e_test.go:372: Run 2
bucket_e2e_test.go:372: Run 3
bucket_e2e_test.go:372: Run 4
bucket_e2e_test.go:372: Run 5
bucket_e2e_test.go:372: Run 6
bucket_e2e_test.go:372: Run 7
bucket_e2e_test.go:372: Run 8
bucket_e2e_test.go:372: Run 9
bucket_e2e_test.go:372: Run 10
bucket_e2e_test.go:402: Test with large, sufficient index cache
bucket_e2e_test.go:372: Run 0
bucket_e2e_test.go:372: Run 1
bucket_e2e_test.go:372: Run 2
bucket_e2e_test.go:372: Run 3
bucket_e2e_test.go:372: Run 4
bucket_e2e_test.go:372: Run 5
bucket_e2e_test.go:372: Run 6
bucket_e2e_test.go:372: Run 7
bucket_e2e_test.go:372: Run 8
bucket_e2e_test.go:372: Run 9
bucket_e2e_test.go:372: Run 10
bucket_e2e_test.go:411: Test with small index cache
bucket_e2e_test.go:372: Run 0
bucket_e2e_test.go:372: Run 1
bucket_e2e_test.go:372: Run 2
bucket_e2e_test.go:372: Run 3
bucket_e2e_test.go:372: Run 4
bucket_e2e_test.go:372: Run 5
bucket_e2e_test.go:372: Run 6
bucket_e2e_test.go:372: Run 7
bucket_e2e_test.go:372: Run 8
bucket_e2e_test.go:372: Run 9
bucket_e2e_test.go:372: Run 10
=== RUN TestBucketStore_ManyParts_e2e
--- PASS: TestBucketStore_ManyParts_e2e (17.94s)
=== RUN TestBucketStore_ManyParts_e2e/inmem
--- PASS: TestBucketStore_ManyParts_e2e/inmem (6.57s)
bucket_e2e_test.go:372: Run 0
bucket_e2e_test.go:372: Run 1
bucket_e2e_test.go:372: Run 2
bucket_e2e_test.go:372: Run 3
bucket_e2e_test.go:372: Run 4
bucket_e2e_test.go:372: Run 5
bucket_e2e_test.go:372: Run 6
bucket_e2e_test.go:372: Run 7
bucket_e2e_test.go:372: Run 8
bucket_e2e_test.go:372: Run 9
bucket_e2e_test.go:372: Run 10
foreach.go:49: THANOS_SKIP_GCS_TESTS envvar present. Skipping test against GCS.
foreach.go:70: THANOS_SKIP_S3_AWS_TESTS envvar present. Skipping test against S3 AWS.
foreach.go:86: THANOS_SKIP_AZURE_TESTS envvar present. Skipping test against Azure.
foreach.go:102: THANOS_SKIP_SWIFT_TESTS envvar present. Skipping test against swift.
foreach.go:118: THANOS_SKIP_TENCENT_COS_TESTS envvar present. Skipping test against Tencent COS.
=== RUN TestBucketStore_ManyParts_e2e/AliYun_oss
--- PASS: TestBucketStore_ManyParts_e2e/AliYun_oss (9.57s)
bucket_e2e_test.go:372: Run 0
bucket_e2e_test.go:372: Run 1
bucket_e2e_test.go:372: Run 2
bucket_e2e_test.go:372: Run 3
bucket_e2e_test.go:372: Run 4
bucket_e2e_test.go:372: Run 5
bucket_e2e_test.go:372: Run 6
bucket_e2e_test.go:372: Run 7
bucket_e2e_test.go:372: Run 8
bucket_e2e_test.go:372: Run 9
bucket_e2e_test.go:372: Run 10
=== RUN TestBucketStore_TimePartitioning_e2e
--- PASS: TestBucketStore_TimePartitioning_e2e (6.19s)
bucket_e2e_test.go:520: Run 0
PASS

Process finished with exit code 0

    bucket_e2e_test.go:372: Run  2
    bucket_e2e_test.go:372: Run  3
    bucket_e2e_test.go:372: Run  4
    bucket_e2e_test.go:372: Run  5
    bucket_e2e_test.go:372: Run  6
    bucket_e2e_test.go:372: Run  7
    bucket_e2e_test.go:372: Run  8
    bucket_e2e_test.go:372: Run  9
    bucket_e2e_test.go:372: Run  10

=== RUN TestBucketStore_TimePartitioning_e2e
--- PASS: TestBucketStore_TimePartitioning_e2e (6.19s)
bucket_e2e_test.go:520: Run 0
PASS

Process finished with exit code 0

wujinhu · 2019-10-23T02:40:44Z

@bwplotka acceptance_e2e_test.go and bucket_e2e_test.go both are ok.

jojohappy

LGTM! We can wait @povilasv to review the next round.

povilasv · 2019-10-23T08:28:16Z

pkg/objstore/oss/oss.go

+		return nil, err
+	}
+
+	if _, err := resp.Read(nil); err != nil {


Why are we reading nil bytes here?

You are right, I ran oss and s3 tests and there is no need here because oss sdk is not same with s3 sdk.

// NotFoundObject error is revealed only after first Read. This does the initial GetRequest. Prefetch this here // for convenience. if _, err := r.Read(nil); err != nil { runutil.CloseWithLogOnErr(b.logger, r, "s3 get range obj close") // First GET Object request error. return nil, err }

povilasv

One last comment other than that LGTM 👍

Signed-off-by: wujinhu <[email protected]>

wujinhu · 2019-10-23T09:28:32Z

@bwplotka @povilasv @jojohappy I think each comment has been addressed. 😆

bwplotka

LGTM! Nice work, thanks!

bwplotka · 2019-10-24T10:56:03Z

pkg/objstore/oss/oss.go

+	return NewTestBucketFromConfig(t, c, false)
+}
+
+func calculateChunks(name string, r io.Reader) (int, int64, error) {


Let's move this discussion here: #678

* add oss support Signed-off-by: wujinhu <[email protected]> * fix docs Signed-off-by: wujinhu <[email protected]> * fix Makefile Signed-off-by: wujinhu <[email protected]> * review comments Signed-off-by: wujinhu <[email protected]> * fix style Signed-off-by: wujinhu <[email protected]> * review comments Signed-off-by: wujinhu <[email protected]> * review comments Signed-off-by: wujinhu <[email protected]> * review comments Signed-off-by: wujinhu <[email protected]> * review comments Signed-off-by: wujinhu <[email protected]> Signed-off-by: Giedrius Statkevičius <[email protected]>

wujinhu force-pushed the master branch from ea7b497 to d67a69d Compare September 26, 2019 08:07

wujinhu closed this Oct 18, 2019

wujinhu force-pushed the master branch from ea7b497 to b516793 Compare October 18, 2019 03:45

wujinhu reopened this Oct 18, 2019

jojohappy mentioned this pull request Oct 18, 2019

Will thanos support aliyun oss (similar like S3) #429

Closed

wujinhu force-pushed the master branch 2 times, most recently from d39ca1c to 406cb76 Compare October 18, 2019 08:35

povilasv requested changes Oct 18, 2019

View reviewed changes

jojohappy reviewed Oct 18, 2019

View reviewed changes

pkg/objstore/oss/oss.go Outdated Show resolved Hide resolved

wujinhu added 3 commits October 21, 2019 10:29

add oss support

f7b22c7

Signed-off-by: wujinhu <[email protected]>

fix docs

e7a5d5d

Signed-off-by: wujinhu <[email protected]>

fix Makefile

79b43df

Signed-off-by: wujinhu <[email protected]>

wujinhu force-pushed the master branch from bca10d8 to 79b43df Compare October 21, 2019 02:35

wujinhu added 3 commits October 21, 2019 10:49

review comments

d3f7eae

Signed-off-by: wujinhu <[email protected]>

fix style

1cb9cb1

Signed-off-by: wujinhu <[email protected]>

review comments

8f818df

Signed-off-by: wujinhu <[email protected]>

bwplotka approved these changes Oct 22, 2019

View reviewed changes

wujinhu added 2 commits October 23, 2019 10:16

review comments

21f758f

Signed-off-by: wujinhu <[email protected]>

review comments

a0cdef1

Signed-off-by: wujinhu <[email protected]>

jojohappy approved these changes Oct 23, 2019

View reviewed changes

povilasv reviewed Oct 23, 2019

View reviewed changes

povilasv approved these changes Oct 23, 2019

View reviewed changes

review comments

96620b9

Signed-off-by: wujinhu <[email protected]>

bwplotka approved these changes Oct 24, 2019

View reviewed changes

bwplotka merged commit 66b3d21 into thanos-io:master Oct 24, 2019

objstore : implement Aliyun OSS #1573

objstore : implement Aliyun OSS #1573

Conversation

wujinhu commented Sep 26, 2019

wujinhu commented Sep 26, 2019

woodliu commented Oct 14, 2019

wujinhu commented Oct 14, 2019

woodliu commented Oct 14, 2019

bwplotka commented Oct 17, 2019

wujinhu commented Oct 18, 2019

wujinhu commented Oct 18, 2019

wujinhu commented Oct 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wujinhu commented Oct 21, 2019

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wujinhu commented Oct 23, 2019

wujinhu commented Oct 23, 2019

wujinhu commented Oct 23, 2019

jojohappy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

povilasv left a comment

Choose a reason for hiding this comment

wujinhu commented Oct 23, 2019

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment