Feat: add Baidu Cloud BOS as storage backends for Loki #4788 #5848

arcosx · 2022-04-09T16:42:59Z

What this PR does / why we need it:

Hi all, I plan to add Baidu Cloud (one of the Chinese cloud vendors, also named as Baidu BCE) BOS (object storage, like s3) as one of the storage backends for Loki. In China many users don't use cloud vendors like amazon and google cloud. Adding Baidu Cloud BOS as a backend storage will help more Chinese users to try Loki.

some links:

about Baidu Cloud Cloud https://intl.cloud.baidu.com/
about Baidu Cloud Object Storage Service (BOS) https://intl.cloud.baidu.com/doc/BOS/s/xjwvyq5g7-en

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

Documentation added
Tests updated
Add an entry in the CHANGELOG.md about the changes.

Signed-off-by: arcosx <arcosx@outlook.com>

owen-d · 2022-04-12T16:58:49Z

Thanks for the PR! I don't have much time at the moment, but I'll see if we can get another to review it in the meantime.

trevorwhitney

LGTM! Great work, thanks for the contribution. Looks like this very closely follows our patterns for S3. Just a few small questions and changes.

We don't have an environment to test this in. I assume you do? Please keep us in the loop on how this is working in that env and follow up with any bugs you see there.

Thanks!

trevorwhitney · 2022-04-12T17:15:04Z

docs/sources/configuration/_index.md

@@ -483,6 +483,9 @@ storage:
  # Configures backend rule storage for a local file system directory.
  [local: <local_storage_config>]

+  # Configures backend rule storage for Baidu BCE BOS.


should add an option to the comment above on type as well to include bos

# Method to use for backend rule storage (azure, gcs, s3, swift, local). # CLI flag: -ruler.storage.type [type: <string> ]

This comment. I agree.

Thanks, I was careless, this has been fixed.

trevorwhitney · 2022-04-12T17:17:31Z

go.mod

@@ -110,6 +110,8 @@ require (
 	k8s.io/klog v1.0.0
 )

+require github.com/baidubce/bce-sdk-go v0.9.81


I'm not an expert around why we have this file organized the way we do, but I imagine we want this in one of the other require sections rather than sitting on it's own?

yes! I move it.

trevorwhitney · 2022-04-12T18:15:56Z

pkg/ruler/base/storage.go

@@ -65,6 +67,9 @@ func (cfg *RuleStoreConfig) Validate() error {
 	if err := cfg.S3.Validate(); err != nil {
 		return errors.Wrap(err, "invalid S3 Storage config")
 	}
+	if err := cfg.BOS.Validate(); err != nil {


this function always returns nil, why bother calling it here and implenting it as a no-op. We could follow a similar patter with the GCSConfig where we don't call Validate here and therefore don't need to implement it as a no-op. WDYT?

I delete the function Validate() .You are right.It no need used now.

trevorwhitney · 2022-04-12T18:18:08Z

pkg/storage/chunk/client/baidubce/bos_storage_client.go

+}
+
+func (cfg *BosStorageConfig) Validate() error {
+	return nil


see comment above, do we need to implement this as a no-op to satisfy an interface or can we just avoid calling it?

trevorwhitney · 2022-04-12T18:23:18Z

pkg/storage/chunk/client/baidubce/bos_storage_client.go

+			}
+			for _, content := range listObjectResult.Contents {
+				// LastModified format 2021-10-28T06:55:01Z
+				LastModifiedTime, err := time.Parse(time.RFC3339, content.LastModified)


convention is lowerCamelCase for the local variable

Suggested change

LastModifiedTime, err := time.Parse(time.RFC3339, content.LastModified)

lastModifiedTime, err := time.Parse(time.RFC3339, content.LastModified)

trevorwhitney · 2022-04-12T18:24:15Z

pkg/storage/chunk/client/baidubce/bos_storage_client.go

+				}
+				storageObjects = append(storageObjects, client.StorageObject{
+					Key:        content.Key,
+					ModifiedAt: LastModifiedTime,


Suggested change

ModifiedAt: LastModifiedTime,

ModifiedAt: lastModifiedTime,

cstyan · 2022-04-13T23:31:11Z

docs/sources/configuration/_index.md

@@ -483,6 +483,9 @@ storage:
  # Configures backend rule storage for a local file system directory.
  [local: <local_storage_config>]

+  # Configures backend rule storage for Baidu BCE BOS.


# Method to use for backend rule storage (azure, gcs, s3, swift, local). # CLI flag: -ruler.storage.type [type: <string> ]

This comment. I agree.

docs/sources/configuration/_index.md

pkg/loki/config_wrapper_test.go

pkg/storage/chunk/client/baidubce/bos_storage_client.go

cstyan · 2022-04-14T00:02:14Z

pkg/storage/chunk/client/baidubce/bos_storage_client.go

+		if realErr.Code == ObjectNotFoundErr {
+			return true
+		}


Will this work? I'm not understanding the link you have above the ObjectNotFoundErr definition, and how will you compare the string ObjectNotFoundErr to an error code?

yes.It works fine.We can use this error code to represent that the storage object does not exist.I test is no problem.
This page describe it . https://intl.cloud.baidu.com/doc/BOS/s/Ajwvysfpl-en

Okay, can you please include a comment to this link or something similar that shows the struct definition for BceServiceError. I was confused because by default I expect something called error.Code to be an integer type.

Yes, I agree with what you said, I think it would be better to return integer error codes here as well.
Now I added comments pointing to its source code.

pkg/storage/factory.go

Signed-off-by: arcosx <arcosx@outlook.com>

arcosx · 2022-04-19T17:22:50Z

Thanks for @trevorwhitney @cstyan guidance, I have a complete test environment on my side, and the following 3-minute video shows the full extent of this feature(The video has sound, but closing it does not affect the viewing), which can be applied on production systems.

Screencast.2022-04-20.00.22.41.mp4

Signed-off-by: arcosx <arcosx@outlook.com>

KMiller-Grafana

A couple of doc improvements. Thanks.

One of these changes should eventually be propagated to other storage block configuration sections in the docs. For now, we can have the BOS section description have better wording than the other storage section descriptions.

CHANGELOG.md

docs/sources/configuration/_index.md

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> Signed-off-by: arcosx <arcosx@outlook.com>

trevorwhitney

Thanks for making those changes, LGTM!

KMiller-Grafana

Changes are to correctly specify the product name. Some changes are required for my approval, while others are suggestions that might not be possible given vendored code.

docs/sources/configuration/_index.md

pkg/storage/chunk/client/baidubce/bos_storage_client.go

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

KMiller-Grafana

Thank you for making all the changes. This PR looks good from a docs perspective now.

arcosx · 2022-04-28T07:34:32Z

@owen-d @trevorwhitney @cstyan @KMiller-Grafana Thanks again for reviewing the code ! Can it be merged?

owen-d

Hey, sorry it took so long for me to review this. It looks great!

Thanks for the diligent work during review and the video demo.

arcosx added 2 commits February 15, 2022 20:52

feat: add baidu bce bos storage support

9d58e8f

Signed-off-by: arcosx <arcosx@outlook.com>

Merge branch 'main' into feat/baidubce

23a8c8e

pull-request-size bot added the size/XXL label Apr 9, 2022

arcosx force-pushed the feat/baidubce-new branch from dd4627b to 22c86e8 Compare April 9, 2022 16:52

arcosx changed the title ~~Feat: add Baidu AI Cloud as storage backends for Loki #4788~~ Feat: add Baidu BCE BOS as storage backends for Loki #4788 Apr 9, 2022

arcosx force-pushed the feat/baidubce-new branch 2 times, most recently from 69e4188 to 33af522 Compare April 9, 2022 17:02

arcosx changed the title ~~Feat: add Baidu BCE BOS as storage backends for Loki #4788~~ Feat: add Baidu Cloud BOS as storage backends for Loki #4788 Apr 9, 2022

add baidu bce bos as chunk storage backend

cd077b7

Signed-off-by: arcosx <arcosx@outlook.com>

arcosx force-pushed the feat/baidubce-new branch from 33af522 to cd077b7 Compare April 10, 2022 06:55

arcosx marked this pull request as ready for review April 10, 2022 06:59

arcosx requested review from KMiller-Grafana and a team as code owners April 10, 2022 06:59

arcosx mentioned this pull request Apr 12, 2022

Feat: add Baidu AI Cloud as storage backends for Loki #4788

Closed

trevorwhitney approved these changes Apr 12, 2022

View reviewed changes

cstyan reviewed Apr 14, 2022

View reviewed changes

arcosx added 2 commits April 18, 2022 17:12

Merge branch 'main' into feat/baidubce-new

655ab0e

fix: some doc error && rewrite bad code

b293942

Signed-off-by: arcosx <arcosx@outlook.com>

arcosx requested review from cstyan and trevorwhitney April 19, 2022 17:28

fix: add the BceServiceError source link

5876eaf

Signed-off-by: arcosx <arcosx@outlook.com>

cstyan approved these changes Apr 20, 2022

View reviewed changes

KMiller-Grafana suggested changes Apr 20, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved

arcosx and others added 2 commits April 21, 2022 11:28

Update CHANGELOG.md

cbe43d6

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

Update docs/sources/configuration/_index.md

a38578f

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> Signed-off-by: arcosx <arcosx@outlook.com>

arcosx force-pushed the feat/baidubce-new branch from 9314495 to a38578f Compare April 21, 2022 05:45

arcosx requested a review from KMiller-Grafana April 21, 2022 12:13

trevorwhitney approved these changes Apr 26, 2022

View reviewed changes

KMiller-Grafana suggested changes Apr 26, 2022

View reviewed changes

arcosx and others added 7 commits April 27, 2022 11:02

Update docs/sources/configuration/_index.md

fb30c1e

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

Update docs/sources/configuration/_index.md

9a51421

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

Update docs/sources/configuration/_index.md

737190b

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

Update docs/sources/configuration/_index.md

01a83ce

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

Update pkg/storage/chunk/client/baidubce/bos_storage_client.go

6fb42b4

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

Update pkg/storage/chunk/client/baidubce/bos_storage_client.go

9a0a0be

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

Update pkg/storage/chunk/client/baidubce/bos_storage_client.go

ae5c2ef

Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

arcosx requested a review from KMiller-Grafana April 27, 2022 05:27

KMiller-Grafana approved these changes Apr 27, 2022

View reviewed changes

owen-d approved these changes May 5, 2022

View reviewed changes

owen-d merged commit e2fdc34 into grafana:main May 5, 2022

arcosx mentioned this pull request May 6, 2022

Feature: support Tencent Cloud COS provider #6070

Open

This was referenced Jun 28, 2022

dannykopping/remove cache stats dannykopping/loki#13

Closed

dannykopping/remove cache stats dannykopping/loki#14

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: add Baidu Cloud BOS as storage backends for Loki #4788 #5848

Feat: add Baidu Cloud BOS as storage backends for Loki #4788 #5848

arcosx commented Apr 9, 2022 •

edited

Loading

owen-d commented Apr 12, 2022

trevorwhitney left a comment

trevorwhitney Apr 12, 2022

cstyan Apr 13, 2022

arcosx Apr 19, 2022

trevorwhitney Apr 12, 2022

arcosx Apr 19, 2022

trevorwhitney Apr 12, 2022

arcosx Apr 19, 2022

trevorwhitney Apr 12, 2022

trevorwhitney Apr 12, 2022

arcosx Apr 19, 2022

trevorwhitney Apr 12, 2022

arcosx Apr 19, 2022

cstyan Apr 13, 2022

cstyan Apr 14, 2022

arcosx Apr 19, 2022

cstyan Apr 20, 2022

arcosx Apr 20, 2022

arcosx commented Apr 19, 2022

KMiller-Grafana left a comment

trevorwhitney left a comment

KMiller-Grafana left a comment

KMiller-Grafana left a comment

arcosx commented Apr 28, 2022 •

edited

Loading

owen-d left a comment

	LastModifiedTime, err := time.Parse(time.RFC3339, content.LastModified)
	lastModifiedTime, err := time.Parse(time.RFC3339, content.LastModified)

Feat: add Baidu Cloud BOS as storage backends for Loki #4788 #5848

Feat: add Baidu Cloud BOS as storage backends for Loki #4788 #5848

Conversation

arcosx commented Apr 9, 2022 • edited Loading

owen-d commented Apr 12, 2022

trevorwhitney 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcosx commented Apr 19, 2022

KMiller-Grafana left a comment

Choose a reason for hiding this comment

trevorwhitney left a comment

Choose a reason for hiding this comment

KMiller-Grafana left a comment

Choose a reason for hiding this comment

KMiller-Grafana left a comment

Choose a reason for hiding this comment

arcosx commented Apr 28, 2022 • edited Loading

owen-d left a comment

Choose a reason for hiding this comment

arcosx commented Apr 9, 2022 •

edited

Loading

arcosx commented Apr 28, 2022 •

edited

Loading