Loki: Add a ring to the query scheduler to allow discovery via the ring as an alternative to DNS #4424

slim-bean · 2021-10-07T01:44:23Z

To support this change, several packages were forked from upstream Cortex. This is intended to be a transient fork to validate these changes in Loki before upstreaming.

The first commit is forking the files, the second commit includes all the changes.

The newly added ring watcher leverages the existing infrastructure which used DNS for finding the scheduler, there are some interfaces and config options I re-used for the the new ring watcher which are now not the most consistently named.

For example, the poll interval on the ring still uses the existing DNS named config options for polling, e.x. dns_lookup_duration

I debated introducing new config options but currently am of the opinion this just makes the config more confusing and cumbersome. Instead explaining the dual functionality in the documentation.

Checklist

Documentation added
Tests updated

pkg/querier/worker_service.go

jeschkies

I like the idea but there seems to be an awful lot of copy pasta. What is the reason not to add it to Cortex?

jeschkies · 2021-10-11T14:08:47Z

pkg/lokifrontend/frontend/config.go

@@ -0,0 +1,75 @@
+package frontend


Isn't the package path stuttering? Why not pkg/loki/frontend/config.go?

this is done mostly for simplicity, lokifrontend/config.go already existed and hopefully when we upstream these changes and removed the forked code it can just remain the way it was.

jeschkies · 2021-10-11T14:10:07Z

pkg/lokifrontend/frontend/transport/roundtripper.go

+)
+
+// GrpcRoundTripper is similar to http.RoundTripper, but works with HTTP requests converted to protobuf messages.
+type GrpcRoundTripper interface {


Is this something we could add to dskit instead?

I think this could eventually be moved to dskit (as it was taken verbatim from cortex), but I don't think that should be in scope for this PR.

jeschkies · 2021-10-11T14:22:44Z

pkg/lokifrontend/frontend/v1/frontend.go

@@ -0,0 +1,352 @@
+package v1


What is the change to the Cortex version?

I don't think I understand the question.

jeschkies · 2021-10-11T14:24:29Z

pkg/lokifrontend/frontend/v2/frontend.go

+
+	"github.com/cortexproject/cortex/pkg/frontend/v2/frontendv2pb"
+	"github.com/cortexproject/cortex/pkg/querier/stats"
+	"github.com/cortexproject/cortex/pkg/ring"


How about using ring from dskit?

because Loki hasnt been updated to use the ring from dskit as a whole, that should be done as a separate step

KMiller-Grafana

Marking my approval of the docs part of this PR.
At some point, it'd be super to correct all the grammar and punctuation and improve the wording of all the configuration knobs. We'll do that in the code and then autogenerate this list of knobs.

slim-bean · 2021-10-13T23:29:49Z

I like the idea but there seems to be an awful lot of copy pasta. What is the reason not to add it to Cortex?

I tried to explain in the description, copying is intentional to prove the concept before upstreaming, also with the refactoring taking place with dskit it would be difficult to make this change upstream and revendor it within the timeframe I'd like to see this working for Loki.

KMiller-Grafana

Re-review: I've added quite a few suggestions. They are not required, but would be very nice in the documentation for this PR.

docs/sources/configuration/_index.md

trevorwhitney

Looks great, excited to get this merged!

My biggest question is around the change to module dependencies. I'm curious why we have to make the Querier and Frontend dependent on the Scheduler? If we need to do this, there are some configurations we're accounting for in other places that are no longer possible. It also seems to remove the need for the v1 Frontend altogether?

trevorwhitney · 2021-10-14T15:30:45Z

pkg/loki/loki.go

@@ -432,16 +433,16 @@ func (t *Loki) setupModuleManager() error {
 		Distributor:              {Ring, Server, Overrides, TenantConfigs},
 		Store:                    {Overrides},
 		Ingester:                 {Store, Server, MemberlistKV, TenantConfigs},
-		Querier:                  {Store, Ring, Server, IngesterQuerier, TenantConfigs},
+		Querier:                  {QueryScheduler, Store, Ring, Server, IngesterQuerier, TenantConfigs},


This part is confusing to me. Do we in fact always want to spin up a scheduler when we spin up a querier? I'm missing the technical reason why, as it seems like the worker config can take a nil for the query schedule ReadRing. Is it an order thing, where when we do have both a querier and a scheduler we need to make sure the scheduler starts up first?

By making this change is seems like we would prevent the need to check for cfg.FrontendAddress as you would always have a scheduler, so you would probably want to either use the scheduler ring or scheduler address on localhost if none other was specified.

This change seems to make it impossible to have a frontend + querier configuration, or to have frontend + scheduler pods be separate from querier pods?

Also, does that mean querierRunningStandalone will never return true anymore?

trevorwhitney · 2021-10-14T15:31:57Z

pkg/loki/loki.go

 		QueryFrontendTripperware: {Server, Overrides, TenantConfigs},
-		QueryFrontend:            {QueryFrontendTripperware},
-		QueryScheduler:           {Server, Overrides},
+		QueryFrontend:            {QueryScheduler, QueryFrontendTripperware},


Again here, I'm confused why the scheduler needs to become a dependency of the frontend?

trevorwhitney · 2021-10-14T15:33:48Z

pkg/loki/modules.go

@@ -416,7 +417,7 @@ func (t *Loki) initQueryFrontend() (_ services.Service, err error) {
 		FrontendV1:    t.Cfg.Frontend.FrontendV1,
 		FrontendV2:    t.Cfg.Frontend.FrontendV2,
 		DownstreamURL: t.Cfg.Frontend.DownstreamURL,
-	}, disabledShuffleShardingLimits{}, t.Cfg.Server.GRPCListenPort, util_log.Logger, prometheus.DefaultRegisterer)
+	}, t.queryScheduler.ReadRing(), disabledShuffleShardingLimits{}, t.Cfg.Server.GRPCListenPort, util_log.Logger, prometheus.DefaultRegisterer)


this call site is getting pretty big, how would you feel about pulling frontend.CombinedFrontendConfig into a local variable and/or moving the arguments to their own lines?

trevorwhitney · 2021-10-14T15:56:52Z

pkg/querier/worker_service.go

 	// is configured, Loki will default to using the frontend on localhost on it's own GRPC listening port.
-	if (*cfg.QuerierWorkerConfig).FrontendAddress == "" && (*cfg.QuerierWorkerConfig).SchedulerAddress == "" {
+	if cfg.SchedulerRing == nil && (*cfg.QuerierWorkerConfig).FrontendAddress == "" && (*cfg.QuerierWorkerConfig).SchedulerAddress == "" {


depending on how we resolve the dependency changes (ie. if the querier really does depend on the scheduler), then we may want to set the scheduler address instead of the frontend address on this condition.

trevorwhitney · 2021-10-14T16:01:26Z

pkg/loki/config_wrapper.go

+		if r.Worker.FrontendAddress == "" &&
+			r.Worker.SchedulerAddress == "" &&
+			r.Frontend.FrontendV2.SchedulerAddress == "" {
+			r.QueryScheduler.UseSchedulerRing = true
+		}


could we add a quick test for this in config_wrapper_test.go?

trevorwhitney · 2021-10-14T16:01:51Z

pkg/loki/config_wrapper.go

@@ -87,5 +95,6 @@ func applyMemberlistConfig(r *ConfigWrapper) {
 		r.Ingester.LifecyclerConfig.RingConfig.KVStore.Store = memberlistStr
 		r.Distributor.DistributorRing.KVStore.Store = memberlistStr
 		r.Ruler.Ring.KVStore.Store = memberlistStr
+		r.QueryScheduler.SchedulerRing.KVStore.Store = memberlistStr


could we add a quick test for this in config_wrapper_test.go? there's a few other memberlist tests so maybe just adding to an existing for copy/pasing?

trevorwhitney · 2021-10-15T23:09:11Z

@slim-bean I reworked the module dependencies, and have been testing locally where I have the read and write targets, and everything is working. I want to try a local microservices setup with this code to make sure that still works, and maybe a single binary as well. I also re-based on master so I need to fix some of the uses of the wrong log library, and then I'll push up the changes.

trevorwhitney · 2021-10-20T17:05:07Z

@slim-bean ok, now I think it's actually ready to go, got the nil pointer issue resolved when running the querier or frontend without a co-located scheduler. I'll babysit this through the CI checks, but then we should be good for some more reviews.

trevorwhitney

Approving my own changes to the module dependency issue, but would like a review from others.

…ia the ring.

…worker and querier workers to find the scheduler address as an alternative to using DNS

…ince #4312 was merged after the original fork was created.

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

…instance

A few doc fixes Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

slim-bean requested a review from a team as a code owner October 7, 2021 01:44

pull-request-size bot added the size/XXL label Oct 7, 2021

slim-bean commented Oct 7, 2021

View reviewed changes

pkg/querier/worker_service.go Outdated Show resolved Hide resolved

slim-bean requested a review from KMiller-Grafana as a code owner October 7, 2021 01:51

slim-bean force-pushed the ring-scheduler branch from 9469fcd to d5a754e Compare October 7, 2021 02:04

slim-bean mentioned this pull request Oct 7, 2021

add virtual targets read/write #4397

Closed

jeschkies reviewed Oct 11, 2021

View reviewed changes

KMiller-Grafana approved these changes Oct 12, 2021

View reviewed changes

slim-bean force-pushed the ring-scheduler branch 2 times, most recently from 087b824 to c6ae5a4 Compare October 13, 2021 23:16

slim-bean requested a review from KMiller-Grafana October 13, 2021 23:18

slim-bean force-pushed the ring-scheduler branch from c6ae5a4 to 8c14e31 Compare October 13, 2021 23:28

KMiller-Grafana approved these changes Oct 14, 2021

View reviewed changes

trevorwhitney requested changes Oct 14, 2021

View reviewed changes

trevorwhitney force-pushed the ring-scheduler branch from d6d3387 to 84fb418 Compare October 19, 2021 15:26

trevorwhitney force-pushed the ring-scheduler branch from 16ae43c to 61d035f Compare October 20, 2021 17:09

trevorwhitney approved these changes Oct 20, 2021

View reviewed changes

slim-bean and others added 10 commits October 21, 2021 14:15

Fork the frontend and scheduler so we can add support for discovery v…

fd22a6c

…ia the ring.

Add a ring to the query scheduler which is then used by the frontend …

d6e149f

…worker and querier workers to find the scheduler address as an alternative to using DNS

update some of the forked code to use dskit grpcclient and grpcutil s…

21d72da

…ince #4312 was merged after the original fork was created.

remove query scheduler dependency from querier

ddb204b

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

migrate logging package and make linter happy

4b919ad

Signed-off-by: Trevor Whitney <trevorjwhitney@gmail.com>

add SafeReadRing for instances when scheduler is not enabled on same …

52ca38f

…instance

Doc changes from code review

b164090

A few doc fixes Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>

go mod tidy on go-kit dep

9a396c8

update changelog

b443179

remove ReadRing in favor of SafeReadRing

2553167

trevorwhitney force-pushed the ring-scheduler branch from 5b83af8 to 2553167 Compare October 21, 2021 20:16

slim-bean merged commit 2b5f300 into main Oct 21, 2021

slim-bean deleted the ring-scheduler branch October 21, 2021 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki: Add a ring to the query scheduler to allow discovery via the ring as an alternative to DNS #4424

Loki: Add a ring to the query scheduler to allow discovery via the ring as an alternative to DNS #4424

slim-bean commented Oct 7, 2021 •

edited

Loading

jeschkies left a comment

jeschkies Oct 11, 2021

slim-bean Oct 13, 2021

jeschkies Oct 11, 2021

trevorwhitney Oct 20, 2021

jeschkies Oct 11, 2021

trevorwhitney Oct 20, 2021

jeschkies Oct 11, 2021

slim-bean Oct 13, 2021

KMiller-Grafana left a comment

slim-bean commented Oct 13, 2021

KMiller-Grafana left a comment

trevorwhitney left a comment

trevorwhitney Oct 14, 2021

trevorwhitney Oct 14, 2021

trevorwhitney Oct 14, 2021

trevorwhitney Oct 14, 2021

trevorwhitney Oct 14, 2021

trevorwhitney Oct 14, 2021

trevorwhitney Oct 14, 2021

trevorwhitney commented Oct 15, 2021

trevorwhitney commented Oct 20, 2021

trevorwhitney left a comment

Loki: Add a ring to the query scheduler to allow discovery via the ring as an alternative to DNS #4424

Loki: Add a ring to the query scheduler to allow discovery via the ring as an alternative to DNS #4424

Conversation

slim-bean commented Oct 7, 2021 • edited Loading

jeschkies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KMiller-Grafana left a comment

Choose a reason for hiding this comment

slim-bean commented Oct 13, 2021

KMiller-Grafana left a comment

Choose a reason for hiding this comment

trevorwhitney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevorwhitney commented Oct 15, 2021

trevorwhitney commented Oct 20, 2021

trevorwhitney left a comment

Choose a reason for hiding this comment

slim-bean commented Oct 7, 2021 •

edited

Loading