Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(storage): GCS backend using thanos.io/objstore #11132

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

feat(storage): GCS backend using thanos.io/objstore #11132

wants to merge 50 commits into from

Conversation

JoaoBraveCoding
Copy link
Collaborator

@JoaoBraveCoding JoaoBraveCoding commented Nov 3, 2023 •
edited by ashwanthgoli
Loading

What this PR does / why we need it:

This PR adds support to use the thanos.io/objstore backend for the GCS provider for all components including the Ruler

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

CLI Table

gcs.bucketname
gcs.service-account
gcs.chunk-buffer-size

NOT PRESENT AT THE MOMENT

gcs.enable-opencensus (Would need objstore patch)
gcs.enable-retries (Would need objstore patch)
gcs.request-timeout (will be added as a common option for all providers)
gcs.enable-http2

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • 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

@JoaoBraveCoding JoaoBraveCoding mentioned this pull request Nov 9, 2023
Merged
2 tasks
@JoaoBraveCoding JoaoBraveCoding changed the title WIP: GCP implementation using thanos.io/objstore GCP implementation using thanos.io/objstore Nov 14, 2023
@JoaoBraveCoding JoaoBraveCoding marked this pull request as ready for review November 14, 2023 10:30
@JoaoBraveCoding JoaoBraveCoding requested a review from a team as a code owner November 14, 2023 10:30
@JoaoBraveCoding JoaoBraveCoding changed the title GCP implementation using thanos.io/objstore storage: GCS backend using thanos.io/objstore Nov 14, 2023
@JoaoBraveCoding JoaoBraveCoding mentioned this pull request Nov 14, 2023
Open
8 tasks
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 21, 2023 •
edited
Loading

Trivy scan found the following vulnerabilities:

@chaudum
Copy link
Contributor

chaudum commented Nov 23, 2023

This PR was inspired by the way Mimir uses thanos.io/objstore with the goal of making this part of the codebase as similar as possible

Thank you! ❤️

@JoaoBraveCoding JoaoBraveCoding mentioned this pull request Nov 24, 2023
Open
8 tasks
@JoaoBraveCoding JoaoBraveCoding changed the title storage: GCS backend using thanos.io/objstore feat(storage): GCS backend using thanos.io/objstore Mar 14, 2024
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Mar 14, 2024
@JoaoBraveCoding JoaoBraveCoding force-pushed the log-4550-gcp branch 2 times, most recently from ec00fec to d51bbd5 Compare September 30, 2024 14:12
ashwanthgoli
ashwanthgoli reviewed Oct 8, 2024
View reviewed changes
pkg/loki/common/common.go Outdated Show resolved Hide resolved
pkg/loki/config_wrapper.go Outdated Show resolved Hide resolved
pkg/storage/bucket/client.go Outdated Show resolved Hide resolved
pkg/loki/modules.go Outdated Show resolved Hide resolved
@ashwanthgoli
use global thanos metrics and other changes (#1)
ca835ad 
    use a global thanos metric.
    remove ruler changes, will be added in a separate pr
    rename configs
DylanGuedes
DylanGuedes reviewed Oct 9, 2024
View reviewed changes
Copy link
Contributor

@DylanGuedes DylanGuedes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I know that the change was introduced in a previous PR but anyway, now we're doing this everywhere:

		var objectClient client.ObjectClient
		var err error
		if t.Cfg.StorageConfig.UseThanosObjstore {
			objectClient, err = storage.NewObjectClientV2("compactor", periodConfig.ObjectType, t.Cfg.StorageConfig)
		} else {
			objectClient, err = storage.NewObjectClient(periodConfig.ObjectType, t.Cfg.StorageConfig, t.ClientMetrics)
		}
		if err != nil ...

WDYT of we instead moving the if conditional to the storage.NewObjectClientV2 instead, and adding a new parameter to it like legacy=true that dictates if internally it will instantiate V2 or V1?
the way we're doing here we're having to duplicate the condition everywhere (which has the risk of we missing something) and external libs will also have to add the same conditional.

JoaoBraveCoding
JoaoBraveCoding commented Oct 15, 2024
View reviewed changes
Copy link
Collaborator Author

@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.

Just some small comments but overall lgmt 🙌 thank you for the work to get this merged!

pkg/storage/chunk/client/gcp/gcs_thanos_object_client.go
Comment on lines +93 to +96
if err != nil {
size = -1
level.Warn(s.logger).Log("msg", "failed to get size of object", "err", err)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here since we are always dealing with the gcs provider is this code path possible? IIUC the TryToGetSize will only produce an error if it gets an unsupported io.Reader

Copy link
Contributor

Choose a reason for hiding this comment

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

likely not for gcs, but it might also fail if the provider specific code fails to get the size for some reason. Seems very unlikely but I added this to not fail the Get request entirely if we cannot figure out the size. Size is only being used to reduce allocations, does not affect any functionality

pkg/storage/bucket/client.go
ErrUnsupportedStorageBackend = errors.New("unsupported storage backend")
ErrInvalidCharactersInStoragePrefix = errors.New("storage prefix contains invalid characters, it may only contain digits and English alphabet letters")

// TODO: Remove this global if we can tag all clients with component name
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even if we do this aren't we still going to hit duplicated metric registration for multiple schema_config?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, i think the comment can be more descriptive. i'll fix it :)

docs/sources/shared/configuration.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DylanGuedes DylanGuedes DylanGuedes left review comments

@ashwanthgoli ashwanthgoli ashwanthgoli left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
size/XL type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JoaoBraveCoding @chaudum @kavirajk @DylanGuedes @ashwanthgoli