Add Huawei Cloud OBS Object Storage Support #50

setoru · 2023-04-27T02:57:17Z

Huawei Cloud is a public cloud provider in China, Its object storage product is OBS.

thanos is not supported OBS right now,This pr implements a OBS client and it seems to be ok.

I would like to become the maintainer for the OBS provider and keep development for Thanos.

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Implement Bucket interface
Add NewTestBucket constructor
Pass the ForeachStore test
Add client implementation to the factory
Add client struct config to bucketcfggen

Verification

Signed-off-by: setoru <setoru127@gmail.com>

matej-g

Thanks for this! For now I skimmed through the PR, few suggestions. It also looks like the tests and lint are failing for now.

matej-g · 2023-04-27T09:33:39Z

objtesting/foreach.go

@@ -4,6 +4,7 @@
 package objtesting

 import (
+	"github.com/thanos-io/objstore/providers/obs"


nit: Can we keep imports grouping? i.e. move this down to the last group.

sure can,I will fix it.

providers/obs/obs.go

matej-g · 2023-04-27T09:34:21Z

scripts/cfggen/main.go

@@ -5,6 +5,7 @@ package main

 import (
 	"fmt"
+	"github.com/thanos-io/objstore/providers/obs"


Nit: same as previous (import grouping)

matej-g · 2023-04-27T09:37:47Z

client/factory.go

@@ -6,6 +6,7 @@ package client
 import (
 	"context"
 	"fmt"
+	"github.com/thanos-io/objstore/providers/obs"


Nit: Import ordering

matej-g · 2023-04-27T09:41:14Z

providers/obs/obs.go

+	_, err := b.client.PutObject(input)
+	return errors.Wrap(err, "fail to upload object")


If find doing if err != nil { return errors.Wrap(...) } to be a readable a bit better rather than returning directly errors.Wrap.

You are right,I will fix it

matej-g · 2023-04-27T09:41:51Z

providers/obs/obs.go

+	initOutput, err := b.client.InitiateMultipartUpload(initInput)
+	return initOutput, errors.Wrap(err, "fail to init multipart upload job")


Same as previous comment (Doing if err != nil { return errors.Wrap(...) } rather than directly returning wrap).

Signed-off-by: setoru <setoru127@gmail.com>

matej-g

This is looking good, thank you! I have added couple more suggestions after second review.

matej-g · 2023-05-04T12:55:05Z

providers/obs/obs.go

+	if length != -1 {
+		input.RangeEnd = off + length - 1
+	} else {
+		input.RangeEnd = math.MaxInt64
+	}


Nit: Small readability improvement suggestion.

Suggested change

if length != -1 {

input.RangeEnd = off + length - 1

} else {

input.RangeEnd = math.MaxInt64

}

input.RangeEnd = math.MaxInt64

if length != -1 {

input.RangeEnd = off + length - 1

}

matej-g · 2023-05-04T12:56:26Z

providers/obs/obs.go

+			Parts:    parts,
+		})
+		if err != nil {
+			return errors.Wrap(err, "fail to complete multipart upload")


Nit: Could we make all comments say failed instead of fail

Suggested change

return errors.Wrap(err, "fail to complete multipart upload")

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

matej-g · 2023-05-04T12:57:29Z

providers/obs/obs.go

+		}
+		for _, content := range output.Contents {
+			if err := f(content.Key); err != nil {
+				return errors.Wrap(err, "fail to call f for object")


We could make this a bit more user friendly, something along to lines failed to call iter function for object X?

matej-g · 2023-05-04T12:58:57Z

README.md

@@ -640,6 +641,41 @@ config:

 You can also include any of the optional configuration just like the example in `Default Provider`.

+##### HuaweiCloud OBS
+
+To use HuaweiCloud OBS as storage store, you should apply a HuaweiCloud Account to create an object storage bucket at first.


Suggested change

To use HuaweiCloud OBS as storage store, you should apply a HuaweiCloud Account to create an object storage bucket at first.

To use HuaweiCloud OBS as an object store, you should apply for a HuaweiCloud Account to create an object storage bucket at first.

matej-g · 2023-05-04T12:59:26Z

README.md

+##### HuaweiCloud OBS
+
+To use HuaweiCloud OBS as storage store, you should apply a HuaweiCloud Account to create an object storage bucket at first.
+Note that detailed from [HuaweiCloud OBS](https://support.huaweicloud.com/obs/index.html)


Suggested change

Note that detailed from [HuaweiCloud OBS](https://support.huaweicloud.com/obs/index.html)

More details: [HuaweiCloud OBS](https://support.huaweicloud.com/obs/index.html)

matej-g · 2023-05-04T12:59:39Z

README.md

+To use HuaweiCloud OBS as storage store, you should apply a HuaweiCloud Account to create an object storage bucket at first.
+Note that detailed from [HuaweiCloud OBS](https://support.huaweicloud.com/obs/index.html)
+
+To configure HuaweiCloud   Account to use OBS as storage store you need to set these parameters in yaml format stored in a file:


Suggested change

To configure HuaweiCloud Account to use OBS as storage store you need to set these parameters in yaml format stored in a file:

To configure HuaweiCloud Account to use OBS as storage store you need to set these parameters in YAML format stored in a file:

matej-g · 2023-05-04T13:02:52Z

providers/obs/obs.go

+	switch oriErr := errors.Cause(err).(type) {
+	case obs.ObsError:
+		if oriErr.Status == "404 Not Found" {


Since we have only one case, why not do oriErr, ok := errors.Cause(err).(obs.ObsError)?

Signed-off-by: setoru <setoru127@gmail.com>

matej-g · 2023-05-05T07:34:15Z

providers/obs/obs.go

 		}
 		for _, content := range output.Contents {
 			if err := f(content.Key); err != nil {
-				return errors.Wrap(err, "fail to call f for object")
+				return errors.Wrap(err, "failed to call iter function for object X")


I mean here instead of X, you could use content.Key to signal which object the function failed for?

matej-g · 2023-05-05T07:35:59Z

providers/obs/obs.go

 			}
 		}
 		for _, topDir := range output.CommonPrefixes {
 			if err := f(topDir); err != nil {
-				return errors.Wrap(err, "fail to call f for top dir object")
+				return errors.Wrap(err, "failed to call f for top dir object")


Can we adjust this the same way as my previous comment (don't say failed to call f ... but rather failed to call iter function)

Signed-off-by: setoru <setoru127@gmail.com>

matej-g

This is looking good now, thank you @setoru! 🙇

setoru · 2023-05-19T02:09:25Z

Thank you for reviewing!

setoru added 5 commits April 25, 2023 19:19

add obs support

576bc37

Signed-off-by: setoru <setoru127@gmail.com>

fix obs code style

aaa702b

Signed-off-by: setoru <setoru127@gmail.com>

add factory config for obs

0bd2405

Signed-off-by: setoru <setoru127@gmail.com>

add obs config to bucketcfggen

e26c94b

Signed-off-by: setoru <setoru127@gmail.com>

add changelog entry

803e50b

Signed-off-by: setoru <setoru127@gmail.com>

matej-g reviewed Apr 27, 2023

View reviewed changes

matej-g mentioned this pull request Apr 27, 2023

When will Huawei OBS storage be added? #36

Open

setoru added 2 commits April 28, 2023 15:53

fix imports

69c1212

Signed-off-by: setoru <setoru127@gmail.com>

fix return readable error

368a778

Signed-off-by: setoru <setoru127@gmail.com>

setoru requested a review from matej-g May 4, 2023 01:32

fix makefile and readme

f2475dc

Signed-off-by: setoru <setoru127@gmail.com>

matej-g reviewed May 4, 2023

View reviewed changes

fix readability problem and comment

e5dd1f8

Signed-off-by: setoru <setoru127@gmail.com>

setoru requested a review from matej-g May 5, 2023 03:18

matej-g reviewed May 5, 2023

View reviewed changes

fix comment

809c245

Signed-off-by: setoru <setoru127@gmail.com>

setoru requested a review from matej-g May 6, 2023 01:19

matej-g approved these changes May 11, 2023

View reviewed changes

matej-g merged commit 23ebe2e into thanos-io:main May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Huawei Cloud OBS Object Storage Support #50

Add Huawei Cloud OBS Object Storage Support #50

setoru commented Apr 27, 2023 •

edited

Loading

matej-g left a comment

matej-g Apr 27, 2023

setoru Apr 28, 2023

matej-g Apr 27, 2023

matej-g Apr 27, 2023

matej-g Apr 27, 2023

setoru Apr 28, 2023

matej-g Apr 27, 2023

matej-g left a comment

matej-g May 4, 2023

matej-g May 4, 2023

matej-g May 4, 2023

matej-g May 4, 2023

matej-g May 4, 2023

matej-g May 4, 2023

matej-g May 4, 2023

matej-g May 5, 2023

matej-g May 5, 2023

matej-g left a comment

setoru commented May 19, 2023

		_, err := b.client.PutObject(input)
		return errors.Wrap(err, "fail to upload object")

		initOutput, err := b.client.InitiateMultipartUpload(initInput)
		return initOutput, errors.Wrap(err, "fail to init multipart upload job")

	return errors.Wrap(err, "fail to complete multipart upload")
	return errors.Wrap(err, "failed to complete multipart upload")

	To use HuaweiCloud OBS as storage store, you should apply a HuaweiCloud Account to create an object storage bucket at first.
	To use HuaweiCloud OBS as an object store, you should apply for a HuaweiCloud Account to create an object storage bucket at first.

	Note that detailed from [HuaweiCloud OBS](https://support.huaweicloud.com/obs/index.html)
	More details: [HuaweiCloud OBS](https://support.huaweicloud.com/obs/index.html)

	To configure HuaweiCloud Account to use OBS as storage store you need to set these parameters in yaml format stored in a file:
	To configure HuaweiCloud Account to use OBS as storage store you need to set these parameters in YAML format stored in a file:

Add Huawei Cloud OBS Object Storage Support #50

Add Huawei Cloud OBS Object Storage Support #50

Conversation

setoru commented Apr 27, 2023 • edited Loading

Changes

Verification

matej-g 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

matej-g 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

matej-g left a comment

Choose a reason for hiding this comment

setoru commented May 19, 2023

setoru commented Apr 27, 2023 •

edited

Loading