Skip to content

Commit 8fc2efc

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 70e397b commit 8fc2efc

22 files changed

+375
-190
lines changed

‎pkg/scyllaclient/client_scylla.go

+23-10
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+
slices2 "github.com/scylladb/scylla-manager/v3/pkg/util2/slices"
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 := slices2.MapWithError(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{}, err
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

+22-13
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

+3-2
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

+8-7
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

+4-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"context"
77
"encoding/json"
88
"fmt"
9+
"net/netip"
910
"reflect"
1011
"slices"
1112
"strings"
@@ -17,6 +18,7 @@ import (
1718
"github.com/scylladb/gocqlx/v2"
1819
"github.com/scylladb/scylla-manager/backupspec"
1920
"github.com/scylladb/scylla-manager/v3/pkg/util/inexlist/ksfilter"
21+
"github.com/scylladb/scylla-manager/v3/pkg/util2/maps"
2022

2123
"go.uber.org/multierr"
2224

@@ -498,13 +500,13 @@ type tabValidator interface {
498500
// Validates that each token range is owned by at least one live backed up node.
499501
// Otherwise, corresponding data wouldn't be included in the backup.
500502
type tokenRangesValidator struct {
501-
liveNodes *strset.Set
503+
liveNodes *map[netip.Addr]struct{}
502504
dcs *strset.Set
503505
}
504506

505507
func (v tokenRangesValidator) validate(ks, tab string, ring scyllaclient.Ring) error {
506508
for _, rt := range ring.ReplicaTokens {
507-
if !v.liveNodes.HasAny(rt.ReplicaSet...) {
509+
if !maps.HasAnyKey(*v.liveNodes, rt.ReplicaSet...) {
508510
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)
509511
}
510512
}

‎pkg/service/backup/service.go

+9-1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"context"
88
"encoding/json"
99
"fmt"
10+
"net/netip"
1011
"sort"
1112
"strings"
1213
"sync"
@@ -30,6 +31,8 @@ import (
3031
"github.com/scylladb/scylla-manager/v3/pkg/util/jsonutil"
3132
"github.com/scylladb/scylla-manager/v3/pkg/util/parallel"
3233
"github.com/scylladb/scylla-manager/v3/pkg/util/query"
34+
"github.com/scylladb/scylla-manager/v3/pkg/util2/maps"
35+
"github.com/scylladb/scylla-manager/v3/pkg/util2/slices"
3336

3437
"github.com/scylladb/scylla-manager/v3/pkg/util/timeutc"
3538
"github.com/scylladb/scylla-manager/v3/pkg/util/uuid"
@@ -193,9 +196,14 @@ func (s *Service) targetFromProperties(ctx context.Context, clusterID uuid.UUID,
193196
return Target{}, errors.Wrap(err, "create cluster session")
194197
}
195198

199+
liveNodeIPs, err := slices.MapWithError(liveNodes.Hosts(), netip.ParseAddr)
200+
if err != nil {
201+
return Target{}, err
202+
}
203+
liveNodesSet := maps.SetFromSlice(liveNodeIPs)
196204
validators := []tabValidator{
197205
tokenRangesValidator{
198-
liveNodes: strset.New(liveNodes.Hosts()...),
206+
liveNodes: &liveNodesSet,
199207
dcs: strset.New(dcs...),
200208
},
201209
}

‎pkg/service/repair/controller.go

+12-10
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]--

0 commit comments

Comments
 (0)