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.

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: Querier: Split gRPC client into two. #12726

Merged
merged 18 commits into from
May 6, 2024

Conversation

DylanGuedes
Copy link
Contributor

@DylanGuedes DylanGuedes commented Apr 22, 2024

What this PR does / why we need it:
Split the gRPC client used by the querier into two, one for the communication with the scheduler, the other for communicating with the query-frontend.

  • This change is retrocompatible: you don't have to change anything to keep existing behavior.
  • To configure the custom scheduler grpc client, you can use the new query_scheduler_grpc_client config or the new CLI flag querier.scheduler-grpc-client
  • If you'd like to configure your frontend grpc client using a better named section, you can use the new query_frontend_grpc_client instead of the old grpc_client_config. Just make sure you don't use both at the same time, it will result in an error.

This work is necessary for configuring custom behavior between querier<->scheduler vs querier<->frontend. A use case is configuring mTLS when a different certificate is used by queriers, schedulers and frontends. You can only configure a single server_name with our current setup, making it impossible.

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
  • 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

@DylanGuedes DylanGuedes requested a review from a team as a code owner April 22, 2024 13:31
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Apr 22, 2024
docs/sources/shared/configuration.md Outdated Show resolved Hide resolved
pkg/querier/worker/worker.go Outdated Show resolved Hide resolved
@slim-bean
Copy link
Collaborator

I think maybe a different approach here would be to introduce two new sections

query_scheduler_grpc_client_config
query_frontend_grpc_client_config

And then leave the current section as is and have it populate both of these values.

This allows us to not have to use a boolean value to decide which to use, instead we should add validation to the validate function in loki.go that errors if you specify grpc_client_config AND either of the two new configs.

We can also decide too if we want to deprecate grpc_client_config

WDYT?

@pull-request-size pull-request-size bot added size/L and removed size/M labels May 2, 2024
@DylanGuedes
Copy link
Contributor Author

I think maybe a different approach here would be to introduce two new sections

query_scheduler_grpc_client_config
query_frontend_grpc_client_config

And then leave the current section as is and have it populate both of these values.

This allows us to not have to use a boolean value to decide which to use, instead we should add validation to the validate function in loki.go that errors if you specify grpc_client_config AND either of the two new configs.

We can also decide too if we want to deprecate grpc_client_config

WDYT?

Ok I pushed a commit doing something similar:

  • No boolean flag anymore
  • Fully retrocompatible - you don't have to change anything to keep the existing behavior
  • If you'd like to customize a custom grpc config for the scheduler connection you only have to use the new query_scheduler_grpc_client
  • If you'd like to use a config that has a better name, you can use query_frontend_grpc_client or the new CLI flags querier.frontend-grpc-client. The only rule is: you can't use both the old grpc_client_config and the new section at the same time; if you try to do so, an error will be thrown

Copy link
Contributor

@salvacorts salvacorts left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you Dylan :)

@@ -5329,83 +5328,83 @@ The `swift_storage_config` block configures the connection to OpenStack Object S

```yaml
# OpenStack Swift authentication API version. 0 to autodetect.
# CLI flag: -<prefix>.swift.auth-version
# CLI flag: -swift.auth-version
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these shouldn't be here.

@github-actions github-actions bot removed the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label May 3, 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 May 5, 2024
@@ -543,19 +543,19 @@ The `alibabacloud_storage_config` block configures the connection to Alibaba Clo

```yaml
# Name of OSS bucket.
# CLI flag: -common.storage.oss.bucketname
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this flag docs was bugged this whole time

@DylanGuedes DylanGuedes merged commit 7b6f057 into main May 6, 2024
59 checks passed
@DylanGuedes DylanGuedes deleted the have-extra-scheduler-grpc-config branch May 6, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants