From 3c6193d6e4f3076c9a67737fcd0a92e6766fea2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Leszczy=C5=84ski?= <2000michal@wp.pl> Date: Thu, 7 Mar 2024 01:22:26 +0100 Subject: [PATCH] fix(scylla_client): fix strategy calculation in DescribeRing Fixes #3745 --- pkg/scyllaclient/client_scylla.go | 29 ++++++-- .../client_scylla_integration_test.go | 73 +++++++++++++++++++ pkg/scyllaclient/model.go | 2 + pkg/util/maputil/map.go | 16 ++++ 4 files changed, 112 insertions(+), 8 deletions(-) create mode 100644 pkg/util/maputil/map.go diff --git a/pkg/scyllaclient/client_scylla.go b/pkg/scyllaclient/client_scylla.go index a172e57067..b50a583942 100644 --- a/pkg/scyllaclient/client_scylla.go +++ b/pkg/scyllaclient/client_scylla.go @@ -20,6 +20,7 @@ import ( "github.com/pkg/errors" "github.com/scylladb/go-set/strset" "github.com/scylladb/scylla-manager/v3/pkg/dht" + "github.com/scylladb/scylla-manager/v3/pkg/util/maputil" "github.com/scylladb/scylla-manager/v3/pkg/util/slice" "go.uber.org/multierr" @@ -393,6 +394,8 @@ func (c *Client) DescribeRing(ctx context.Context, keyspace string) (Ring, error replicaTokens := make(map[uint64][]TokenRange) replicaHash := make(map[uint64][]string) + isNetworkTopologyStrategy := true + var dcRF map[string]int for _, p := range resp.Payload { // Parse tokens startToken, err := strconv.ParseInt(p.StartToken, 10, 64) @@ -415,6 +418,18 @@ func (c *Client) DescribeRing(ctx context.Context, keyspace string) (Ring, error EndToken: endToken, }) + ring.RF = max(ring.RF, len(p.Endpoints)) + tokenDCrf := make(map[string]int) + for _, e := range p.EndpointDetails { + tokenDCrf[e.Datacenter]++ + } + // NetworkTopologyStrategy -> all token ranges have the same dc to rf mapping + if dcRF == nil || maputil.Equal(dcRF, tokenDCrf) { + dcRF = tokenDCrf + } else { + isNetworkTopologyStrategy = false + } + // Update host to DC mapping for _, e := range p.EndpointDetails { ring.HostDC[e.Host] = e.Datacenter @@ -443,16 +458,14 @@ func (c *Client) DescribeRing(ctx context.Context, keyspace string) (Ring, error } // Detect replication strategy - if len(ring.HostDC) == 1 { + switch { + case len(ring.HostDC) == 1: ring.Replication = LocalStrategy - } else { + case isNetworkTopologyStrategy: ring.Replication = NetworkTopologyStrategy - for _, tokens := range dcTokens { - if tokens != len(resp.Payload) { - ring.Replication = SimpleStrategy - break - } - } + ring.DCrf = dcRF + default: + ring.Replication = SimpleStrategy } return ring, nil diff --git a/pkg/scyllaclient/client_scylla_integration_test.go b/pkg/scyllaclient/client_scylla_integration_test.go index 80b6ad5d53..cd8752ab21 100644 --- a/pkg/scyllaclient/client_scylla_integration_test.go +++ b/pkg/scyllaclient/client_scylla_integration_test.go @@ -13,7 +13,9 @@ import ( "testing" "time" + "github.com/scylladb/scylla-manager/v3/pkg/testutils/db" . "github.com/scylladb/scylla-manager/v3/pkg/testutils/testconfig" + "github.com/scylladb/scylla-manager/v3/pkg/util/maputil" "github.com/google/go-cmp/cmp" "github.com/scylladb/go-log" @@ -71,6 +73,77 @@ func TestClientStatusIntegration(t *testing.T) { } } +func TestClientDescribeRingIntegration(t *testing.T) { + testCases := []struct { + replicationStmt string + replication scyllaclient.ReplicationStrategy + rf int + dcRF map[string]int + }{ + { + replicationStmt: "{'class': 'SimpleStrategy', 'replication_factor': 4}", + replication: scyllaclient.SimpleStrategy, + rf: 4, + }, + { + replicationStmt: "{'class': 'NetworkTopologyStrategy', 'dc1': 1}", + replication: scyllaclient.NetworkTopologyStrategy, + rf: 1, + dcRF: map[string]int{ + "dc1": 1, + }, + }, + { + replicationStmt: "{'class': 'NetworkTopologyStrategy', 'dc1': 2, 'dc2': 2}", + replication: scyllaclient.NetworkTopologyStrategy, + rf: 4, + dcRF: map[string]int{ + "dc1": 2, + "dc2": 2, + }, + }, + { + replicationStmt: "{'class': 'NetworkTopologyStrategy', 'dc1': 3, 'dc2': 3}", + replication: scyllaclient.NetworkTopologyStrategy, + rf: 6, + dcRF: map[string]int{ + "dc1": 3, + "dc2": 3, + }, + }, + } + + client, err := scyllaclient.NewClient(scyllaclient.TestConfig(ManagedClusterHosts(), AgentAuthToken()), log.NewDevelopment()) + if err != nil { + t.Fatal(err) + } + clusterSession := db.CreateSessionAndDropAllKeyspaces(t, client) + defer clusterSession.Close() + + for i := range testCases { + tc := testCases[i] + if err := clusterSession.ExecStmt("DROP KEYSPACE IF EXISTS test_ks"); err != nil { + t.Fatal(err) + } + if err := clusterSession.ExecStmt("CREATE KEYSPACE test_ks WITH replication = " + tc.replicationStmt); err != nil { + t.Fatal(err) + } + ring, err := client.DescribeRing(context.Background(), "test_ks") + if err != nil { + t.Fatal(err) + } + if tc.replication != ring.Replication { + t.Fatalf("Replication: expected %s, got %s", tc.replication, ring.Replication) + } + if tc.rf != ring.RF { + t.Fatalf("RF: expected %d, got %d", tc.rf, ring.RF) + } + if !maputil.Equal(tc.dcRF, ring.DCrf) { + t.Fatalf("DCrf: expected %v, got %v", tc.dcRF, ring.DCrf) + } + } +} + func TestClientActiveRepairsIntegration(t *testing.T) { client, err := scyllaclient.NewClient(scyllaclient.TestConfig(ManagedClusterHosts(), AgentAuthToken()), log.NewDevelopment()) if err != nil { diff --git a/pkg/scyllaclient/model.go b/pkg/scyllaclient/model.go index 3dc655e783..7140e4dc11 100644 --- a/pkg/scyllaclient/model.go +++ b/pkg/scyllaclient/model.go @@ -179,6 +179,8 @@ type Ring struct { ReplicaTokens []ReplicaTokenRanges HostDC map[string]string Replication ReplicationStrategy + RF int + DCrf map[string]int // initialized only for NetworkTopologyStrategy } // Datacenters returns a list of datacenters the keyspace is replicated in. diff --git a/pkg/util/maputil/map.go b/pkg/util/maputil/map.go new file mode 100644 index 0000000000..6ae9e5234e --- /dev/null +++ b/pkg/util/maputil/map.go @@ -0,0 +1,16 @@ +// Copyright (C) 2024 ScyllaDB + +package maputil + +// Equal checks if m1 and m2 are the same mapping. +func Equal[K, V comparable](m1, m2 map[K]V) bool { + if len(m1) != len(m2) { + return false + } + for k, v1 := range m1 { + if v2, ok := m2[k]; !ok || v1 != v2 { + return false + } + } + return true +}