Split querier GRPCClientConfig into separate frontend and scheduler client configs #6445
Conversation
fayzal-g
changed the title
fayzal-g
changed the title
pkg/querier/worker/worker.go
Outdated
| @@ -92,7 +109,8 @@ type querierWorker struct { | |||
| cfg Config | |||
| log log.Logger | |||
|
|
|||
| processor processor | |||
| processorType processorType | |||
There was a problem hiding this comment.
Instead of introducing processorType, we could explicitly pass correct grpc client config, and perhaps (if possible) get rid of cfg field completely (there's only one other field used from it: MaxConcurrentRequests). WDYT?
| @@ -44,7 +52,8 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | |||
| f.DurationVar(&cfg.DNSLookupPeriod, "querier.dns-lookup-period", 10*time.Second, "How often to query DNS for query-frontend or query-scheduler address.") | |||
| f.StringVar(&cfg.QuerierID, "querier.id", "", "Querier ID, sent to the query-frontend to identify requests from the same querier. Defaults to hostname.") | |||
|
|
|||
| cfg.GRPCClientConfig.RegisterFlagsWithPrefix("querier.frontend-client", f) | |||
| cfg.QueryFrontendGRPCClientConfig.RegisterFlagsWithPrefix("querier.frontend-client", f) | |||
| cfg.QuerySchedulerGRPCClientConfig.RegisterFlagsWithPrefix("querier.scheduler-client", f) | |||
There was a problem hiding this comment.
If someone is setting -querier.frontend-client.* today, it won't apply to scheduler client after this PR. That's a breaking change, and it should be called out in changelog.
It could be nice if we initialized values of scheduler-client from frontend-client settings. Perhaps by checking default values? (Goal is to not break existing configurations.)
| @@ -250,6 +250,8 @@ mimir: | |||
| frontend_worker: | |||
| grpc_client_config: | |||
| max_send_msg_size: 419430400 # 400MiB | |||
| query_scheduler_grpc_client_config: | |||
There was a problem hiding this comment.
I'd check with @krajorama how to introduce new flag into Helm like this.
There was a problem hiding this comment.
As suggested, will address Helm/Jsonnet changes in follow-up PR.
operations/mimir/querier.libsonnet
Outdated
| 'querier.frontend-client.grpc-max-send-msg-size': 100 << 20, | ||
| 'querier.scheduler-client.grpc-max-send-msg-size': 100 << 20, |
There was a problem hiding this comment.
"100 << 20" (which should be converted to human-readable number really), is the default value, and we don't need to set it. I'd suggest removing querier.frontend-client.grpc-max-send-msg-size instead (in separate PR).
There was a problem hiding this comment.
As suggested, will address Helm/Jsonnet changes in follow-up PR.
What this PR does
Currently, the same
GRPCClientConfigis used in thequerier_workerto interact with both thequery_frontendandquery_scheduler- it has been discovered that this causes an issue when configuring TLS.To illustrate:
query_frontendis issued with a Server certificate with SAN:query-frontend.dnsquery_scheduleris issued with a Server certificate with SAN:query-scheduler.dnsWhen configuring the
tls_server_namefor the querier worker (-querier.frontend-client.tls-server-name), there will be an error during the TLS handshake with either the frontend or the scheduler, depending on which server name is specified in the config.To alleviate this, an additional
GRPCClientConfighas been introduced specifically for thequery-scheduler- the existing config will apply only for thefrontendwhich is what would be expected anyway given the flag-querier.frontend-client.tls-server-name, the scheduler will now use-querier.scheduler-client.tls-server-nameChecklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]