Implement endpoint groups #5548

fpetkovski · 2022-07-28T08:11:47Z

This draft PR illustrates how an HA group of endpoints can be configured in Thanos Query.

Looking for feedback on the approach!

Fixes #5335

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Add the flags endpoint-group and endpoint-group-strict to Thanos Query for load balancing instead of fanout.

Verification

fpetkovski · 2022-07-28T09:30:57Z

One downside of using load balancing from gRPC is that dns resolution is done once during startup and is cached for a very long time, potentially forever. Addresses will be re-resolved when the downstream target goes away.

This can be problematic if the endpoing group is scaled out and new targets are added. The recommended workaround here is to set max_connection_age to something like 5m which will cause periodic dns resolution, but also a complete recreation of all connections to the endpoint group.

fpetkovski · 2022-07-29T11:22:18Z

cc @saswatamcode @SrushtiSapkale

saswatamcode

Thanks a lot for this! ✨

Some comments!

saswatamcode · 2022-08-01T05:31:03Z

pkg/extgrpc/client.go

+	serviceConfig := `
+{
+  "loadBalancingPolicy":"round_robin",
+  "retryPolicy": {


Perhaps this should be configurable via flags?

Yes, this could be a good idea. The new endpoint config would also help here.

Can we add this flag in this PR, with this particular config as the default? 🙂

This policy is really an implementation detail, I don't think it makes sense to expose it to the end user. The retry always has to be there so that the client will re-resolve dns endpoints when the one it's connected to goes away. This is how HA is implemented in gRPC.

pkg/query/endpointset.go

stale · 2022-10-01T10:55:39Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

fpetkovski · 2022-11-02T05:57:36Z

I am not sure what the status of the LFX project is, but this could be a short term solution that gets us unblocked until we have a better implementation.

sonatype-lift · 2022-11-02T06:14:00Z

cmd/thanos/query.go

@@ -11,6 +11,8 @@
 	"strings"
 	"time"

+	"google.golang.org/grpc"


typecheck:
other declaration of grpc

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

stale · 2023-01-07T21:34:42Z

Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days.
Do you mind updating us on the status? Is there anything we can help with? If you plan to still work on it, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next week, this issue will be closed (we can always reopen a PR if you get back to this!). Alternatively, use remind command if you wish to be reminded at some point in future.

trevorriles · 2023-01-23T14:24:28Z

Currently very interested in this work. What are the blockers for this being merged?

fpetkovski · 2023-01-23T14:27:46Z

There was an LFX project that was a superset of this functionality: #5505

Maybe @saswatamcode or @matej-g have some information on how far that got and whether it makes sense to continue working on this PR.

trevorriles · 2023-01-23T14:28:57Z

There was an LFX project that was a superset of this functionality: #5505

Maybe @saswatamcode or @matej-g have some information on how far that got and whether it makes sense to continue working on this PR.

Thanks for the link!

saswatamcode · 2023-01-24T05:08:16Z

Yes, we made some progress on that end, but I think there were some more items left as well. This is also related to #2600

I'll try to organize them a bit, and move this forward! 🙂

fpetkovski · 2023-01-24T07:52:16Z

I see that HA endpoints are out of scope of that proposal: https://github.com/thanos-io/thanos/pull/5505/files#diff-5dad1d444b473dcd0b72f4770b3ba03089499cfaf027205c83914f63124644e5R38. Are you looking to cover them in your work?

fpetkovski · 2023-02-03T17:54:37Z

@SuperQ brought this feature up yesterday during contributor hours. Do we want to proceed with something like this until we have more extensive endpoint configuration? It would be great to not have to run envoy for load balancing gRPC connections since it's another component in the query path that can break. cc @saswatamcode @bwplotka

saswatamcode

Thanks! I think it would be great to proceed with this until we have a full-fledged endpoint configuration! 💪🏻

Just some minor comments on this!

saswatamcode · 2023-02-09T04:10:14Z

pkg/extgrpc/client.go

+	serviceConfig := `
+{
+  "loadBalancingPolicy":"round_robin",
+  "retryPolicy": {


Can we add this flag in this PR, with this particular config as the default? 🙂

saswatamcode · 2023-02-09T04:20:50Z

cmd/thanos/query.go

@@ -128,6 +128,9 @@ func registerQuery(app *extkingpin.App) {
 	endpoints := extkingpin.Addrs(cmd.Flag("endpoint", "Addresses of statically configured Thanos API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect Thanos API servers through respective DNS lookups.").
 		PlaceHolder("<endpoint>"))

+	endpointGroups := extkingpin.Addrs(cmd.Flag("endpoint-group", "DNS name of statically configured Thanos API server groups (repeatable). Targets resolved from the DNS name will be queried in a round-robin instead of a fanout manner. This flag should be used when connecting a Thanos Query to HA groups of Thanos components.").


Suggested change

endpointGroups := extkingpin.Addrs(cmd.Flag("endpoint-group", "DNS name of statically configured Thanos API server groups (repeatable). Targets resolved from the DNS name will be queried in a round-robin instead of a fanout manner. This flag should be used when connecting a Thanos Query to HA groups of Thanos components.").

endpointGroups := extkingpin.Addrs(cmd.Flag("endpoint-group", "DNS name of statically configured Thanos API server groups (repeatable). Targets resolved from the DNS name will be queried in a round-robin manner by default instead of a fanout manner. This flag should be used when connecting a Thanos Query to HA groups of Thanos components.").

cmd/thanos/query.go

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

saswatamcode

LGTM!

fpetkovski · 2023-02-09T08:03:17Z

I marked these flags as experimental, there could be some hidden dragons that we'll uncover over time.

docs/components/tools.md

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

benjaminhuo · 2023-03-22T10:09:38Z

cmd/thanos/query.go

@@ -128,6 +128,9 @@ func registerQuery(app *extkingpin.App) {
 	endpoints := extkingpin.Addrs(cmd.Flag("endpoint", "Addresses of statically configured Thanos API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect Thanos API servers through respective DNS lookups.").
 		PlaceHolder("<endpoint>"))

+	endpointGroups := extkingpin.Addrs(cmd.Flag("endpoint-group", "Experimental: DNS name of statically configured Thanos API server groups (repeatable). Targets resolved from the DNS name will be queried in a round-robin, instead of a fanout manner. This flag should be used when connecting a Thanos Query to HA groups of Thanos components.").


HA groups of Thanos components. means store only?

It can be any component, querier, store, exemplar etc.. as long as they implement the same APIs. So a group should not have a mix of components.

* Implement endpoint groups Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Fix imports Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Remove stray grpc option Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Mark as experimental and regenerate docs Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> --------- Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

pull-request-size bot added the size/M label Jul 28, 2022

fpetkovski force-pushed the ha-endpoints branch 3 times, most recently from 20e4953 to 6a6bdab Compare July 28, 2022 08:20

fpetkovski marked this pull request as draft July 28, 2022 08:21

fpetkovski force-pushed the ha-endpoints branch from 6a6bdab to f7e061e Compare July 28, 2022 08:32

saswatamcode previously approved these changes Aug 1, 2022

View reviewed changes

stale bot added the stale label Oct 1, 2022

fpetkovski removed the stale label Nov 2, 2022

fpetkovski marked this pull request as ready for review November 2, 2022 05:58

fpetkovski dismissed saswatamcode’s stale review via 7a6c1e3 November 2, 2022 05:58

fpetkovski force-pushed the ha-endpoints branch from f7e061e to 7a6c1e3 Compare November 2, 2022 05:58

sonatype-lift bot reviewed Nov 2, 2022

View reviewed changes

stale bot added the stale label Jan 7, 2023

stale bot removed the stale label Jan 23, 2023

fpetkovski force-pushed the ha-endpoints branch from 2d49543 to 8c75c4a Compare February 2, 2023 14:55

saswatamcode previously approved these changes Feb 9, 2023

View reviewed changes

fpetkovski added 2 commits February 9, 2023 08:16

Implement endpoint groups

f15209f

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

Fix imports

8584398

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

Remove stray grpc option

d51fd71

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

fpetkovski dismissed saswatamcode’s stale review via d51fd71 February 9, 2023 07:27

fpetkovski force-pushed the ha-endpoints branch from 8c75c4a to d51fd71 Compare February 9, 2023 07:27

saswatamcode previously approved these changes Feb 9, 2023

View reviewed changes

fpetkovski dismissed saswatamcode’s stale review via 7343447 February 9, 2023 08:03

saswatamcode reviewed Feb 9, 2023

View reviewed changes

docs/components/tools.md Outdated Show resolved Hide resolved

Mark as experimental and regenerate docs

7fe57a1

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

fpetkovski force-pushed the ha-endpoints branch from 7343447 to 7fe57a1 Compare February 9, 2023 08:24

saswatamcode approved these changes Feb 9, 2023

View reviewed changes

saswatamcode enabled auto-merge (squash) February 9, 2023 08:44

saswatamcode merged commit 6ac440a into thanos-io:main Feb 9, 2023

benjaminhuo reviewed Mar 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement endpoint groups #5548

Implement endpoint groups #5548

fpetkovski commented Jul 28, 2022 •

edited

Loading

fpetkovski commented Jul 28, 2022

fpetkovski commented Jul 29, 2022

saswatamcode left a comment

saswatamcode Aug 1, 2022

fpetkovski Aug 1, 2022

saswatamcode Feb 9, 2023

fpetkovski Feb 9, 2023

saswatamcode Feb 9, 2023

stale bot commented Oct 1, 2022

fpetkovski commented Nov 2, 2022

sonatype-lift bot Nov 2, 2022

stale bot commented Jan 7, 2023

trevorriles commented Jan 23, 2023

fpetkovski commented Jan 23, 2023

trevorriles commented Jan 23, 2023

saswatamcode commented Jan 24, 2023

fpetkovski commented Jan 24, 2023

fpetkovski commented Feb 3, 2023

saswatamcode left a comment

saswatamcode Feb 9, 2023

saswatamcode Feb 9, 2023

saswatamcode left a comment

fpetkovski commented Feb 9, 2023

benjaminhuo Mar 22, 2023

fpetkovski Mar 22, 2023

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

	endpointGroups := extkingpin.Addrs(cmd.Flag("endpoint-group", "DNS name of statically configured Thanos API server groups (repeatable). Targets resolved from the DNS name will be queried in a round-robin instead of a fanout manner. This flag should be used when connecting a Thanos Query to HA groups of Thanos components.").
	endpointGroups := extkingpin.Addrs(cmd.Flag("endpoint-group", "DNS name of statically configured Thanos API server groups (repeatable). Targets resolved from the DNS name will be queried in a round-robin manner by default instead of a fanout manner. This flag should be used when connecting a Thanos Query to HA groups of Thanos components.").

Implement endpoint groups #5548

Implement endpoint groups #5548

Conversation

fpetkovski commented Jul 28, 2022 • edited Loading

Changes

Verification

fpetkovski commented Jul 28, 2022

fpetkovski commented Jul 29, 2022

saswatamcode 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

stale bot commented Oct 1, 2022

fpetkovski commented Nov 2, 2022

sonatype-lift bot Nov 2, 2022

Choose a reason for hiding this comment

stale bot commented Jan 7, 2023

trevorriles commented Jan 23, 2023

fpetkovski commented Jan 23, 2023

trevorriles commented Jan 23, 2023

saswatamcode commented Jan 24, 2023

fpetkovski commented Jan 24, 2023

fpetkovski commented Feb 3, 2023

saswatamcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saswatamcode left a comment

Choose a reason for hiding this comment

fpetkovski commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpetkovski commented Jul 28, 2022 •

edited

Loading