From 42fcc5d36711558d6f528fe0f44042fc5d46fb6b Mon Sep 17 00:00:00 2001 From: Julien Duchesne Date: Tue, 8 Oct 2024 21:06:53 -0400 Subject: [PATCH] Really fix flaky `TestMultipleClients` tests https://github.com/grafana/dskit/pull/599 went in a wrong direction, so I revert it here: it makes all `TestMultipleClients` tests flaky since not all members are necessarily joined after the updates Instead, to fix the `TestMultipleClientsWithMixedLabelsAndExpectFailure` test, I add a couple more labeled members, that way, even if we reach 3 updates (which happened some times), we'll never get to 5 with a single member Also, try to fix the `TestTLSServerWithLocalhostCertWithClientCertificateEnforcementUsingClientCA1` test by closing GRPC connections. Maybe this (https://github.com/golang/go/issues/36700) is the issue? --- crypto/tls/test/tls_integration_test.go | 3 +-- kv/memberlist/memberlist_client_test.go | 13 +++++-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/crypto/tls/test/tls_integration_test.go b/crypto/tls/test/tls_integration_test.go index 941cd39e3..436b48f6d 100644 --- a/crypto/tls/test/tls_integration_test.go +++ b/crypto/tls/test/tls_integration_test.go @@ -175,9 +175,8 @@ func newIntegrationClientServer( dialOptions = append([]grpc.DialOption{grpc.WithDefaultCallOptions(clientConfig.CallOptions()...)}, dialOptions...) conn, err := grpc.NewClient(grpcHost, dialOptions...) - assert.NoError(t, err, tc.name) - require.NoError(t, err, tc.name) require.NoError(t, err, tc.name) + defer conn.Close() client := grpc_health_v1.NewHealthClient(conn) diff --git a/kv/memberlist/memberlist_client_test.go b/kv/memberlist/memberlist_client_test.go index 0d14c5f76..4df4f0572 100644 --- a/kv/memberlist/memberlist_client_test.go +++ b/kv/memberlist/memberlist_client_test.go @@ -590,12 +590,16 @@ func TestMultipleClientsWithMixedLabelsAndExpectFailure(t *testing.T) { // 1) "" // 2) "label1" // 3) "label2" + // 4) "label3" + // 5) "label4" // // We expect that it won't be possible to build a memberlist cluster with mixed labels. var membersLabel = []string{ "", "label1", "label2", + "label3", + "label4", } configGen := func(i int) KVConfig { @@ -609,7 +613,7 @@ func TestMultipleClientsWithMixedLabelsAndExpectFailure(t *testing.T) { err := testMultipleClientsWithConfigGenerator(t, len(membersLabel), configGen) require.Error(t, err) - require.Contains(t, err.Error(), fmt.Sprintf("expected to see %d members, got", len(membersLabel))) + require.Contains(t, err.Error(), fmt.Sprintf("expected to see at least %d updates", len(membersLabel))) } func TestMultipleClientsWithMixedLabelsAndClusterLabelVerificationDisabled(t *testing.T) { @@ -720,7 +724,6 @@ func testMultipleClientsWithConfigGenerator(t *testing.T, members int, configGen firstKv := clients[0] ctx, cancel := context.WithTimeout(context.Background(), casInterval*3) // Watch for 3x cas intervals. updates := 0 - gotMembers := 0 firstKv.WatchKey(ctx, key, func(in interface{}) bool { updates++ @@ -733,18 +736,12 @@ func testMultipleClientsWithConfigGenerator(t *testing.T, members int, configGen "tokens, oldest timestamp:", now.Sub(time.Unix(minTimestamp, 0)).String(), "avg timestamp:", now.Sub(time.Unix(avgTimestamp, 0)).String(), "youngest timestamp:", now.Sub(time.Unix(maxTimestamp, 0)).String()) - gotMembers = len(r.Members) return true // yes, keep watching }) cancel() // make linter happy t.Logf("Ring updates observed: %d", updates) - // We expect that all members are in the ring - if gotMembers != members { - return fmt.Errorf("expected to see %d members, got %d", members, gotMembers) - } - 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)