Expose memberlist label configs #187

replay · 2022-07-07T19:43:16Z

This makes three changes

Switches the vendored version of memberlist to a branch in our fork where we fixed the skip-inbound-label-check (PR)
Exposes the relevant options to allow users of dskit to configure the memberlist label feature
Adds unit tests which test 4 configurations of memberlist clusters:
1. TestMultipleClients Cluster with no labels and skip-inbound-label-check disabled, expected to succeed in joining
2. TestMultipleClientsWithMixedLabelsAndExpectFailure Cluster where some members have labels and some don't and skip-inbound-label-check is disabled, this cluster is expected to fail because the members can't join each other
3. TestMultipleClientsWithMixedLabelsAndSkipLabelCheck Cluster where some members have labels and some don't and skip-inbound-label-check is enabled, this cluster is expected to succeed in joining
4. TestMultipleClientsWithSameLabelWithoutSkipLabelCheck Cluster where all members have the same label and skip-inbound-label-check is disabled, this cluster is expected to succeed in joining

The above unit tests basically test a migration scenario where we migrate an existing cluster which currently doesn't use labels to use labels by going through the following steps:

Enable skip-inbound-label-check so that members with different labels (some without any label) can join each other
Roll out a label to all processes, since this change gets rolled out slowly across the pods it is important that pods with different labels can join each other into one cluster
Disable skip-inbound-label-check

Furthermore the unit tests also verify that if skip-inbound-label-check is disabled, processes which have different labels cannot join each other, providing isolation between the processes.

Signed-off-by: Mauro Stettler <[email protected]>

aknuds1

Please see comments.

In general, I'm not sure why you modify test helpers into returning errors instead of failing on them directly? Personally I prefer for test helpers to catch errors, so their callers don't have to check for them (which increases test verbosity).

aknuds1 · 2022-07-08T07:28:19Z

kv/memberlist/memberlist_client_test.go

-	if err := casWithErr(context.Background(), t, kv, key, updateFn); err != nil {
-		t.Fatal(err)
-	}
+func cas(kv *Client, key string, updateFn func(*data) (*data, bool, error)) error {


Why the change to return an error instead of failing the test on error (as a t.Helper())? I think normally it's preferable to catch test errors in test helpers instead of returning them.

aknuds1 · 2022-07-08T07:30:52Z

kv/memberlist/memberlist_client_test.go

 func TestMultipleClients(t *testing.T) {
-	c := dataCodec{}
+	members := 10


[Nit] Might as well be a constant:

Suggested change

members := 10

const members = 10

aknuds1 · 2022-07-08T07:31:29Z

kv/memberlist/memberlist_client_test.go

-			BindAddrs: []string{"localhost"},
-			BindPort:  0, // randomize ports
+func TestMultipleClientsWithMixedLabelsAndSkipLabelCheck(t *testing.T) {
+	members := 3


[Nit] Might as well be a constant:

Suggested change

members := 3

const members = 3

aknuds1 · 2022-07-08T07:32:25Z

kv/memberlist/memberlist_client_test.go


-	var clients []*Client
+func TestMultipleClientsWithMixedLabelsAndExpectFailure(t *testing.T) {
+	members := 3


[Nit] Might as well be a constant:

Suggested change

members := 3

const members = 3

aknuds1 · 2022-07-08T07:33:04Z

kv/memberlist/memberlist_client_test.go

+}
+
+func TestMultipleClientsWithSameLabelWithoutSkipLabelCheck(t *testing.T) {
+	members := 10


[Nit] Might as well be a constant:

Suggested change

members := 10

const members = 10

aknuds1 · 2022-07-08T07:34:04Z

kv/memberlist/memberlist_client_test.go

+	configGen := func(i int) KVConfig {
+		cfg := defaultKVConfig(i)
+
+		cfg.Label = label


For consistency, should we set SkipInboundLabelCheck also here?

Suggested change

cfg.Label = label

cfg.SkipInboundLabelCheck = false

cfg.Label = label

aknuds1 · 2022-07-08T07:35:32Z

kv/memberlist/memberlist_client_test.go

+	require.NoError(t, err)
+}
+
+func testMultipleClientsWithConfigGenerator(t *testing.T, members int, configGen func(memberId int) KVConfig) error {


Why not make this into a t.Helper() that catches errors instead of returning them, it would only make the tests simpler?

Because of TestMultipleClientsWithMixedLabelsAndExpectFailure.

pracucci

Changes LGTM (modulo few nits).

kv/memberlist/memberlist_client.go

pracucci · 2022-07-08T09:55:31Z

kv/memberlist/memberlist_client_test.go

@@ -594,7 +705,11 @@ func TestMultipleClients(t *testing.T) {
 	if updates < members {
 		// in general, at least one update from each node. (although that's not necessarily true...
 		// but typically we get more updates than that anyway)
-		t.Errorf("expected to see updates, got %d", updates)
+		return fmt.Errorf("expected to see %d updates, got %d", members, updates)


Suggested change

return fmt.Errorf("expected to see %d updates, got %d", members, updates)

return fmt.Errorf("expected to see at least %d updates, got %d", members, updates)

pracucci · 2022-07-08T09:57:01Z

kv/memberlist/memberlist_client_test.go

+	require.NoError(t, err)
+}
+
+func testMultipleClientsWithConfigGenerator(t *testing.T, members int, configGen func(memberId int) KVConfig) error {


Because of TestMultipleClientsWithMixedLabelsAndExpectFailure.

pracucci · 2022-07-08T10:01:50Z

kv/memberlist/memberlist_client_test.go

-		cfg.GossipInterval = 100 * time.Millisecond
-		cfg.GossipNodes = 3
-		cfg.PushPullInterval = 5 * time.Second
+	err := testMultipleClientsWithConfigGenerator(t, members, configGen)


[nit] We can remove members variable at all.

Suggested change

err := testMultipleClientsWithConfigGenerator(t, members, configGen)

err := testMultipleClientsWithConfigGenerator(t, len(labels), configGen)

pracucci · 2022-07-08T10:02:29Z

kv/memberlist/memberlist_client_test.go

+		return cfg
+	}
+
+	err := testMultipleClientsWithConfigGenerator(t, members, configGen)


Same comment here.

pracucci · 2022-07-08T10:45:51Z

kv/memberlist/memberlist_client.go

@@ -399,6 +404,9 @@ func (m *KV) buildMemberlistConfig() (*memberlist.Config, error) {
 	mlCfg.AdvertiseAddr = m.cfg.AdvertiseAddr
 	mlCfg.AdvertisePort = m.cfg.AdvertisePort

+	mlCfg.Label = m.cfg.Label
+	mlCfg.SkipInboundLabelCheck = m.cfg.SkipInboundLabelCheck


Below we log:

level.Info(m.logger).Log("msg", "Using memberlist cluster node name", "name", mlCfg.Name)

I would move that log at the end of the function, and change it to something like:

level.Info(m.logger).Log("msg", "Using memberlist cluster label %q and node name %q", mlCfg.Label, mlCfg.Name)

When manually testing this, I found it useful seeing the actual label.

pracucci · 2022-07-08T10:54:41Z

kv/memberlist/memberlist_client.go

@@ -193,6 +196,8 @@ func (cfg *KVConfig) RegisterFlagsWithPrefix(f *flag.FlagSet, prefix string) {
 	f.BoolVar(&cfg.EnableCompression, prefix+"memberlist.compression-enabled", mlDefaults.EnableCompression, "Enable message compression. This can be used to reduce bandwidth usage at the cost of slightly more CPU utilization.")
 	f.StringVar(&cfg.AdvertiseAddr, prefix+"memberlist.advertise-addr", mlDefaults.AdvertiseAddr, "Gossip address to advertise to other members in the cluster. Used for NAT traversal.")
 	f.IntVar(&cfg.AdvertisePort, prefix+"memberlist.advertise-port", mlDefaults.AdvertisePort, "Gossip port to advertise to other members in the cluster. Used for NAT traversal.")
+	f.StringVar(&cfg.Label, prefix+"memberlist.label", mlDefaults.Label, "Label is an optional set of bytes to include on the outside of each packet and stream.")


The "label" CLI flag and YAML config option name is very generic. I'm wondering if we should call it something like "cluster-label" or "cluster-id" given we use it to authenticate the cluster ID. If so, then I would change the "skip" CLI/YAML option accordingly.

cluster-label has a nice sound to it

I think the config option (which inverts the behavior to memberlist unfortunately) which I would prefer would be

enforce-cluster-label

Although we might want to get more opinions on enforce i'm not sure how well that would be understood for non native english speakers and if there is something more clear?

(Also enforce-cluster-label would default to true IMO)

I like "cluster-label" too. While working on the jsonnet changes to configure it, it sounds nice in the config.

enforce-cluster-label

WDYT about "cluster-label-verification-enabled" instead of "enforce-cluster-label"? While doing change I've the feeling "enforce-cluster-label" may not be that clear

From Ed on Slack:

sounds good to me! I like that better

From Ed on Slack:

also you could do it then as cluster-label-verification-disabled
and the default could be false and it would then match the same semantics as the existing config in memberlist

pracucci · 2022-07-08T12:55:42Z

@replay I pick up the work to do changes to config.

Signed-off-by: Marco Pracucci <[email protected]>

…cation-disabled' Signed-off-by: Marco Pracucci <[email protected]>

pracucci · 2022-07-08T13:25:28Z

Switches the vendored version of memberlist to a branch in our fork where we fixed the skip-inbound-label-check (grafana/memberlist#3)

I merged that PR and updated go.mod in this PR.

I pick up the work to do changes to config.

I pushed few commits to apply my own comments and config renamed as discussed with Ed.

Could you review it again, please?

…gation in unit tests Signed-off-by: Marco Pracucci <[email protected]>

kv/memberlist/memberlist_client.go

Co-authored-by: Ed Welch <[email protected]>

slim-bean

LGTM with a couple fixes to the explanation of the variables (based on our recent renaming)

pracucci · 2022-07-08T13:46:08Z

Thanks @slim-bean for the edits 🤦 Applied.

kv/memberlist/memberlist_client.go

replay · 2022-07-08T14:09:04Z

In general, I'm not sure why you modify test helpers into returning errors instead of failing on them directly? Personally I prefer for test helpers to catch errors, so their callers don't have to check for them (which increases test verbosity).

I agree with you that it is nicer to just pass testing.T into the helpers and then fail directly in the helpers. But in this case I wanted to add a unit test which brings up a cluster with a configuration that is supposed to fail, so I needed to be able to check whether it returns an error without failing the whole test in the helper. That's the test TestMultipleClientsWithMixedLabelsAndExpectFailure

replay · 2022-07-08T14:09:16Z

Thanks for your changes Marco, they make sense to me

Signed-off-by: Marco Pracucci <[email protected]>

pstibrany · 2022-07-11T08:37:21Z

kv/memberlist/memberlist_client.go

@@ -140,6 +140,9 @@ type KVConfig struct {
 	AdvertiseAddr string `yaml:"advertise_addr"`
 	AdvertisePort int    `yaml:"advertise_port"`

+	ClusterLabel                     string `yaml:"cluster_label" category:"experimental"`
+	ClusterLabelVerificationDisabled bool   `yaml:"cluster_label_verification_disabled" category:"experimental"`


Shall we call this ClusterLabelVerificationDisabled and make it default to true? Most of our other options are -enabled, not -disabled. Reverse logic is confusing to users.

replay added 2 commits July 7, 2022 19:23

use version of memberlist with fix "skip label check"

dd91d22

Signed-off-by: Mauro Stettler <[email protected]>

expose memberlist label options

6ce50e8

Signed-off-by: Mauro Stettler <[email protected]>

replay marked this pull request as draft July 7, 2022 19:43

replay added 3 commits July 7, 2022 22:50

create unit tests which test clusters of using labels

7eab3e2

Signed-off-by: Mauro Stettler <[email protected]>

linter

b705fd6

Signed-off-by: Mauro Stettler <[email protected]>

more specific error check

526b435

Signed-off-by: Mauro Stettler <[email protected]>

replay marked this pull request as ready for review July 7, 2022 23:59

aknuds1 reviewed Jul 8, 2022

View reviewed changes

aknuds1 requested a review from a team July 8, 2022 07:38

pracucci self-requested a review July 8, 2022 07:55

pracucci approved these changes Jul 8, 2022

View reviewed changes

pracucci reviewed Jul 8, 2022

View reviewed changes

pracucci mentioned this pull request Jul 8, 2022

Add memberlist cluster label support to jsonnet grafana/mimir#2349

Merged

3 tasks

pracucci added 3 commits July 8, 2022 14:57

Apply suggestions from code review

d9d2440

Code review changes

dc20201

Signed-off-by: Marco Pracucci <[email protected]>

Renamed 'cluster-label-verification-enabled' to 'cluster-label-verifi…

9fc085b

…cation-disabled' Signed-off-by: Marco Pracucci <[email protected]>

Increase the number of nodes to gossip to, in order to speed up propa…

2810763

…gation in unit tests Signed-off-by: Marco Pracucci <[email protected]>

slim-bean reviewed Jul 8, 2022

View reviewed changes

kv/memberlist/memberlist_client.go Outdated Show resolved Hide resolved

slim-bean reviewed Jul 8, 2022

View reviewed changes

kv/memberlist/memberlist_client.go Outdated Show resolved Hide resolved

Apply suggestions from code review

a9c66a8

Co-authored-by: Ed Welch <[email protected]>

slim-bean approved these changes Jul 8, 2022

View reviewed changes

pracucci reviewed Jul 8, 2022

View reviewed changes

kv/memberlist/memberlist_client.go Outdated Show resolved Hide resolved

Update kv/memberlist/memberlist_client.go

b451f41

pracucci merged commit 99f3d00 into main Jul 8, 2022

pracucci deleted the expose_memberlist_label_configs branch July 8, 2022 14:10

pracucci added a commit that referenced this pull request Jul 8, 2022

Add missing CHANGELOG entry for PR #187

35f20bc

Signed-off-by: Marco Pracucci <[email protected]>

pracucci mentioned this pull request Jul 8, 2022

Add missing CHANGELOG entry for PR #187 #188

Merged

2 tasks

replay pushed a commit that referenced this pull request Jul 8, 2022

Add missing CHANGELOG entry for PR #187 (#188)

9c29dc5

Signed-off-by: Marco Pracucci <[email protected]>

pstibrany reviewed Jul 11, 2022

View reviewed changes

pracucci mentioned this pull request Jul 13, 2022

Fix memberlist log #191

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose memberlist label configs #187

Expose memberlist label configs #187

replay commented Jul 7, 2022 •

edited

Loading

aknuds1 left a comment •

edited

Loading

aknuds1 Jul 8, 2022

aknuds1 Jul 8, 2022

aknuds1 Jul 8, 2022

aknuds1 Jul 8, 2022 •

edited

Loading

aknuds1 Jul 8, 2022

aknuds1 Jul 8, 2022

aknuds1 Jul 8, 2022

pracucci Jul 8, 2022

pracucci left a comment

pracucci Jul 8, 2022

pracucci Jul 8, 2022

pracucci Jul 8, 2022

pracucci Jul 8, 2022

pracucci Jul 8, 2022

pracucci Jul 8, 2022

slim-bean Jul 8, 2022

slim-bean Jul 8, 2022

pracucci Jul 8, 2022

pracucci Jul 8, 2022

pracucci Jul 8, 2022

pracucci Jul 8, 2022

pracucci commented Jul 8, 2022

pracucci commented Jul 8, 2022

slim-bean left a comment

pracucci commented Jul 8, 2022

replay commented Jul 8, 2022

replay commented Jul 8, 2022

pstibrany Jul 11, 2022

	cfg.Label = label
	cfg.SkipInboundLabelCheck = false
	cfg.Label = label

	return fmt.Errorf("expected to see %d updates, got %d", members, updates)
	return fmt.Errorf("expected to see at least %d updates, got %d", members, updates)

	err := testMultipleClientsWithConfigGenerator(t, members, configGen)
	err := testMultipleClientsWithConfigGenerator(t, len(labels), configGen)

Expose memberlist label configs #187

Expose memberlist label configs #187

Conversation

replay commented Jul 7, 2022 • edited Loading

aknuds1 left a comment • edited Loading

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

aknuds1 Jul 8, 2022 • edited Loading

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

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

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

pracucci commented Jul 8, 2022

pracucci commented Jul 8, 2022

slim-bean left a comment

Choose a reason for hiding this comment

pracucci commented Jul 8, 2022

replay commented Jul 8, 2022

replay commented Jul 8, 2022

Choose a reason for hiding this comment

replay commented Jul 7, 2022 •

edited

Loading

aknuds1 left a comment •

edited

Loading

aknuds1 Jul 8, 2022 •

edited

Loading