objstore : implement Aliyun OSS #1234

ghost · 2019-06-07T15:01:57Z

[] CHANGELOG entry if change is relevant to the end user.

My company infra is worked with Aliyun Cloud , the cold-monitoring data have to be shipped to Aliyun OSS.This PR works to implement it. The code I submit can upload prometheus data to aliyun oss , it also works well to go with thanos bucket ls/verify/inspect although I only deploy it in test env , because I need to make thanos working with the whole framework ,like service discovery & load balance .I will work on this to make aliyun oss objstore work well . If you want something such as doc, I will do it later .

In order to make this PR, I pass over the FOREACH test , the print seem to be OK . although along with this test , i jump over the "bucket creating & bucket delete" testing , I will do it next week .

ps. The object-config.file format like
object.config-file format
type: OSS
config:
bucket: "XXXX"
endpoint: "XXXX"
secret_key: "XXXX"
secret_id: "XXXX"

Changes

pull-d67a69d & pull-4abb631 is the first try, they work for implement aliyun oss objstore

pull-f973be4 can be ignored , I was stupid to push my local changed compiling files go.mod & go.sum to repository , so i have to restore them .

pull-453829 & pull-86c5548 is uploading some images , IGNORE THEM ALSO

Verification

aliyun oss
https://github.com/shaulboozhiao/thanos/blob/master/docs/img/thanos.jpg

FOREACH test result
[thanos@XXX objtesting]$ go test acceptance_e2e_test.go foreach.go
ok command-line-arguments 0.160s

and aliyun oss page

foreach test
https://github.com/shaulboozhiao/thanos/blob/master/docs/img/foreachtest.jpg

wujinhu · 2019-06-10T00:39:21Z

@shaulboozhiao I have opened another PR to implement this, 1238, and we will maintain this.

wujinhu · 2019-06-10T07:02:11Z

sorry，i did not know you have worked on this before i created this pr

jojohappy

LGTM, Thanks! Some suggestions.

Please add this into CHANGELOG. It is a major changes.

Don't forget adding THANOS_SKIP_ALIYUN_OSS_TESTS=true to .circleci/config.yml to skip oss storage testing, at this moment your ci has been failed.

pkg/objstore/oss/oss.go

jojohappy · 2019-06-10T07:24:28Z

@shaulboozhiao Please keep a calm, everyone will respect for your work. You submit the PR at first, so we can close another one. And we also can talk with each other offline in slack or wechat.

Anyway, your PR is very nice! Good job.

domgreen · 2019-06-10T09:42:36Z

@shaulboozhiao can I please remind you that as a project Thanos follows a code of conduct please be respectful to everyone in the community.

If there are any concerns please deal with them in a way you would like to be treated yourself or reach out to any of the maintainers if you have an issue.

ghost · 2019-06-10T14:27:27Z

@domgreen Thans bros, It is my fault to be so childish, I will keep in mind, thank you for your advice .

ghost · 2019-06-10T14:38:57Z

@wujinhu I am so sorry to you , Cause I am so childish , so mean to you . I must to say , please @wujinhu dedicate all your time to aliyun product development , It is awesome cloud infra. ~~ I am so so sorry for my words ~~

debug makefile

add docs dependency

wujinhu · 2019-06-11T12:32:35Z

@shaulboozhiao Never mind. I think we share the same goal. We can make this provider better together. You are doing a great job, I will take a review later, thanks. :)

debug make test

ghost · 2019-06-11T13:11:02Z

@jojohappy @bwplotka @povilasv While runing circleci test , it reports Docs have discrepancies, do 'make docs' and commit changes: I do not know what the problem is , and I do not want to crack Makefile , so I edit .circleci/config.yml and add make docs before make check-docs , is it all right ? or shauld i revert this edits ?

jojohappy · 2019-06-11T13:45:24Z

Docs have discrepancies, do 'make docs' and commit changes

@shaulboozhiao You should run make docs locally, as you said, it would generate the new documents, then commit and push changes to your remote branch.

We should ensure that the documents are the newest in our master branch.

wujinhu · 2019-07-09T10:57:59Z

pkg/objstore/oss/oss.go

+				part, err := uploadEveryPart(lastslice, chunksnum+1)
+				if err != nil {
+					if err := b.bucket.AbortMultipartUpload(init); err != nil {
+						return errors.Wrap(err, "multi-part upload ends with aborting")


Duplicated AbortMultipartUpload here.

No,It is not duplicated , both norm chunk and the last part should abort muliti-part uploading because of failure.

jojohappy

Thanks! More nit picks.

pkg/objstore/oss/oss.go

jojohappy · 2019-07-10T02:14:29Z

pkg/objstore/oss/oss.go

+					if err := b.bucket.AbortMultipartUpload(init); err != nil {
+						return errors.Wrap(err, "multi-part upload ends with aborting")
+					}
+					return errors.Wrap(err, "multi-part upload last chunk failed")


Suggested change

return errors.Wrap(err, "multi-part upload last chunk failed")

return errors.Wrap(err, "failed to upload the last chunk")

jojohappy · 2019-07-10T02:14:54Z

pkg/objstore/oss/oss.go

+		{
+			init, err := b.bucket.InitiateMultipartUpload(name)
+			if err != nil {
+				return errors.Wrap(err, "initiate multi-part upload failed")


Suggested change

return errors.Wrap(err, "initiate multi-part upload failed")

return errors.Wrap(err, "failed to initiate multi-part upload")

jojohappy · 2019-07-10T02:15:19Z

pkg/objstore/oss/oss.go

+		return errors.New("can not guess the size of io.Reader")
+	case 0:
+		if err := b.bucket.PutObject(name, ncloser); err != nil {
+			return errors.Wrap(err, "upload oss object")


Suggested change

return errors.Wrap(err, "upload oss object")

return errors.Wrap(err, "failed to upload object")

jojohappy · 2019-07-10T02:18:50Z

pkg/objstore/oss/oss.go

+			}
+			_, err = b.bucket.CompleteMultipartUpload(init, parts)
+			if err != nil {
+				return errors.Wrap(err, "multi-part upload failed")


Suggested change

return errors.Wrap(err, "multi-part upload failed")

return errors.Wrap(err, "failed to set multi-part upload completive")

jojohappy · 2019-07-19T08:44:18Z

@shaulboozhiao Any update for this?

ghost · 2019-07-23T01:44:16Z

@jojohappy Sorry , I will update it later ~~

ghost · 2019-07-27T11:40:02Z

@wujinhu @jojohappy Code is updated

jojohappy

LGTM! Thanks!

But it seems that you changed some import code, could you tell us what format tool you use?

jojohappy · 2019-07-29T02:19:50Z

pkg/objstore/client/factory.go

@@ -13,6 +13,9 @@ import (
 	"github.com/thanos-io/thanos/pkg/objstore/azure"
 	"github.com/thanos-io/thanos/pkg/objstore/cos"
 	"github.com/thanos-io/thanos/pkg/objstore/gcs"
+


Remove blank line.

jojohappy · 2019-07-29T02:19:57Z

pkg/objstore/client/factory.go

@@ -13,6 +13,9 @@ import (
 	"github.com/thanos-io/thanos/pkg/objstore/azure"
 	"github.com/thanos-io/thanos/pkg/objstore/cos"
 	"github.com/thanos-io/thanos/pkg/objstore/gcs"
+
+	"github.com/thanos-io/thanos/pkg/objstore/oss"
+


jojohappy · 2019-07-29T02:20:40Z

cmd/thanos/rule.go

@@ -39,7 +39,9 @@ import (
 	"github.com/thanos-io/thanos/pkg/discovery/cache"
 	"github.com/thanos-io/thanos/pkg/discovery/dns"
 	"github.com/thanos-io/thanos/pkg/extprom"
+


Remove blank line.

jojohappy · 2019-07-29T02:20:49Z

cmd/thanos/rule.go

 	extpromhttp "github.com/thanos-io/thanos/pkg/extprom/http"
+


jojohappy · 2019-07-29T02:21:04Z

cmd/thanos/query.go

@@ -29,7 +29,9 @@ import (
 	"github.com/thanos-io/thanos/pkg/discovery/cache"
 	"github.com/thanos-io/thanos/pkg/discovery/dns"
 	"github.com/thanos-io/thanos/pkg/extprom"
+


Remove blank line.

jojohappy · 2019-07-29T02:21:13Z

cmd/thanos/query.go

 	extpromhttp "github.com/thanos-io/thanos/pkg/extprom/http"
+


jojohappy · 2019-07-29T08:25:50Z

cmd/thanos/bucket.go

@@ -31,6 +22,15 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/common/route"
 	"github.com/prometheus/tsdb/labels"
+	"github.com/thanos-io/thanos/pkg/block"


Why those import packages are moved here?

jojohappy · 2019-07-29T08:34:34Z

pkg/objstore/oss/oss.go

+					if err := b.bucket.AbortMultipartUpload(init); err != nil {
+						return prt, errors.Wrap(err, "failed to upload multi-part chunk")
+					}
+				}


You should return err at the end of if blank. Otherwise the error would return nil below.

jojohappy · 2019-07-30T02:15:21Z

CHANGELOG.md

@@ -12,7 +12,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
 ## Unreleased.

 - [#1338](https://github.com/thanos-io/thanos/pull/1338) Querier still warns on store API duplicate, but allows a single one from duplicated set. This is gracefully warn about the problematic logic and not disrupt immediately.
-
+- `AliYun OSS` object storage, see [documents](docs/storage.md#aliyun-oss-configuration) for further information.


Please add the PR id and link at the start of message.

povilasv · 2019-07-30T07:12:08Z

pkg/objstore/oss/oss.go

+	"time"
+
+	alioss "github.com/aliyun/aliyun-oss-go-sdk/oss"
+


remove the blank line

povilasv · 2019-07-30T07:17:12Z

pkg/objstore/oss/oss.go

+
+func calculateChunks(name string, r io.Reader) (int, int64, error) {
+	switch r.(type) {
+	case *os.File:


Consider just changing the interface for upload from io.Reader to something like:

type ReadSizer interface { Read(p []byte) (n int, err error) Size() int64 }

As this clearly won't work if change our types.

povilasv · 2019-07-30T07:19:54Z

pkg/objstore/oss/oss.go

+	}
+
+	ncloser := ioutil.NopCloser(r)
+	switch chunksnum {


could we simplify this code?

Something like:

if chunksnum == 0 { if err := b.bucket.PutObject(name, ncloser); err != nil { return errors.Wrap(err, "failed to upload oss object") } return nil } Init, err := b.bucket.InitiateMultipartUpload(name) if err != nil { return errors.Wrap(err, "failed to initiate multi-part upload") } ...

pkg/query/api/v1.go

wujinhu · 2019-07-31T02:25:17Z

pkg/objstore/oss/oss.go

+		if fileInfo, err := f.Stat(); err == nil {
+			s := fileInfo.Size()
+			return int(math.Floor(float64(s) / alioss.MaxPartSize)), s % alioss.MaxPartSize, nil
+		}


I think we should upload smaller part each time, not 5GB each part, things will fail.

I agree with you , the smaller part uploading will accelerate compacting . But there is a another problem , see UploadPart method in multipart.go (https://github.com/aliyun/aliyun-oss-go-sdk/blob/master/oss/multipart.go) , you can not excess ten thousand times for the multi-part upload task . This means , if I upload every part exactly 100M , I can upload to Aliyun OSS 1TiB object at most .As compacting works on and on , I think it may be not enough .

I think you can define part size like below:
partSize = max(100MB, fileSize / 10001)

wujinhu · 2019-08-20T10:48:59Z

@shaulboozhiao Any updates? @jojohappy and I found a bug here.

func setRange(start, end int64) (alioss.Option, error) {
	var opt alioss.Option
	if 0 <= start && start <= end {
		opt = alioss.Range(start, end)
	} else {
		return nil, errors.Errorf("Invalid range specified: start=%d end=%d", start, end)
	}
	return opt, nil
}

OSS will get the whole object if end > size of this object. So, we will get error message like below if we call getRange method with large length.

bucket_e2e_test.go:355: unexpected error: rpc error: code = Aborted desc = fetch series for block 01DJQA7FZPVH4RM86QDFW6GVM5: preload series: invalid remaining size 427, expected 1430846

You can run bucket_e2e_test.go and view OSS documentation for more details.
https://help.aliyun.com/document_detail/31980.html?spm=a2c4g.11186623.6.781.181266f2Y8E5zs

wujinhu · 2019-08-21T01:36:13Z

@shaulboozhiao Do you still have time to finish this PR? I think I can do this if you do mind.

jojohappy · 2019-08-21T01:52:34Z

OSS will get the whole object if end > size of this object. So, we will get error message like below if we call getRange method with large length.

@shaulboozhiao It meant that we could not fetch objects with specific range if it is out of length of object, but thanos need it. Other object storage service, such as gcs, s3, support it as default, but not Aliyun OSS. So we should check the end of range whether or not is out of length of object in this case.

bwplotka · 2019-08-23T12:31:09Z

Awesome work @shaulboozhiao here and everyone else 👍 ❤️ Looks like you @shaulboozhiao did the majority of work and only last touches/rebase plus some testing are left. (:

Since you are busy I think it makes sense for others to continue your work on top of your commits if that's ok with you! @wujinhu if @shaulboozhiao is busy and you have time, feel free to get his changes on your branch and start a new PR and we will close this one.

As per our Code of Conduct we are here all for joint collaboration and being friendly to each other, so I think that makes sense. (:
We all want your changes @shaulboozhiao to be available for everyone, so let's deliver it together!

In case of any questions we are all available on CNCF slack (:

wujinhu · 2019-09-02T07:18:51Z

Because @shaulboozhiao is not responsive, I will open another PR this week as discussed above.

bwplotka · 2019-09-02T08:09:15Z

Awesome, closing this then now. @wujinhu make sure to grab @shaulboozhiao changes and feel free to finish on top of this 👍

thanos and others added 5 commits June 7, 2019 22:30

Implemented AliYun OSS for objstore support

d67a69d

restore github.com/improbable-eng/thanos/blob/master/go.mod & go.sum

f973be4

Implemented AliYun OSS for objstore support following Object Storage->

4abb631

aliyun oss objstore works well

4538290

put img under 'img' path

86c5548

ghost mentioned this pull request Jun 8, 2019

objstore : implement Aliyun OSS #1221

Closed

jojohappy suggested changes Jun 10, 2019

View reviewed changes

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

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

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

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

bwplotka requested review from bwplotka and povilasv June 10, 2019 09:28

bwplotka added the feature request/improvement label Jun 10, 2019

zhaoxiaobo_cd and others added 8 commits June 11, 2019 09:38

correct method Iter for pageing view objects

714b5d3

Update config.yml

9dd19b7

change storage.md

5202497

change storage.md

9b64327

Update storage.md

3240df3

Update Makefile

aec4ee4

Update config.yml

1182217

debug makefile

Update config.yml

a827af3

add docs dependency

snowbubblyzone added 3 commits June 11, 2019 20:33

Update config.yml

495908d

debug make test

Update oss.go

a783bb3

Update config.yml

b47347f

wujinhu reviewed Jul 9, 2019

View reviewed changes

jojohappy reviewed Jul 10, 2019

View reviewed changes

[email protected] and others added 12 commits July 27, 2019 16:59

update code for multi-part upload , change improbable-eng to thanos-io

dca0fb7

update code for multi-part upload , change improbable-eng to thanos-io

feed309

Merge branch 'master' into master

1091424

Update main.go

540fc5b

Update pool_test.go

c1bab13

Update handler.go

f6f50d3

Update proxy.go

1130f3d

Update bucket.go

656e1fb

Update oss.go

037f411

Update oss.go

0f89466

Update bucket.go

efa50b1

update code for multi-part upload

04c9098

jojohappy suggested changes Jul 30, 2019

View reviewed changes

povilasv requested changes Jul 30, 2019

View reviewed changes

wujinhu suggested changes Jul 31, 2019

View reviewed changes

remove blank line , add PR to CHANGELOG.md

6b6a70c

bwplotka closed this Sep 2, 2019

wujinhu mentioned this pull request Sep 4, 2019

objstore : implement Aliyun OSS #1490

Closed

wujinhu mentioned this pull request Sep 26, 2019

objstore : implement Aliyun OSS #1573

Merged

	return errors.Wrap(err, "multi-part upload last chunk failed")
	return errors.Wrap(err, "failed to upload the last chunk")

	return errors.Wrap(err, "initiate multi-part upload failed")
	return errors.Wrap(err, "failed to initiate multi-part upload")

	return errors.Wrap(err, "upload oss object")
	return errors.Wrap(err, "failed to upload object")

	return errors.Wrap(err, "multi-part upload failed")
	return errors.Wrap(err, "failed to set multi-part upload completive")

objstore : implement Aliyun OSS #1234

objstore : implement Aliyun OSS #1234

Conversation

ghost commented Jun 7, 2019 • edited by ghost Loading

Changes

Verification

wujinhu commented Jun 10, 2019

wujinhu commented Jun 10, 2019

jojohappy left a comment

Choose a reason for hiding this comment

jojohappy commented Jun 10, 2019 • edited Loading

domgreen commented Jun 10, 2019

ghost commented Jun 10, 2019

ghost commented Jun 10, 2019

wujinhu commented Jun 11, 2019

ghost commented Jun 11, 2019

jojohappy commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jojohappy commented Jul 19, 2019

ghost commented Jul 23, 2019

ghost commented Jul 27, 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

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 Aug 20, 2019

wujinhu commented Aug 21, 2019

jojohappy commented Aug 21, 2019

bwplotka commented Aug 23, 2019 • edited Loading

wujinhu commented Sep 2, 2019

bwplotka commented Sep 2, 2019

ghost commented Jun 7, 2019 •

edited by ghost

Loading

jojohappy commented Jun 10, 2019 •

edited

Loading

bwplotka commented Aug 23, 2019 •

edited

Loading