From 1f10cd18ba43e8b6f91d1562f974331bbadccdb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Leszczy=C5=84ski?= <2000michal@wp.pl> Date: Tue, 17 Sep 2024 12:33:31 +0200 Subject: [PATCH] fix(repair): don't rely on ring replication strategy Replication strategy is not returned from Scylla API, but assumed by SM. That's why we shouldn't rely on it. Fixes #4022 --- pkg/service/repair/plan.go | 25 ++--- pkg/service/repair/repair_test.go | 151 ++++++++++++++++-------------- 2 files changed, 88 insertions(+), 88 deletions(-) diff --git a/pkg/service/repair/plan.go b/pkg/service/repair/plan.go index 50a2cbea37..a615908b83 100644 --- a/pkg/service/repair/plan.go +++ b/pkg/service/repair/plan.go @@ -255,27 +255,18 @@ func ShouldRepairRing(ring scyllaclient.Ring, dcs []string, host string) bool { } } - switch ring.Replication { - case scyllaclient.SimpleStrategy: - // Check range consisting of excluded hosts - excluded := 0 - for _, dc := range ring.HostDC { - if !repairedDCs.Has(dc) { - excluded++ - } - } - return ring.RF > excluded+1 - case scyllaclient.NetworkTopologyStrategy: + for _, rt := range ring.ReplicaTokens { rep := 0 - for dc, rf := range ring.DCrf { - if repairedDCs.Has(dc) { - rep += rf + for _, r := range rt.ReplicaSet { + if repairedDCs.Has(ring.HostDC[r]) { + rep++ } } - return rep > 1 - default: - return false + if rep <= 1 { + return false + } } + return true } // maxHostIntensity sets max_ranges_in_parallel for all repaired host. diff --git a/pkg/service/repair/repair_test.go b/pkg/service/repair/repair_test.go index 6d4d517c24..c93b43468a 100644 --- a/pkg/service/repair/repair_test.go +++ b/pkg/service/repair/repair_test.go @@ -6,6 +6,7 @@ import ( "fmt" "testing" + "github.com/scylladb/scylla-manager/v3/pkg/dht" "github.com/scylladb/scylla-manager/v3/pkg/scyllaclient" "github.com/scylladb/scylla-manager/v3/pkg/service/repair" ) @@ -124,6 +125,20 @@ func TestShouldRepairRing(t *testing.T) { "h9": "dc4", } + rs := func(reps ...string) scyllaclient.ReplicaTokenRanges { + return scyllaclient.ReplicaTokenRanges{ + ReplicaSet: reps, + Ranges: []scyllaclient.TokenRange{ + { + StartToken: dht.Murmur3MinToken, + EndToken: dht.Murmur3MaxToken, + }, + }, + } + } + + allDCs := []string{"dc1", "dc2", "dc3", "dc4"} + testCases := []struct { Ring scyllaclient.Ring DCs []string @@ -132,114 +147,118 @@ func TestShouldRepairRing(t *testing.T) { }{ { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.LocalStrategy, - RF: 1, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{rs("h1")}, + HostDC: hostDC, }, - DCs: []string{"dc1", "dc2", "dc3", "dc4"}, + DCs: allDCs, Expected: false, }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.SimpleStrategy, - RF: 1, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1"), rs("h2"), rs("h3"), + rs("h4"), rs("h5"), rs("h6"), + rs("h7"), rs("h8"), rs("h9"), + }, + HostDC: hostDC, }, - DCs: []string{"dc1", "dc2", "dc3", "dc4"}, + DCs: allDCs, Expected: false, }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.SimpleStrategy, - RF: 2, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2"), rs("h2", "h3"), rs("h3", "h4"), + rs("h4", "h5"), rs("h5", "h6"), rs("h6", "h7"), + rs("h7", "h8"), rs("h8", "h9"), rs("h9", "h1"), + }, + HostDC: hostDC, }, - DCs: []string{"dc1", "dc2", "dc3", "dc4"}, + DCs: allDCs, Expected: true, }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.SimpleStrategy, - RF: 3, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2", "h3"), rs("h2", "h3", "h4"), rs("h3", "h4", "h5"), + rs("h4", "h5", "h6"), rs("h5", "h6", "h7"), rs("h6", "h7", "h8"), + rs("h7", "h8", "h9"), rs("h8", "h9", "h1"), rs("h9", "h1", "h2"), + }, + HostDC: hostDC, }, DCs: []string{"dc3", "dc4"}, Expected: false, }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.SimpleStrategy, - RF: 4, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2", "h3", "h4"), rs("h2", "h3", "h4", "h5"), rs("h3", "h4", "h5", "h6"), + rs("h4", "h5", "h6", "h7"), rs("h5", "h6", "h7", "h8"), rs("h6", "h7", "h8", "h9"), + rs("h7", "h8", "h9", "h1"), rs("h8", "h9", "h1", "h2"), rs("h9", "h1", "h2", "h3"), + }, + HostDC: hostDC, }, DCs: []string{"dc3", "dc4"}, Expected: true, }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.SimpleStrategy, - RF: 4, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2", "h3", "h4"), rs("h2", "h3", "h4", "h5"), rs("h3", "h4", "h5", "h6"), + rs("h4", "h5", "h6", "h7"), rs("h5", "h6", "h7", "h8"), rs("h6", "h7", "h8", "h9"), + rs("h7", "h8", "h9", "h1"), rs("h8", "h9", "h1", "h2"), rs("h9", "h1", "h2", "h3"), + }, + HostDC: hostDC, }, DCs: []string{"dc4"}, Expected: false, }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.NetworkTopologyStrategy, - RF: 4, - DCrf: map[string]int{ - "dc1": 1, - "dc2": 1, - "dc3": 1, - "dc4": 1, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2", "h3", "h6"), rs("h1", "h2", "h4", "h6"), rs("h1", "h2", "h5", "h6"), + rs("h1", "h2", "h3", "h7"), rs("h1", "h2", "h4", "h7"), rs("h1", "h2", "h5", "h7"), + rs("h1", "h2", "h3", "h8"), rs("h1", "h2", "h4", "h8"), rs("h1", "h2", "h5", "h8"), + rs("h1", "h2", "h3", "h9"), rs("h1", "h2", "h4", "h9"), rs("h1", "h2", "h5", "h9"), }, + HostDC: hostDC, }, - DCs: []string{"dc1", "dc2", "dc3", "dc4"}, + DCs: allDCs, Expected: true, }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.NetworkTopologyStrategy, - RF: 4, - DCrf: map[string]int{ - "dc1": 1, - "dc2": 1, - "dc3": 1, - "dc4": 1, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2", "h3", "h6"), rs("h1", "h2", "h4", "h6"), rs("h1", "h2", "h5", "h6"), + rs("h1", "h2", "h3", "h7"), rs("h1", "h2", "h4", "h7"), rs("h1", "h2", "h5", "h7"), + rs("h1", "h2", "h3", "h8"), rs("h1", "h2", "h4", "h8"), rs("h1", "h2", "h5", "h8"), + rs("h1", "h2", "h3", "h9"), rs("h1", "h2", "h4", "h9"), rs("h1", "h2", "h5", "h9"), }, + HostDC: hostDC, }, DCs: []string{"dc1"}, Expected: false, }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.NetworkTopologyStrategy, - RF: 8, - DCrf: map[string]int{ - "dc1": 1, - "dc2": 1, - "dc3": 2, - "dc4": 2, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2", "h3", "h4", "h6", "h7"), rs("h1", "h2", "h5", "h4", "h6", "h7"), rs("h1", "h2", "h3", "h5", "h6", "h7"), + rs("h1", "h2", "h3", "h4", "h6", "h8"), rs("h1", "h2", "h5", "h4", "h6", "h8"), rs("h1", "h2", "h3", "h5", "h6", "h8"), + rs("h1", "h2", "h3", "h4", "h6", "h9"), rs("h1", "h2", "h5", "h4", "h6", "h9"), rs("h1", "h2", "h3", "h5", "h6", "h9"), + rs("h1", "h2", "h3", "h4", "h7", "h8"), rs("h1", "h2", "h5", "h4", "h7", "h8"), rs("h1", "h2", "h3", "h5", "h7", "h8"), + rs("h1", "h2", "h3", "h4", "h8", "h9"), rs("h1", "h2", "h5", "h4", "h8", "h9"), rs("h1", "h2", "h3", "h5", "h8", "h9"), }, + HostDC: hostDC, }, DCs: []string{"dc3"}, Expected: true, }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.NetworkTopologyStrategy, - RF: 8, - DCrf: map[string]int{ - "dc1": 1, - "dc2": 1, - "dc3": 3, - "dc4": 4, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2", "h3", "h4", "h5", "h6", "h7", "h8", "h9"), }, + HostDC: hostDC, }, DCs: []string{"dc4"}, Host: "h6", @@ -247,15 +266,10 @@ func TestShouldRepairRing(t *testing.T) { }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.NetworkTopologyStrategy, - RF: 8, - DCrf: map[string]int{ - "dc1": 1, - "dc2": 1, - "dc3": 3, - "dc4": 4, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2", "h3", "h4", "h5", "h6", "h7", "h8", "h9"), }, + HostDC: hostDC, }, DCs: []string{"dc4"}, Host: "h2", @@ -263,17 +277,12 @@ func TestShouldRepairRing(t *testing.T) { }, { Ring: scyllaclient.Ring{ - HostDC: hostDC, - Replication: scyllaclient.NetworkTopologyStrategy, - RF: 8, - DCrf: map[string]int{ - "dc1": 1, - "dc2": 1, - "dc3": 3, - "dc4": 4, + ReplicaTokens: []scyllaclient.ReplicaTokenRanges{ + rs("h1", "h2", "h3", "h4", "h5", "h6", "h7", "h8", "h9"), }, + HostDC: hostDC, }, - DCs: []string{"dc1", "dc2", "dc3", "dc4"}, + DCs: allDCs, Host: "h1", Expected: true, },