Skip to content

Conversation

@ashwanthgoli
Copy link
Contributor

@ashwanthgoli ashwanthgoli commented Nov 13, 2024

What this PR does / why we need it:
Adds support for aliyun oss and baidu bos thanos clients.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
@ashwanthgoli
Copy link
Contributor Author

@liguozhong, @arcosx loki is planning to introduce object store clients based off https://github.com/thanos-io/objstore, but we do not have a test environment to run a sanity check for aliyun oss and baidu bos.

I noticed that you folks added the support for these providers. It would be great if you could help run a sanity check with this change on your test environment :) Happy to help with any questions you have

@ashwanthgoli ashwanthgoli marked this pull request as ready for review November 13, 2024 12:35
@ashwanthgoli ashwanthgoli requested a review from a team as a code owner November 13, 2024 12:35
@ashwanthgoli ashwanthgoli requested review from a team and JoaoBraveCoding and removed request for a team November 13, 2024 12:35
@arcosx
Copy link
Contributor

arcosx commented Nov 13, 2024

@liguozhong, @arcosx loki is planning to introduce object store clients based off https://github.com/thanos-io/objstore, but we do not have a test environment to run a sanity check for aliyun oss and baidu bos.

I noticed that you folks added the support for these providers. It would be great if you could help run a sanity check with this change on your test environment :) Happy to help with any questions you have

OK,I will test for bos.

@ashwanthgoli
Copy link
Contributor Author

@arcosx thanks a lot! Just to clarity, thanos clients have to be explicitly enabled by setting -use-thanos-objstore=true
and the storage has to be configured in the object_store block or using cli flags with prefix-object-store.bos.

storage_config:
  object_store:
    bos:
      bucket: ****
Copy link
Collaborator

@JoaoBraveCoding JoaoBraveCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a code POV lgtm

case Swift:
cfg.Swift.MaxRetries = 1
case Filesystem:
case Filesystem, Alibaba, BOS:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed: with thanos-io/objstore#147 merged we can now define these for these two no? Also fine to do it on a follow up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both bos and oss do not have a any way to configure retries, that's what i gathered from my last search :)

@ashwanthgoli ashwanthgoli merged commit fb6789d into main Nov 15, 2024
59 checks passed
@ashwanthgoli ashwanthgoli deleted the thanos-more-providers branch November 15, 2024 14:10
thevops pushed a commit to thevops/loki that referenced this pull request Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 participants