query: add endpointset flow #4421

hitanshu-mehta · 2021-07-06T21:21:08Z

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

Changes

This PR is part of implementing the Unified Endpoint Proposal.
I have implemented Info gRPC Service in Querier/Sidecar/StoreGW: Implement Info service and add --endpoint flag in Querier #4282 and also added --endpoint flag. But new endpoint flow is baked into the existing storeset flow. This can make the migration process difficult as discussed here. So I have added new endpoint flow in a separate file.
Most of the code is copied from storeset.go but I have tried to make it more generic.

Verification

Added unit tests. ( Again similar to unit tests of storeset but made more generic and covered some additional scenarios relevant to endpoint )

pkg/query/endpointset.go

bill3tt · 2021-07-13T17:38:16Z

@hitanshu-mehta just an FYI your first link is broken - it should be https://thanos.io/tip/proposals-accepted/202101-endpoint-discovery.md/

hitanshu-mehta · 2021-07-13T17:50:30Z

@hitanshu-mehta just an FYI your first link is broken - it should be https://thanos.io/tip/proposals-accepted/202101-endpoint-discovery.md/

Changed it. Thank you!

bill3tt

Today has been a day of really big PRs 😅

I'm afraid that I'm not currently familiar enough with storeset.go to be able to review this PR will any level of detail or confidence.

If there is a lot of code in common, and we plan on deprecating --store and --rule eventually, I'm wondering whether we can make storeset a special case of endpointset or vice versa?

pkg/query/endpointset.go

kakkoyun

I have skimmed it. It looks good. I need to spare more time to review the details.

pkg/query/endpointset.go

kakkoyun

@hitanshu-mehta This looks good. But I failed to find where do we actually use the new EndpointSet? Will there be a follow-up PR? If so we should mention this. Even better let's utilize it in this PR.

hitanshu-mehta · 2021-08-12T14:44:09Z

Earlier I opened #4282 PR, where a new endpoint flow was baked into storeset, which made it difficult to understand and remove old code. here is my discussion regarding this with @bwplotka.

Yes, there will be follow-up PR or I will rebase and update my old PR. My plan is to follow this deprecation plan. Hence, I am doing changes according to that plan. Please let me know if I can improve something :)

bwplotka

I love it, LGTM, some suggestions only.

BUT! I would remove storeset and start using new endpointset (with old flags purely, don't change that yet) in this PR.

Reasons:

We can verify if endpointset we work on on now works as designed
We don't commit code we don't use
Reduce migration duration
WDYT? @hitanshu-mehta ? Up for challenge? I am happy to work with you and do reviews everyday so this can land for Thanos v0.22.0 which we plan to cut on Monday.

pkg/query/endpointset.go

bwplotka · 2021-08-17T15:09:03Z

pkg/query/endpointset.go

+}
+
+// StrictStatic returns true if the endpoint has been statically defined and it is under a strict mode.
+func (es *grpcEndpointSpec) StrictStatic() bool {


Suggested change

func (es *grpcEndpointSpec) StrictStatic() bool {

func (es *grpcEndpointSpec) IsStrictStatic() bool {

Are we sure we are not creating something for tests? I think I did that for storeset. Otherwise we can remove yea

Yes. We only have one implementation which is also used in tests.

pkg/query/endpointset.go

bwplotka · 2021-08-17T15:10:00Z

pkg/query/endpointset.go

+
+// endpointSetNodeCollector is a metric collector reporting the number of available storeAPIs for Querier.
+// A Collector is required as we want atomic updates for all 'thanos_store_nodes_grpc_connections' series.
+// TODO(hitanshu-mehta) Currently,only collecting metrics of storeAPI. Make this struct generic.


Suggested change

// TODO(hitanshu-mehta) Currently,only collecting metrics of storeAPI. Make this struct generic.

// TODO(hitanshu-mehta) Currently, only collecting metrics of storeAPI. Make this struct generic.

Why we cannot make this struct genereric? What about using storeSet node collector then instead of creating new code that has to be refactored in new PR

This PR was already very big, so I thought it would be better if I do this in a separate PR.

Is it fine if I made this struct generic in this PR?

Up to you, as long as there are not duplicated storeset vs endpointset, all good

pkg/query/endpointset.go

bwplotka · 2021-08-17T16:19:18Z

pkg/query/endpointset.go

+		}
+
+		if er.HasMetricMetadataAPI() {
+			level.Info(e.logger).Log("msg", "adding new MetricMetadataAPI to query endpointset", "address", addr)


hmhmh, we are not adding anything here ;p just logging I think it might confuse people if some component has all 5 APIs implemented. Users will think we added 5 components but we added one. Maybe more code, but I combine all into one message: added component with A, B ,C APIs.

WDYT? 🤗

Yes, sounds good :)

pkg/query/endpointset.go

hitanshu-mehta · 2021-08-17T17:19:37Z

@bwplotka Agree with the reasons. I will do the required changes :)

WDYT? @hitanshu-mehta ? Up for challenge? I am happy to work with you and do reviews everyday so this can land for Thanos v0.22.0 which we plan to cut on Monday.

Up for the challenge!! Thank you the help and lets merge it by Monday 🚀

Signed-off-by: Hitanshu Mehta <[email protected]>

sonatype-lift · 2021-08-18T11:26:27Z

pkg/query/endpointset_test.go

+			return nil, err
+		}
+
+		srv := grpc.NewServer()


opt.semgrep.go.grpc.security.grpc-server-insecure-connection.grpc-server-insecure-connection: Found an insecure gRPC connection. This allows for a connection without encryption to this server. A malicious attacker could tamper with the gRPC message, which could compromise the machine.
(at-me in a reply with help or ignore)

Signed-off-by: Hitanshu Mehta <[email protected]>

bwplotka

Super nice work. I have small comments only, otherwise LGTM. Let's go before release 💪🏽

bwplotka · 2021-08-25T18:29:57Z

pkg/query/endpointset.go

+func (es *grpcEndpointSpec) Metadata(ctx context.Context, client *endpointClients) (*endpointMetadata, error) {
+	resp, err := client.info.Info(ctx, &infopb.InfoRequest{}, grpc.WaitForReady(true))
+	if err != nil {
+		// Call Info method of StoreAPI, this way querier will be able to discovery old components not exposing InfoAPI.


bwplotka · 2021-08-25T18:31:22Z

pkg/query/endpointset.go

+		metadata, err := es.getMetadataUsingStoreAPI(ctx, client.store)
+		if err != nil {
+			return nil, errors.Wrapf(err, "fetching info from %s", es.addr)
+		}
+		return metadata, nil


Suggested change

metadata, err := es.getMetadataUsingStoreAPI(ctx, client.store)

if err != nil {

return nil, errors.Wrapf(err, "fetching info from %s", es.addr)

}

return metadata, nil

metadata, merr := es.getMetadataUsingStoreAPI(ctx, client.store)

if merr != nil {

return nil, errors.Wrapf(merr, "fallback fetching info from %s after err: %v", es.addr, err)

}

return metadata, nil

bwplotka · 2021-08-25T18:32:37Z

pkg/query/endpointset.go

+
+// endpointSetNodeCollector is a metric collector reporting the number of available storeAPIs for Querier.
+// A Collector is required as we want atomic updates for all 'thanos_store_nodes_grpc_connections' series.
+// TODO(hitanshu-mehta) Currently,only collecting metrics of storeAPI. Make this struct generic.


Up to you, as long as there are not duplicated storeset vs endpointset, all good

bwplotka · 2021-08-25T18:33:06Z

pkg/query/endpointset.go

+type EndpointSet struct {
+	logger log.Logger
+
+	// Endpoint specifications can change dynamically. If some store is missing from the list, we assuming it is no longer


Suggested change

// Endpoint specifications can change dynamically. If some store is missing from the list, we assuming it is no longer

// Endpoint specifications can change dynamically. If some component is missing from the list, we assume it is no longer

bwplotka · 2021-08-25T18:33:25Z

pkg/query/endpointset.go

+	logger log.Logger
+
+	// Endpoint specifications can change dynamically. If some store is missing from the list, we assuming it is no longer
+	// accessible and we close gRPC client for it.


Suggested change

// accessible and we close gRPC client for it.

// accessible and we close gRPC client for it, unless it is strict.

bwplotka · 2021-08-25T18:33:42Z

pkg/query/endpointset.go

+		gRPCInfoCallTimeout:      5 * time.Second,
+		endpoints:                make(map[string]*endpointRef),
+		endpointStatuses:         make(map[string]*EndpointStatus),
+		unhealthyEndpointTimeout: unhealthyStoreTimeout,


Suggested change

unhealthyEndpointTimeout: unhealthyStoreTimeout,

unhealthyEndpointTimeout: unhealthyEndpointTimeout,

bwplotka · 2021-08-25T18:33:49Z

pkg/query/endpointset.go

+	reg *prometheus.Registry,
+	endpointSpecs func() []EndpointSpec,
+	dialOpts []grpc.DialOption,
+	unhealthyStoreTimeout time.Duration,


Suggested change

unhealthyStoreTimeout time.Duration,

unhealthyEndpointTimeout time.Duration,

bwplotka · 2021-08-25T18:34:51Z

pkg/query/endpointset.go

+	}
+}
+
+// TODO(bwplotka): Consider moving storeRef out of this package and renaming it, as it also supports rules API.


We can remove it now?

bwplotka · 2021-08-25T18:35:37Z

pkg/query/endpointset.go

+		clients.store = storepb.NewStoreClient(er.cc)
+		er.StoreClient = clients.store
+	} else {
+		er.clients.store = nil


Why we need this?

Because here when we see the endpoint for the first time we assume the StoreAPI is exposed by that endpoint (which may not be true for some ruler) and we create a store API client because as a fallback we might have to call info method of storeAPI.
In this step, I am setting it to null if we find out that the store API is not exposed.

right, some comment would be nice here then

bwplotka · 2021-08-25T18:36:23Z

pkg/query/endpointset_test.go

+						}
+						return endpointSpec
+					},
+					expectedStores:         4, // sidecar + querier + receiver + storeGW


Nice work on those 👍🏽

Thank you :)

Signed-off-by: Hitanshu Mehta <[email protected]>

bwplotka

👍🏽

bwplotka · 2021-08-25T19:23:44Z

pkg/query/endpointset.go

+		clients.store = storepb.NewStoreClient(er.cc)
+		er.StoreClient = clients.store
+	} else {
+		er.clients.store = nil


right, some comment would be nice here then

bwplotka · 2021-08-25T19:25:37Z

e2e fail might be relevant:

Rescheduled, let's see if it repeats.

Signed-off-by: Hitanshu Mehta <[email protected]>

hitanshu-mehta · 2021-08-26T14:01:41Z

@bwplotka Yes, there was a bug. I have fixed it. Now, e2e tests have passed but units and documentation checks have failed. I tried, but was not able to find the reason :( Can you please help?

GiedriusS

With this PR, I get a bunch of "Duplicate store address is provided" spam:

Rgp 31 13:08:57 XXX thanos_endpoint[166897]: level=info ts=2021-08-31T13:08:57.843619303Z caller=endpointset.go:360 component=endpointset msg="adding new sidecar with [storeAPI rulesAPI exemplarsAPI targetsAPI MetricMetadataAPI]" address=1.2.3.4:10901 extLset="{dc=\"aaaa\", www=\"1\"}"

...
Rgp 31 13:09:02 XXX thanos_endpoint[166897]: level=warn ts=2021-08-31T13:09:02.833746591Z caller=query.go:637 msg="Duplicate store address is provided" addr=1.2.3.4:10901

I have both --store and --rule pointing to the same IP addresses (Sidecar nodes). Removing --rule fixes this problem. Presumably, this happens because the same endpoint spec is added for all flags. Maybe we can simply improve the logging so that a message would be printed only if duplicate nodes have been specified with the same flags? The deduplication should still happen at the end, though, but without any logging.

Perhaps we could fix this before merging @hitanshu-mehta ?

Signed-off-by: Hitanshu Mehta <[email protected]>

GiedriusS

LGTM, great work 👍

Signed-off-by: Hitanshu Mehta <[email protected]>

bwplotka · 2021-08-31T16:31:44Z

🎉

hanjm · 2021-09-09T04:33:04Z

🎉 hi, could it support file sd?

* Create endpoint flow Signed-off-by: Hitanshu Mehta <[email protected]> * add unit test for endpointSet Signed-off-by: Hitanshu Mehta <[email protected]> * lint fixes Signed-off-by: Hitanshu Mehta <[email protected]> * fix typo Signed-off-by: Hitanshu Mehta <[email protected]> * remove code smells Signed-off-by: Hitanshu Mehta <[email protected]> * start using endpointset instead of storeset Signed-off-by: Hitanshu Mehta <[email protected]> * remove storeset Signed-off-by: Hitanshu Mehta <[email protected]> * minor nits Signed-off-by: Hitanshu Mehta <[email protected]> * Fix failing e2e tests Signed-off-by: Hitanshu Mehta <[email protected]> * improve logging Signed-off-by: Hitanshu Mehta <[email protected]> * fix comment Signed-off-by: Hitanshu Mehta <[email protected]>

sonatype-lift bot reviewed Jul 6, 2021

View reviewed changes

pkg/query/endpointset.go Outdated Show resolved Hide resolved

bill3tt reviewed Jul 13, 2021

View reviewed changes

pkg/query/endpointset.go Outdated Show resolved Hide resolved

kakkoyun reviewed Jul 22, 2021

View reviewed changes

pkg/query/endpointset.go Outdated Show resolved Hide resolved

pkg/query/endpointset.go Show resolved Hide resolved

hitanshu-mehta requested a review from kakkoyun August 2, 2021 13:51

kakkoyun reviewed Aug 12, 2021

View reviewed changes

bwplotka previously approved these changes Aug 17, 2021

View reviewed changes

hitanshu-mehta dismissed bwplotka’s stale review via 8e23c4a August 18, 2021 05:51

hitanshu-mehta force-pushed the endpoint branch from 8e23c4a to c2afd79 Compare August 18, 2021 05:52

hitanshu-mehta added 5 commits August 18, 2021 16:15

Create endpoint flow

baeecc7

Signed-off-by: Hitanshu Mehta <[email protected]>

add unit test for endpointSet

e5ba86c

Signed-off-by: Hitanshu Mehta <[email protected]>

lint fixes

9019977

Signed-off-by: Hitanshu Mehta <[email protected]>

fix typo

219f4f9

Signed-off-by: Hitanshu Mehta <[email protected]>

remove code smells

171231a

Signed-off-by: Hitanshu Mehta <[email protected]>

hitanshu-mehta force-pushed the endpoint branch from c2afd79 to 171231a Compare August 18, 2021 10:51

sonatype-lift bot reviewed Aug 18, 2021

View reviewed changes

start using endpointset instead of storeset

7a33e85

Signed-off-by: Hitanshu Mehta <[email protected]>

hitanshu-mehta force-pushed the endpoint branch from 0c01cdb to b7fc894 Compare August 21, 2021 08:59

remove storeset

291bcdd

Signed-off-by: Hitanshu Mehta <[email protected]>

hitanshu-mehta force-pushed the endpoint branch from b7fc894 to 291bcdd Compare August 21, 2021 11:57

hitanshu-mehta requested a review from bwplotka August 21, 2021 11:58

bwplotka previously approved these changes Aug 25, 2021

View reviewed changes

minor nits

9696234

Signed-off-by: Hitanshu Mehta <[email protected]>

hitanshu-mehta dismissed bwplotka’s stale review via 9696234 August 25, 2021 19:10

bwplotka previously approved these changes Aug 25, 2021

View reviewed changes

bwplotka enabled auto-merge (squash) August 25, 2021 19:24

Fix failing e2e tests

873f39d

Signed-off-by: Hitanshu Mehta <[email protected]>

auto-merge was automatically disabled August 26, 2021 12:07
Head branch was pushed to by a user without write access

hitanshu-mehta dismissed bwplotka’s stale review via 873f39d August 26, 2021 12:07

GiedriusS previously approved these changes Aug 31, 2021

View reviewed changes

improve logging

8d0f0ff

Signed-off-by: Hitanshu Mehta <[email protected]>

hitanshu-mehta dismissed GiedriusS’s stale review via 8d0f0ff August 31, 2021 14:07

GiedriusS previously approved these changes Aug 31, 2021

View reviewed changes

hitanshu-mehta dismissed GiedriusS’s stale review via a99d374 August 31, 2021 15:22

fix comment

2f8ae5e

Signed-off-by: Hitanshu Mehta <[email protected]>

hitanshu-mehta force-pushed the endpoint branch from a99d374 to 2f8ae5e Compare August 31, 2021 15:23

GiedriusS approved these changes Aug 31, 2021

View reviewed changes

GiedriusS enabled auto-merge (squash) August 31, 2021 15:25

bwplotka disabled auto-merge August 31, 2021 16:31

bwplotka merged commit 8862ad5 into thanos-io:main Aug 31, 2021

GiedriusS mentioned this pull request Sep 6, 2021

Cut release 0.23.0-rc.0 #4625

Merged

matej-g mentioned this pull request Sep 24, 2021

Endpoint set on query is causing increase in gRPC error rate #4699

Closed

hitanshu-mehta mentioned this pull request Oct 20, 2021

Querier/Sidecar/StoreGW: Implement Info service and add --endpoint flag in Querier #4282

Merged

2 tasks

	func (es *grpcEndpointSpec) StrictStatic() bool {
	func (es *grpcEndpointSpec) IsStrictStatic() bool {

	// TODO(hitanshu-mehta) Currently,only collecting metrics of storeAPI. Make this struct generic.
	// TODO(hitanshu-mehta) Currently, only collecting metrics of storeAPI. Make this struct generic.

	// Endpoint specifications can change dynamically. If some store is missing from the list, we assuming it is no longer
	// Endpoint specifications can change dynamically. If some component is missing from the list, we assume it is no longer

	// accessible and we close gRPC client for it.
	// accessible and we close gRPC client for it, unless it is strict.

	unhealthyEndpointTimeout: unhealthyStoreTimeout,
	unhealthyEndpointTimeout: unhealthyEndpointTimeout,

	unhealthyStoreTimeout time.Duration,
	unhealthyEndpointTimeout time.Duration,

query: add endpointset flow #4421

query: add endpointset flow #4421

Conversation

hitanshu-mehta commented Jul 6, 2021 • edited Loading

Changes

Verification

bill3tt commented Jul 13, 2021

hitanshu-mehta commented Jul 13, 2021

bill3tt left a comment • edited Loading

Choose a reason for hiding this comment

kakkoyun left a comment

Choose a reason for hiding this comment

kakkoyun left a comment

Choose a reason for hiding this comment

hitanshu-mehta commented Aug 12, 2021 • edited Loading

bwplotka 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

hitanshu-mehta Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

hitanshu-mehta commented Aug 17, 2021

sonatype-lift bot Aug 18, 2021

Choose a reason for hiding this comment

bwplotka 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

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

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Aug 25, 2021

hitanshu-mehta commented Aug 26, 2021

GiedriusS left a comment • edited Loading

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

bwplotka commented Aug 31, 2021

hanjm commented Sep 9, 2021

hitanshu-mehta commented Jul 6, 2021 •

edited

Loading

bill3tt left a comment •

edited

Loading

hitanshu-mehta commented Aug 12, 2021 •

edited

Loading

hitanshu-mehta Aug 17, 2021 •

edited

Loading

GiedriusS left a comment •

edited

Loading