-
Notifications
You must be signed in to change notification settings - Fork 495
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
Add ring-based service discovery to query-scheduler #2957
Add ring-based service discovery to query-scheduler #2957
Conversation
5e4f678
to
c6611d0
Compare
if err := cfg.FrontendV2.Validate(log); err != nil { | ||
return err | ||
} | ||
if err := cfg.QueryMiddleware.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.
Note to reviewers: previously it was directly called from pkg/mimir/mimir.go
.
t.Cfg.Worker.MaxConcurrentRequests = t.Cfg.Querier.EngineConfig.MaxConcurrent | ||
t.Cfg.Worker.QuerySchedulerDiscovery = t.Cfg.QueryScheduler.ServiceDiscovery |
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.
Note to reviewers: moved above to let the new utility function t.Cfg.Worker.IsFrontendOrSchedulerConfigured()
to work correctly (it requires QuerySchedulerDiscovery
to be set).
ad68f32
to
64d4534
Compare
docs/sources/operators-guide/architecture/components/query-scheduler/index.md
Outdated
Show resolved
Hide resolved
docs/sources/operators-guide/architecture/components/query-scheduler/index.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
… or query-scheduler are in use in the querier worker initialization Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
64d4534
to
8f1c586
Compare
Thanks @replay. I fixed the typos you reported. |
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.
Solid code! My comments are mostly doc related.
We have three interfaces which are exactly the same, and need to be in sync:
worker.serviceDiscoveryNotifications
schedulerdiscovery.Notifications
util.DNSNotifications
This is a design smell. I wonder if ringServiceDiscovery
service should live next to DNSWatcher, and we only use single DNSNotifications
interface?
(Unrelated question: I am also wondering if more Go-like approach would be to send notifications over channel, instead of using Listener. That way client code doesn't need to adhere to any interface, and just consume updates. But that's a change for different PR.)
docs/sources/operators-guide/architecture/components/query-scheduler/index.md
Outdated
Show resolved
Hide resolved
@@ -52,6 +52,7 @@ This document groups API endpoints by service. Note that the API endpoints are e | |||
| [Label values cardinality](#label-values-cardinality) | Querier, Query-frontend | `GET, POST <prometheus-http-prefix>/api/v1/cardinality/label_values` | | |||
| [Build information](#build-information) | Querier, Query-frontend, Ruler | `GET <prometheus-http-prefix>/api/v1/status/buildinfo` | | |||
| [Get tenant ingestion stats](#get-tenant-ingestion-stats) | Querier | `GET /api/v1/user_stats` | | |||
| [Query-scheduler ring status](#query-scheduler-ring-status) | Query-scheduler | `GET /query-scheduler/ring` | |
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.
Why do query-frontends and queriers not expose this ring?
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 is a good feedback. As we discussed in a previous call, let me open an issue and then try to address it in a consistent way in Mimir (e.g. store-gateway ring is currently not exposed by queriers, and doc about /ingester/ring
is missing to mention it's also exposed by distributors, queriers, rulers).
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: #2977
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
The reason of this design is because of the next follow up PR. In the next PR I will introduce the "shard size" for the query-scheduler and I will need to notify active and inactive query-scheduler instances (because the querier workers will also need to open 1 connection to each inactive one, to ensure queues are drained). This means the service discovery notification interface for the query-scheduler will be a custom one, reason why I've defined What's about I think this is something we can revisit in the next PR once the final logic will be in place.
The main reason why I kept using the previous interface is to reduce the number of changes to the minimum, and hopefully reduce the risk of introducing regressions in this PR (when using the DNS service discovery). |
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 addressing my feedback.
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.
Left a couple of nitpicks, although I see it's already merged.
query_scheduler: | ||
# Comment to not use the query-scheduler and enqueue queries directly in the query-frontend. | ||
service_discovery_mode: "ring" | ||
ring: | ||
kvstore: | ||
store: consul | ||
consul: | ||
host: consul:8500 | ||
|
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.
Do we expect someone to run it this way? I mean, users would need to explicitly do -target=all,query-scheduler
like you did in docker-compose.yml
, right? Sounds like something advanced for someone who is aiming for the simplest deployment.
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.
Do we expect someone to run it this way?
No, we don't. It was introduced to test manually it in monolithic mode too (e.g. the were some bugs in my first implementation). If you prefer, I can revert it.
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'd prefer to remove it in future PRs, as this now doesn't test the usual scenario.
OTOH, I wonder if it would make sense to have query-scheduler enabled by default in monolith in future versions, with memberlist ring?
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'd prefer to remove it in future PRs, as this now doesn't test the usual scenario.
See: #2987
OTOH, I wonder if it would make sense to have query-scheduler enabled by default in monolith in future versions, with memberlist ring?
Yes, but the first pre-requisite would be switch to ring-based service discovery by default. We'll first get some confidence with the query-scheduler ring, then offer a migration path from DNS to ring, finally switch default to ring. Will be a long journey.
// serviceDiscoveryNotifications is the interface implemented by the component receiving service discovery notifications. | ||
type serviceDiscoveryNotifications interface { | ||
AddressAdded(address string) | ||
AddressRemoved(address string) | ||
} |
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.
We're already importing schedulerdiscovery
, so why not using schedulerdiscovery.Notifications
interface here?
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 see your comment about this:
I could have just used schedulerdiscovery.Notifications interface, but looks queried using it for query-frontends discovery (how is query-scheduler related at all if you're not running it?) so I preferred to duplicate it given wasn't too complex.
I think we can just reuse it.
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: #2957 (comment)
What's about worker.serviceDiscoveryNotifications? The querier worker need to discover either query-frontends or query-schedulers. I could have just used schedulerdiscovery.Notifications interface, but looks queried using it for query-frontends discovery (how is query-scheduler related at all if you're not running it?) so I preferred to duplicate it given wasn't too complex.
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 think we can just reuse it.
Ok. Let's see the next PR which will change this interface, and then we can take a better decision on how to proceed on this interface.
What this PR does
In this PR I'm proposing to add a ring-based service discovery mechanism to let query-frontends and queriers find query-scheduler addresses. The rationale is explained in #2950 and this work would also be preparatory for scaling the backend component in the read-write deployment mode (see #2749)
How it works:
-query-scheduler.service-discovery-mode=ring
and eventually configure the ring backend (nothing to do if you use the default memberlist)Notes to reviewers:
Which issue(s) this PR fixes or relates to
Fixes #2950
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]