[memberlist] Add support to join memberlist via SRV records #2788

periklis · 2020-06-25T07:32:10Z

What this PR does:

This PR enables lookup for members to join via SRV records.

Which issue(s) this PR fixes:
Follow-up on discussion thread here

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

cc @pstibrany

CHANGELOG.md

pracucci · 2020-06-25T08:58:06Z

pkg/ring/kv/memberlist/memberlist_client.go

@@ -475,6 +487,28 @@ func (m *KV) joinMembersOnStartup(ctx context.Context, members []string) error {
 	return lastErr
 }

+// Provides a simple SRV lookup to join a memberlist cluster w/o knowning members' addresses upfront.
+// Note: Overwrites flag `memberlist.join` if provided in addition to `memberlist.join-service`.
+func (m *KV) lookupMembers(srv string) error {


I'm wondering if we could reuse https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery instead of having another way to configure service discovery. The DNS service discovery we're currently using is based on Thanos one: you can see an example of its usage at pkg/querier/blocks_store_balanced_set.go.

Would it make sense to use it for gossip too?

@pracucci & @pstibrany Giving this a thought for the last couple of days, I believe it is a good idea to expand memberlist joins to use the same DNS service discovery in addition to our joinMembers flag. Although a simple net.lookupSRV is enough the use cases provided by Thanos' DNS provider make sense also for memberlists:

Joining a cluster means resolving the addresses of all members upfront. However, leaving the resolution mechanism to a third-party dependency is good from a separation of concerns perspective and adds testability. OTOH my simple net.lookupSRV would need a function pointer handle to make it testable, that's not a big deal though.

Re-Joining the cluster - e.g. periodically - should work the same as joining anew with the only difference of re-establishing connections.

AFAICS, the DNS service discovery mechanism is free of any caching of members and in addition to that the ultimate list of which members to connect to is maintained on memberlist side. In addition, reading through the hashicorp lib I don't see a blocker regarding how to resolve member addresses. WDYT?

@pracucci FWIW, using the DNS service disovery is not bound to DNS caching timeouts or so like that would demand to rejoin the cluster based on these timeouts right?

AFAICS, the DNS service discovery mechanism is free of any caching of members and in addition to that the ultimate list of which members to connect to is maintained on memberlist side. In addition, reading through the hashicorp lib I don't see a blocker regarding how to resolve member addresses. WDYT?

The way the Thanos DNS provider works is that Addresses() returns the result of the last Resolve(). For this specific use case you could just call Resolve() and then Address() in sequence.

FWIW, using the DNS service disovery is not bound to DNS caching timeouts or so like that would demand to rejoin the cluster based on these timeouts right?

I'm not sure about the question. The DNS service discovery just uses the Go's DNS Lookup under the hood, so there's real difference compared to your current implementation, except that it would be more generic and we would reuse the same mechanism we have.

WDYT?

My concern question is rather a concern of the sort: Using the thanos DNS provider do I need to implement re-joins based on DNS TTLs?

I wouldn't bring TTLs into the question here.

pstibrany

I think the logic I'd like to see is that we use dns.NewProvider, pass configured JoinMembers to (*Provider).Resolve method, and use the result ((*Provider).Addresses()) to call memberlist.Join method. If RejoinInterval in memberlist client is configured, we would periodically repeat the process.

For this to work we perhaps don't even need extra options, as it should be backwards compatible.

What do you think, what are possible problems with such approach?

pstibrany · 2020-06-26T13:11:22Z

pkg/ring/kv/memberlist/memberlist_client.go

@@ -134,6 +135,7 @@ type KVConfig struct {
 	DeadNodeReclaimTime time.Duration `yaml:"dead_node_reclaim_time"`

 	// List of members to join
+	JoinService      string              `yaml:"join_service"`


Would it make sense to use this as a boolean flag to enable SRV records lookup for names in join_members?

pstibrany · 2020-06-26T13:18:25Z

pkg/ring/kv/memberlist/memberlist_client.go

@@ -475,6 +487,28 @@ func (m *KV) joinMembersOnStartup(ctx context.Context, members []string) error {
 	return lastErr
 }

+// Provides a simple SRV lookup to join a memberlist cluster w/o knowning members' addresses upfront.
+// Note: Overwrites flag `memberlist.join` if provided in addition to `memberlist.join-service`.
+func (m *KV) lookupMembers(srv string) error {


I wouldn't bring TTLs into the question here.

periklis · 2020-06-26T15:12:16Z

I think the logic I'd like to see is that we use dns.NewProvider, pass configured JoinMembers to (*Provider).Resolve method, and use the result ((*Provider).Addresses()) to call memberlist.Join method. If RejoinInterval in memberlist client is configured, we would periodically repeat the process.

For this to work we perhaps don't even need extra options, as it should be backwards compatible.

What do you think, what are possible problems with such approach?

Indeed the boolean flag with the present JoinMembers flag will be sufficient and backward compatible.

pstibrany · 2020-06-26T15:31:35Z

Indeed the boolean flag with the present JoinMembers flag will be sufficient and backward compatible.

Maybe we can even do without it (I suggested bool flag before writing my other comment).

periklis · 2020-06-26T15:34:58Z

Indeed the boolean flag with the present JoinMembers flag will be sufficient and backward compatible.

Maybe we can even do without it (I suggested bool flag before writing my other comment).

How would that work? Scan for dns(srv|srvnoa)+ prefixes in joinMembers? Try DNS and fallback on err?

pstibrany · 2020-06-26T16:06:09Z

Maybe we can even do without it (I suggested bool flag before writing my other comment).

How would that work? Scan for dns(srv|srvnoa)+ prefixes in joinMembers? Try DNS and fallback on err?

Yes, or just look for ‘+’, which is not allowed in host names. If there is no +, we pass hostnames to memberlist directly. If there is, we do our own resolution.

periklis · 2020-06-26T16:07:10Z

Good! I think we have a plan. I will amend this PR on Monday.

periklis · 2020-06-29T08:57:54Z

@pracucci @pstibrany PTAL the last version is based on the DNS service discovery mechanism. Btw. I am trying to figure out how to write a proper integration test for this.

pracucci

Thanks @periklis for working on this! The new design LGTM. I left few comments about implementation details.

pracucci · 2020-06-30T08:49:36Z

CHANGELOG.md

@@ -159,6 +159,7 @@
  * `cortex_querier_storegateway_refetches_per_query`
 * [ENHANCEMENT] Delete requests can now be canceled #2555
 * [ENHANCEMENT] Table manager can now provision tables for delete store #2546
+* [ENHANCEMENT] Memberlist members can join cluster via SRV records. #2788


Could you move this to the "master / unreleased" section above, please?

pracucci · 2020-06-30T09:55:58Z

pkg/ring/kv/memberlist/memberlist_client.go

@@ -377,6 +382,10 @@ func (m *KV) starting(_ context.Context) error {
 var errFailedToJoinCluster = errors.New("failed to join memberlist cluster on startup")

 func (m *KV) running(ctx context.Context) error {
+	// Lookup SRV records for given service to discover members.
+	// Overwrite any given `memberlist.join` members if any found via SRV lookup.
+	m.discoverMembers()


Shouldn't we also call it before the call to m.memberlist.Join(m.cfg.JoinMembers) done in the loop below?

pracucci · 2020-06-30T09:56:37Z

pkg/ring/kv/memberlist/memberlist_client.go

@@ -475,6 +484,35 @@ func (m *KV) joinMembersOnStartup(ctx context.Context, members []string) error {
 	return lastErr
 }

+// Provides a dns-based member disovery to join a memberlist cluster w/o knowning members' addresses upfront.
+func (m *KV) discoverMembers() {
+	if len(m.cfg.JoinMembers) > 0 {


We can circuit break:

if len(m.cfg.JoinMembers) == 0 { return }

pracucci · 2020-06-30T09:57:21Z

pkg/ring/kv/memberlist/memberlist_client.go

+				if i == 0 {
+					util.WarnExperimentalUse("DNS-based memberlist service discovery")
+				}


No need to mark it as experimental. We can get rid of it.

pracucci · 2020-06-30T09:57:28Z

pkg/ring/kv/memberlist/memberlist_client.go

@@ -164,7 +165,7 @@ func (cfg *KVConfig) RegisterFlags(f *flag.FlagSet, prefix string) {
 	f.BoolVar(&cfg.RandomizeNodeName, prefix+"memberlist.randomize-node-name", true, "Add random suffix to the node name.")
 	f.DurationVar(&cfg.StreamTimeout, prefix+"memberlist.stream-timeout", 0, "The timeout for establishing a connection with a remote node, and for read/write operations. Uses memberlist LAN defaults if 0.")
 	f.IntVar(&cfg.RetransmitMult, prefix+"memberlist.retransmit-factor", 0, "Multiplication factor used when sending out messages (factor * log(N+1)).")
-	f.Var(&cfg.JoinMembers, prefix+"memberlist.join", "Other cluster members to join. Can be specified multiple times.")
+	f.Var(&cfg.JoinMembers, prefix+"memberlist.join", "Other cluster members to join. Can be specified multiple times. Or:\nEXPERIMENTAL: List in DNS Service Discovery format: https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery")


No need to mark it experimental.

Suggested change

f.Var(&cfg.JoinMembers, prefix+"memberlist.join", "Other cluster members to join. Can be specified multiple times. Or:\nEXPERIMENTAL: List in DNS Service Discovery format: https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery")

f.Var(&cfg.JoinMembers, prefix+"memberlist.join", "Other cluster members to join. It can be an IP, hostname or an entry specified in the DNS Service Discovery format (see https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery for more details).")

pracucci · 2020-06-30T09:57:46Z

pkg/ring/kv/memberlist/memberlist_client.go

+					util.WarnExperimentalUse("DNS-based memberlist service discovery")
+				}
+
+				err := m.provider.Resolve(context.TODO(), []string{member})


context.Background() instead of context.TODO()

pracucci · 2020-06-30T10:00:57Z

pkg/ring/kv/memberlist/memberlist_client.go

@@ -475,6 +484,35 @@ func (m *KV) joinMembersOnStartup(ctx context.Context, members []string) error {
 	return lastErr
 }

+// Provides a dns-based member disovery to join a memberlist cluster w/o knowning members' addresses upfront.
+func (m *KV) discoverMembers() {


I would prefer to not manipulate the config. I would suggest to pick in input m.cfg.JoinMembers and return a list of addresses and then passing these addresses to the m.joinMembersOnStartup() and m.memberlist.Join() instead of m.cfg.JoinMembers.

pstibrany

Looks good, left some small comments.

pstibrany · 2020-07-01T10:21:07Z

pkg/ring/kv/memberlist/memberlist_client.go

+	// Lookup SRV records for given service to discover members.
+	// Overwrite any given `memberlist.join` members if any found via SRV lookup.
+	members := m.discoverMembers(m.cfg.JoinMembers)
+


Should this be inside if len(m.cfg.JoinMembers) > 0 condition?

pstibrany · 2020-07-01T10:22:18Z

pkg/ring/kv/memberlist/memberlist_client.go

+	for _, member := range members {
+		if strings.Contains(member, "+") {
+
+			err := m.provider.Resolve(context.Background(), []string{member})


Should we resolve all "+" members at once, instead of one by one?

pracucci

Thanks @periklis for patiently addressing all comments. Very much appreciated! I have one last comment (I just realized it) and then we should be good to go to me.

pracucci · 2020-07-01T11:08:46Z

pkg/ring/kv/memberlist/memberlist_client.go

 func NewKV(cfg KVConfig) *KV {
 	cfg.TCPTransport.MetricsRegisterer = cfg.MetricsRegisterer
 	cfg.TCPTransport.MetricsNamespace = cfg.MetricsNamespace

 	mlkv := &KV{
 		cfg:            cfg,
+		provider:       dns.NewProvider(util.Logger, cfg.MetricsRegisterer, dns.GolangResolverType),


The Prometheus registerer passed to the DNS provider should be something like this (to keep it consistent with other components and avoid "duplicated metrics registration"):

dnsProviderRegisterer := prometheus.WrapRegistererWithPrefix("cortex_", prometheus.WrapRegistererWith(prometheus.Labels{ "name": "memberlist", }, cfg.MetricsRegisterer))

@pstibrany Make sense to you? Is it safe to use an hardcoded memberlist name?

IC, the cfg.MetricsRegisterer is only the defaultRegisterer up in the stack pkg/cortex/modules.go for memberlist. It makes sense to me to separate the provider metrics per component.

periklis · 2020-07-01T11:17:36Z

Thanks @periklis for patiently addressing all comments. Very much appreciated! I have one last comment (I just realized it) and then we should be good to go to me.

You are welcome! Although, I believe I should have catched most of them, if I put more focus time on this PR ;)

pracucci

LGTM!

pstibrany

Thank you Periklis for this work. I’ve left few small comments, but it’s almost ready.

pstibrany · 2020-07-01T18:47:21Z

pkg/ring/kv/memberlist/memberlist_client.go

 func NewKV(cfg KVConfig) *KV {
 	cfg.TCPTransport.MetricsRegisterer = cfg.MetricsRegisterer
 	cfg.TCPTransport.MetricsNamespace = cfg.MetricsNamespace

+	mr := prometheus.WrapRegistererWithPrefix("cortex_", prometheus.WrapRegistererWith(prometheus.Labels{


Nit: We can use extprom.WrapRegistererWith from Thanos to avoid wrapping cfg.MetricsRegisterer when it is nil.

pstibrany · 2020-07-01T18:53:45Z

pkg/ring/kv/memberlist/memberlist_client.go

@@ -201,6 +202,9 @@ type KV struct {

 	cfg KVConfig

+	// dns discovery provider
+	provider *dns.Provider


Nit: I would call this dnsProvider or resolver. provider sounds too generic,.

TBH, i like to read members always with the given type without duplications between type and name, e.g. provider of type dns.Provider. I admit there is a small duplication here but it is prefixed with dns. so for me not an issue. Same for resolver, it duplicates method names: resolver.resolve(members). Later reminds me on the old Java dictatorship everything is a class, whereas if I can build something as a simple function why calling it via a class with the same name.

Calling out provider of type dns.Provider resolve(members) sounds ok. WDYT?

It's not a big deal, my problem is that later in the code, m.provider seems too generic -- it's not clear what it actually provides without checking the type.

pstibrany · 2020-07-01T18:57:24Z

pkg/ring/kv/memberlist/memberlist_client.go

@@ -475,6 +489,33 @@ func (m *KV) joinMembersOnStartup(ctx context.Context, members []string) error {
 	return lastErr
 }

+// Provides a dns-based member disovery to join a memberlist cluster w/o knowning members' addresses upfront.
+func (m *KV) discoverMembers(members []string) []string {
+	if len(m.cfg.JoinMembers) == 0 {


This should be len(members).

pkg/ring/kv/memberlist/memberlist_client.go

periklis · 2020-07-02T06:59:38Z

@pstibrany Furthermore I missed to pass the context from running to discoverMembers.

pstibrany

LGTM, thanks!

pstibrany · 2020-07-02T07:09:26Z

pkg/ring/kv/memberlist/memberlist_client.go

+
+	err := m.provider.Resolve(ctx, resolve)
+	if err != nil {
+		level.Error(util.Logger).Log("msg", "failed to resolve members from SRV records", "addrs", strings.Join(resolve, ","))


I'd drop from SRV records part from the message, resolver may support different ways of resolving.

This PR enables dns based discovery for members to join via SRV records. Member lookup is only supported on memberlist initial creation and not part of any periodic re-joins. Signed-off-by: Periklis Tsirakidis <periklis@redhat.com>

pull-request-size bot added the size/M label Jun 25, 2020

periklis force-pushed the memberlist-srv-records branch from 1b6dace to 5457d8f Compare June 25, 2020 07:32

pracucci reviewed Jun 25, 2020

View reviewed changes

pstibrany reviewed Jun 26, 2020

View reviewed changes

periklis force-pushed the memberlist-srv-records branch from 5457d8f to 85c3540 Compare June 29, 2020 08:54

pracucci reviewed Jun 30, 2020

View reviewed changes

periklis force-pushed the memberlist-srv-records branch from 85c3540 to a086636 Compare June 30, 2020 14:56

pstibrany reviewed Jul 1, 2020

View reviewed changes

periklis force-pushed the memberlist-srv-records branch from a086636 to 98f117b Compare July 1, 2020 10:55

pracucci reviewed Jul 1, 2020

View reviewed changes

periklis force-pushed the memberlist-srv-records branch from 98f117b to 14110ae Compare July 1, 2020 11:16

periklis force-pushed the memberlist-srv-records branch from 14110ae to 550ab54 Compare July 1, 2020 11:19

pracucci approved these changes Jul 1, 2020

View reviewed changes

pstibrany reviewed Jul 1, 2020

View reviewed changes

periklis force-pushed the memberlist-srv-records branch from 550ab54 to 841b338 Compare July 2, 2020 07:01

pstibrany approved these changes Jul 2, 2020

View reviewed changes

periklis force-pushed the memberlist-srv-records branch from 841b338 to 474af35 Compare July 2, 2020 07:10

pstibrany merged commit 0ea5a8b into cortexproject:master Jul 2, 2020

	f.Var(&cfg.JoinMembers, prefix+"memberlist.join", "Other cluster members to join. Can be specified multiple times. Or:\nEXPERIMENTAL: List in DNS Service Discovery format: https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery")
	f.Var(&cfg.JoinMembers, prefix+"memberlist.join", "Other cluster members to join. It can be an IP, hostname or an entry specified in the DNS Service Discovery format (see https://cortexmetrics.io/docs/configuration/arguments/#dns-service-discovery for more details).")

[memberlist] Add support to join memberlist via SRV records #2788

[memberlist] Add support to join memberlist via SRV records #2788

Uh oh!

Conversation

periklis commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

periklis commented Jun 26, 2020

pstibrany commented Jun 26, 2020

periklis commented Jun 26, 2020

pstibrany commented Jun 26, 2020

periklis commented Jun 26, 2020

periklis commented Jun 29, 2020

pracucci 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

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

periklis commented Jul 1, 2020

pracucci left a comment

Choose a reason for hiding this comment

pstibrany 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

Uh oh!

periklis commented Jul 2, 2020

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

periklis commented Jun 25, 2020 •

edited

Loading