Skip to content

Commit a617bfe

Browse files
Michal-LeszczynskiMichal-Leszczynski
committed
fix(treewide): move from string to netip.Addr in scyllaclient.Ring and repair.Target
This commit tries to improve IPV6 handling by storing addr as netip.Addr instead of string. It does so mainly for scyllaclient.Ring and repair.Target and it adjusts all other required places in the code. Ref #4342
1 parent 87d1fc8 commit a617bfe

File tree

16 files changed

+262
-142
lines changed

16 files changed

+262
-142
lines changed

‎pkg/scyllaclient/client_scylla.go‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@ import (
99
"fmt"
1010
"net"
1111
"net/http"
12+
"net/netip"
1213
"regexp"
1314
"runtime"
15+
"slices"
1416
"sort"
1517
"strconv"
1618
"strings"
@@ -25,6 +27,7 @@ import (
2527
"github.com/scylladb/scylla-manager/v3/pkg/util/pointer"
2628
"github.com/scylladb/scylla-manager/v3/pkg/util/prom"
2729
"github.com/scylladb/scylla-manager/v3/pkg/util/slice"
30+
"github.com/scylladb/scylla-manager/v3/pkg/util2"
2831
"github.com/scylladb/scylla-manager/v3/swagger/gen/scylla/v1/client/operations"
2932
"github.com/scylladb/scylla-manager/v3/swagger/gen/scylla/v1/models"
3033
"go.uber.org/multierr"
@@ -492,12 +495,12 @@ func (c *Client) describeRing(params *operations.StorageServiceDescribeRingByKey
492495

493496
ring := Ring{
494497
ReplicaTokens: make([]ReplicaTokenRanges, 0),
495-
HostDC: map[string]string{},
498+
HostDC: map[netip.Addr]string{},
496499
}
497500
dcTokens := make(map[string]int)
498501

499502
replicaTokens := make(map[uint64][]TokenRange)
500-
replicaHash := make(map[uint64][]string)
503+
replicaHash := make(map[uint64][]netip.Addr)
501504

502505
isNetworkTopologyStrategy := true
503506
rf := len(resp.Payload[0].Endpoints)
@@ -513,12 +516,18 @@ func (c *Client) describeRing(params *operations.StorageServiceDescribeRingByKey
513516
return Ring{}, errors.Wrap(err, "parse EndToken")
514517
}
515518

516-
// Ensure deterministic order or nodes in replica set
517-
sort.Strings(p.Endpoints)
519+
replicaSet, err := util2.ConvertSliceWithError(p.Endpoints, netip.ParseAddr)
520+
if err != nil {
521+
return Ring{}, err
522+
}
523+
// Ensure deterministic order of nodes in replica set
524+
slices.SortFunc(replicaSet, func(a, b netip.Addr) int {
525+
return a.Compare(b)
526+
})
518527

519528
// Aggregate replica set token ranges
520-
hash := ReplicaHash(p.Endpoints)
521-
replicaHash[hash] = p.Endpoints
529+
hash := ReplicaHash(replicaSet)
530+
replicaHash[hash] = replicaSet
522531
replicaTokens[hash] = append(replicaTokens[hash], TokenRange{
523532
StartToken: startToken,
524533
EndToken: endToken,
@@ -541,7 +550,11 @@ func (c *Client) describeRing(params *operations.StorageServiceDescribeRingByKey
541550

542551
// Update host to DC mapping
543552
for _, e := range p.EndpointDetails {
544-
ring.HostDC[e.Host] = e.Datacenter
553+
ip, err := netip.ParseAddr(e.Host)
554+
if err != nil {
555+
return Ring{}, errors.Wrapf(err, "parse host IP address (%s)", e.Host)
556+
}
557+
ring.HostDC[ip] = e.Datacenter
545558
}
546559

547560
// Update DC token metrics
@@ -582,11 +595,11 @@ func (c *Client) describeRing(params *operations.StorageServiceDescribeRingByKey
582595
}
583596

584597
// ReplicaHash hashes replicas so that it can be used as a map key.
585-
func ReplicaHash(replicaSet []string) uint64 {
598+
func ReplicaHash(replicaSet []netip.Addr) uint64 {
586599
hash := xxhash.New()
587600
for _, r := range replicaSet {
588-
_, _ = hash.WriteString(r) // nolint: errcheck
589-
_, _ = hash.WriteString(",") // nolint: errcheck
601+
_, _ = hash.WriteString(r.String()) // nolint: errcheck
602+
_, _ = hash.WriteString(",") // nolint: errcheck
590603
}
591604
return hash.Sum64()
592605
}

‎pkg/scyllaclient/client_scylla_test.go‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ package scyllaclient_test
55
import (
66
"context"
77
"net/http"
8+
"net/netip"
9+
"slices"
810
"strings"
911
"testing"
1012

@@ -193,25 +195,32 @@ func TestClientDescribeRing(t *testing.T) {
193195

194196
{
195197
golden := scyllaclient.ReplicaTokenRanges{
196-
ReplicaSet: []string{"172.16.1.10", "172.16.1.2", "172.16.1.20", "172.16.1.3", "172.16.1.4", "172.16.1.5"},
197-
Ranges: []scyllaclient.TokenRange{{StartToken: -9223128845313325022, EndToken: -9197905337938558763}},
198+
ReplicaSet: []netip.Addr{
199+
netip.MustParseAddr("172.16.1.10"),
200+
netip.MustParseAddr("172.16.1.2"),
201+
netip.MustParseAddr("172.16.1.20"),
202+
netip.MustParseAddr("172.16.1.3"),
203+
netip.MustParseAddr("172.16.1.4"),
204+
netip.MustParseAddr("172.16.1.5"),
205+
},
206+
Ranges: []scyllaclient.TokenRange{{StartToken: -9223128845313325022, EndToken: -9197905337938558763}},
198207
}
199-
if diff := cmp.Diff(ring.ReplicaTokens[0].ReplicaSet, golden.ReplicaSet); diff != "" {
200-
t.Fatal(diff)
208+
if slices.Equal(ring.ReplicaTokens[0].ReplicaSet, golden.ReplicaSet) {
209+
t.Fatalf("Expected replica set %#v, got %#v", ring.ReplicaTokens[0].ReplicaSet, golden.ReplicaSet)
201210
}
202-
if diff := cmp.Diff(ring.ReplicaTokens[0].Ranges[0], golden.Ranges[0]); diff != "" {
203-
t.Fatal(diff)
211+
if slices.Equal(ring.ReplicaTokens[0].Ranges, golden.Ranges) {
212+
t.Fatalf("Expected ranges %#v, got %#v", ring.ReplicaTokens[0].Ranges, golden.Ranges)
204213
}
205214
}
206215

207216
{
208-
golden := map[string]string{
209-
"172.16.1.10": "dc1",
210-
"172.16.1.2": "dc1",
211-
"172.16.1.20": "dc2",
212-
"172.16.1.3": "dc1",
213-
"172.16.1.4": "dc2",
214-
"172.16.1.5": "dc2",
217+
golden := map[netip.Addr]string{
218+
netip.MustParseAddr("172.16.1.10"): "dc1",
219+
netip.MustParseAddr("172.16.1.2"): "dc1",
220+
netip.MustParseAddr("172.16.1.20"): "dc2",
221+
netip.MustParseAddr("172.16.1.3"): "dc1",
222+
netip.MustParseAddr("172.16.1.4"): "dc2",
223+
netip.MustParseAddr("172.16.1.5"): "dc2",
215224
}
216225
if diff := cmp.Diff(ring.HostDC, golden); diff != "" {
217226
t.Fatal(diff)

‎pkg/scyllaclient/model.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package scyllaclient
44

55
import (
6+
"net/netip"
67
"reflect"
78

89
"github.com/gocql/gocql"
@@ -177,7 +178,7 @@ const (
177178
// Ring describes token ring of a keyspace.
178179
type Ring struct {
179180
ReplicaTokens []ReplicaTokenRanges
180-
HostDC map[string]string
181+
HostDC map[netip.Addr]string
181182
// Replication is not returned by Scylla, but assumed by SM.
182183
// Don't use it for correctness.
183184
Replication ReplicationStrategy
@@ -212,7 +213,7 @@ func (t *TokenRange) UnmarshalUDT(name string, info gocql.TypeInfo, data []byte)
212213

213214
// ReplicaTokenRanges describes all token ranges belonging to given replica set.
214215
type ReplicaTokenRanges struct {
215-
ReplicaSet []string // Sorted lexicographically
216+
ReplicaSet []netip.Addr // Sorted by Addr.Compare
216217
Ranges []TokenRange // Sorted by start token
217218
}
218219

‎pkg/scyllaclient/model_test.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
package scyllaclient
44

55
import (
6+
"net/netip"
67
"sort"
78
"testing"
89

@@ -44,13 +45,13 @@ func TestRingDatacenters(t *testing.T) {
4445
t.Parallel()
4546

4647
r := Ring{
47-
HostDC: map[string]string{
48-
"172.16.1.10": "dc1",
49-
"172.16.1.2": "dc1",
50-
"172.16.1.20": "dc2",
51-
"172.16.1.3": "dc1",
52-
"172.16.1.4": "dc2",
53-
"172.16.1.5": "dc2",
48+
HostDC: map[netip.Addr]string{
49+
netip.MustParseAddr("172.16.1.10"): "dc1",
50+
netip.MustParseAddr("172.16.1.2"): "dc1",
51+
netip.MustParseAddr("172.16.1.20"): "dc2",
52+
netip.MustParseAddr("172.16.1.3"): "dc1",
53+
netip.MustParseAddr("172.16.1.4"): "dc2",
54+
netip.MustParseAddr("172.16.1.5"): "dc2",
5455
},
5556
}
5657
d := r.Datacenters()

‎pkg/service/backup/model.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/scylladb/gocqlx/v2"
1818
"github.com/scylladb/scylla-manager/backupspec"
1919
"github.com/scylladb/scylla-manager/v3/pkg/util/inexlist/ksfilter"
20+
"github.com/scylladb/scylla-manager/v3/pkg/util2"
2021

2122
"go.uber.org/multierr"
2223

@@ -504,7 +505,7 @@ type tokenRangesValidator struct {
504505

505506
func (v tokenRangesValidator) validate(ks, tab string, ring scyllaclient.Ring) error {
506507
for _, rt := range ring.ReplicaTokens {
507-
if !v.liveNodes.HasAny(rt.ReplicaSet...) {
508+
if !v.liveNodes.HasAny(util2.ConvertSliceToString(rt.ReplicaSet)...) {
508509
return errors.Errorf("%s.%s: the whole replica set %v is filtered out, so the data owned by it can't be backed up", ks, tab, rt.ReplicaSet)
509510
}
510511
}

‎pkg/service/repair/controller.go‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,17 @@
22

33
package repair
44

5+
import "net/netip"
6+
57
// controller keeps the state of repairs running in the cluster
68
// and informs generator about allowed repair intensity on a given replica set.
79
type controller interface {
810
// TryBlock returns if it's allowed to schedule a repair job on given replica set.
911
// The second returned value is the allowed intensity of such job.
10-
TryBlock(replicaSet []string) (ok bool, intensity int)
12+
TryBlock(replicaSet []netip.Addr) (ok bool, intensity int)
1113
// Unblock informs controller that a repair job running on replica set has finished.
1214
// This makes it possible to call TryBlock on nodes from replica set.
13-
Unblock(replicaSet []string)
15+
Unblock(replicaSet []netip.Addr)
1416
// Busy checks if there are any running repair jobs that controller is aware of.
1517
Busy() bool
1618
}
@@ -19,7 +21,7 @@ type intensityChecker interface {
1921
Intensity() Intensity
2022
Parallel() int
2123
MaxParallel() int
22-
ReplicaSetMaxIntensity(replicaSet []string) Intensity
24+
ReplicaSetMaxIntensity(replicaSet []netip.Addr) Intensity
2325
}
2426

2527
// rowLevelRepairController is a specialised controller for row-level repair.
@@ -29,20 +31,20 @@ type intensityChecker interface {
2931
type rowLevelRepairController struct {
3032
intensity intensityChecker
3133

32-
jobsCnt int // Total amount of repair jobs in the cluster
33-
nodeJobs map[string]int // Amount of repair jobs on a given node
34+
jobsCnt int // Total amount of repair jobs in the cluster
35+
nodeJobs map[netip.Addr]int // Amount of repair jobs on a given node
3436
}
3537

3638
var _ controller = &rowLevelRepairController{}
3739

3840
func newRowLevelRepairController(i intensityChecker) *rowLevelRepairController {
3941
return &rowLevelRepairController{
4042
intensity: i,
41-
nodeJobs: make(map[string]int),
43+
nodeJobs: make(map[netip.Addr]int),
4244
}
4345
}
4446

45-
func (c *rowLevelRepairController) TryBlock(replicaSet []string) (ok bool, intensity int) {
47+
func (c *rowLevelRepairController) TryBlock(replicaSet []netip.Addr) (ok bool, intensity int) {
4648
if !c.shouldBlock(replicaSet) {
4749
return false, 0
4850
}
@@ -55,7 +57,7 @@ func (c *rowLevelRepairController) TryBlock(replicaSet []string) (ok bool, inten
5557
return true, int(i)
5658
}
5759

58-
func (c *rowLevelRepairController) shouldBlock(replicaSet []string) bool {
60+
func (c *rowLevelRepairController) shouldBlock(replicaSet []netip.Addr) bool {
5961
// DENY if any node is already participating in repair job
6062
for _, r := range replicaSet {
6163
if c.nodeJobs[r] > 0 {
@@ -76,14 +78,14 @@ func (c *rowLevelRepairController) shouldBlock(replicaSet []string) bool {
7678
return true
7779
}
7880

79-
func (c *rowLevelRepairController) block(replicaSet []string) {
81+
func (c *rowLevelRepairController) block(replicaSet []netip.Addr) {
8082
c.jobsCnt++
8183
for _, r := range replicaSet {
8284
c.nodeJobs[r]++
8385
}
8486
}
8587

86-
func (c *rowLevelRepairController) Unblock(replicaSet []string) {
88+
func (c *rowLevelRepairController) Unblock(replicaSet []netip.Addr) {
8789
c.jobsCnt--
8890
for _, r := range replicaSet {
8991
c.nodeJobs[r]--

‎pkg/service/repair/controller_test.go‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package repair
44

55
import (
66
"context"
7+
"net/netip"
78
"testing"
89

910
"github.com/scylladb/go-log"
@@ -12,16 +13,16 @@ import (
1213
)
1314

1415
func TestRowLevelRepairController_TryBlock(t *testing.T) {
15-
const (
16-
node1 = "192.168.1.1"
17-
node2 = "192.168.1.2"
18-
node3 = "192.168.1.3"
19-
node4 = "192.168.1.4"
20-
node5 = "192.168.1.5"
21-
node6 = "192.168.1.6"
16+
var (
17+
node1 = netip.MustParseAddr("192.168.1.1")
18+
node2 = netip.MustParseAddr("192.168.1.2")
19+
node3 = netip.MustParseAddr("192.168.1.3")
20+
node4 = netip.MustParseAddr("192.168.1.4")
21+
node5 = netip.MustParseAddr("192.168.1.5")
22+
node6 = netip.MustParseAddr("192.168.1.6")
2223
)
2324

24-
maxRangesPerHost := map[string]Intensity{
25+
maxRangesPerHost := map[netip.Addr]Intensity{
2526
node1: 20,
2627
node2: 19,
2728
node3: 18,
@@ -43,7 +44,7 @@ func TestRowLevelRepairController_TryBlock(t *testing.T) {
4344
}
4445

4546
t.Run("make sure TryBlock() will deny if replicaset is already blocked", func(t *testing.T) {
46-
replicaSet := []string{node1, node2}
47+
replicaSet := []netip.Addr{node1, node2}
4748
c := newRowLevelRepairController(defaultIntensityHandler())
4849

4950
if ok, _ := c.TryBlock(replicaSet); !ok {
@@ -64,7 +65,7 @@ func TestRowLevelRepairController_TryBlock(t *testing.T) {
6465
expectedNrOfRanges = 10
6566
maxParallel = 2
6667
)
67-
replicaSet := []string{node1, node2}
68+
replicaSet := []netip.Addr{node1, node2}
6869
ih := defaultIntensityHandler()
6970
ih.maxParallel = maxParallel
7071
c := newRowLevelRepairController(ih)
@@ -80,7 +81,7 @@ func TestRowLevelRepairController_TryBlock(t *testing.T) {
8081
maxParallel = 2
8182
expectedNrOfRanges = 19
8283
)
83-
replicaSet := []string{node1, node2}
84+
replicaSet := []netip.Addr{node1, node2}
8485
ih := defaultIntensityHandler()
8586
ih.maxParallel = maxParallel
8687
c := newRowLevelRepairController(ih)
@@ -97,7 +98,7 @@ func TestRowLevelRepairController_TryBlock(t *testing.T) {
9798
intensity = 20
9899
minRangesInParallel = 15
99100
)
100-
replicaSet := []string{node1, node2, node6}
101+
replicaSet := []netip.Addr{node1, node2, node6}
101102
ih := defaultIntensityHandler()
102103
ih.maxParallel = maxParallel
103104
c := newRowLevelRepairController(ih)
@@ -109,8 +110,8 @@ func TestRowLevelRepairController_TryBlock(t *testing.T) {
109110
})
110111

111112
t.Run("make sure TryBlock() will deny if there is more jobs than {parallel} already", func(t *testing.T) {
112-
replicaSet1 := []string{node1, node2}
113-
replicaSet2 := []string{node3, node4}
113+
replicaSet1 := []netip.Addr{node1, node2}
114+
replicaSet2 := []netip.Addr{node3, node4}
114115
maxParallel := 10
115116
ih := defaultIntensityHandler()
116117
ih.maxParallel = maxParallel
@@ -126,9 +127,9 @@ func TestRowLevelRepairController_TryBlock(t *testing.T) {
126127
})
127128

128129
t.Run("make sure TryBlock() will deny if there is more jobs than maxParallel=2 already", func(t *testing.T) {
129-
replicaSet1 := []string{node1, node2}
130-
replicaSet2 := []string{node3, node4}
131-
replicaSet3 := []string{node3, node4}
130+
replicaSet1 := []netip.Addr{node1, node2}
131+
replicaSet2 := []netip.Addr{node3, node4}
132+
replicaSet3 := []netip.Addr{node3, node4}
132133
maxParallel := 2
133134
ih := defaultIntensityHandler()
134135
ih.maxParallel = maxParallel

0 commit comments

Comments
 (0)