-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Feat: add Baidu Cloud BOS as storage backends for Loki #4788 #5848
Conversation
Signed-off-by: arcosx <arcosx@outlook.com>
dd4627b
to
22c86e8
Compare
69e4188
to
33af522
Compare
Signed-off-by: arcosx <arcosx@outlook.com>
33af522
to
cd077b7
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
docs/sources/configuration/_index.md
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should add an option to the comment above on type
as well to include bos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Method to use for backend rule storage (azure, gcs, s3, swift, local).
# CLI flag: -ruler.storage.type
[type: <string> ]
This comment. I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I was careless, this has been fixed.
go.mod
Outdated
@@ -110,6 +110,8 @@ require ( | |||
k8s.io/klog v1.0.0 | |||
) | |||
|
|||
require github.com/baidubce/bce-sdk-go v0.9.81 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! I move it.
pkg/ruler/base/storage.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I delete the function Validate()
.You are right.It no need used now.
} | ||
|
||
func (cfg *BosStorageConfig) Validate() error { | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above, do we need to implement this as a no-op to satisfy an interface or can we just avoid calling it?
} | ||
for _, content := range listObjectResult.Contents { | ||
// LastModified format 2021-10-28T06:55:01Z | ||
LastModifiedTime, err := time.Parse(time.RFC3339, content.LastModified) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convention is lowerCamelCase
for the local variable
LastModifiedTime, err := time.Parse(time.RFC3339, content.LastModified) | |
lastModifiedTime, err := time.Parse(time.RFC3339, content.LastModified) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
storageObjects = append(storageObjects, client.StorageObject{ | ||
Key: content.Key, | ||
ModifiedAt: LastModifiedTime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModifiedAt: LastModifiedTime, | |
ModifiedAt: lastModifiedTime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
docs/sources/configuration/_index.md
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Method to use for backend rule storage (azure, gcs, s3, swift, local).
# CLI flag: -ruler.storage.type
[type: <string> ]
This comment. I agree.
if realErr.Code == ObjectNotFoundErr { | ||
return true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Signed-off-by: arcosx <arcosx@outlook.com>
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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>
9314495
to
a38578f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those changes, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making all the changes. This PR looks good from a docs perspective now.
@owen-d @trevorwhitney @cstyan @KMiller-Grafana Thanks again for reviewing the code ! Can it be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CHANGELOG.md
about the changes.