Skip to content

Commit 35ec42c

Browse files
craig[bot]rafissfqazitbg
committed
156228: multiregion: allow zone config extensions to remove read replicas r=rafiss a=rafiss Previously, zone config extensions validated that num_replicas could not be set lower than the total number of replicas required for the database's survival goal (which includes both voters and non-voting read replicas). This prevented users from removing read replicas to reduce storage costs while maintaining the required number of voters for the survival goal. The survival goal only depends on the number of voting replicas (num_voters), not the total number of replicas. Non-voting replicas are used for follower reads and do not participate in Raft consensus or affect availability guarantees. This commit changes the validation in ExtendZoneConfigWithGlobal() and ExtendZoneConfigWithRegionalIn() to check that num_replicas is not set lower than num_voters instead of the total numReplicas. This allows users to set num_replicas equal to num_voters, effectively removing read replicas while maintaining the required voter count for their survival goal. For example, in a SURVIVE ZONE FAILURE database with 3 regions, users can now reduce num_replicas from 5 (default: 3 voters + 2 read replicas) to 3 (just the required voters), saving storage while maintaining zone failure tolerance. Also fixes a typo in the error message ("num_replica" -> "num_replicas"). Resolves: #105210 Epic: CRDB-45964 Release note (bug fix): Fixed a bug where zone config extensions incorrectly prevented users from removing non-voting read replicas from multi-region databases. Users can now set num_replicas equal to num_voters to remove read replicas while maintaining the required number of voting replicas for their database's survival goal. This allows reducing storage costs without compromising availability guarantees. 156275: catalog/lease: fix race condition with testing knob r=fqazi a=fqazi Previously, the testing knob TestingDisableRangeFeedCheckpoint was read without any mutex, which could lead to a race condition in certain tests as detected by the race detector. To address this, this patch moves the field into the lease manager as an atomic, since testing knobs are copied, so they cannot store mutexes or atomics. Fixes: #156182 Release note: None 156288: raftstorebench: prevent iter use-after-close r=tbg a=tbg The `statsLoop` could still be running. In the linked test flake below, we see it call `(*pebble).GetMetrics` when the engines are already closed. Rearrange the code so that we wait for `statsLoop` to terminate properly before exiting the test. Closes #156138. Epic: none Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com> Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com> Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
4 parents 75b649c + bd42c7c + 2e9913f + 7880791 commit 35ec42c

File tree

5 files changed

+134
-39
lines changed

5 files changed

+134
-39
lines changed

‎pkg/ccl/logictestccl/testdata/logic_test/multi_region_zone_config_extensions‎

Lines changed: 92 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -290,10 +290,100 @@ statement error pq: zone config extension cannot unset the home region \(ca-cent
290290
ALTER DATABASE "mr-zone-configs" ALTER LOCALITY REGIONAL IN "ca-central-1" CONFIGURE ZONE USING
291291
lease_preferences = '[[+region=ap-southeast-2]]'
292292

293-
statement error pq: zone config extension cannot set num_replicas 4 that is lower than the one required for the survival goal: 5 with goal REGION_FAILURE
293+
statement error pq: cannot set num_replicas below 5 \(num_voters required for survival goal REGION_FAILURE\), got 4
294294
ALTER DATABASE "mr-zone-configs" ALTER LOCALITY GLOBAL CONFIGURE ZONE USING
295295
num_replicas = 4
296296

297-
statement error pq: zone config extension cannot set num_voters 4 that is lower than the one required for the survival goal: 5 with goal REGION_FAILURE
297+
statement error pq: cannot set num_voters below 5 \(num_voters required for survival goal REGION_FAILURE\), got 4
298298
ALTER DATABASE "mr-zone-configs" ALTER LOCALITY GLOBAL CONFIGURE ZONE USING
299299
num_voters = 4
300+
301+
# Test that num_replicas can be set to num_voters (removing read replicas).
302+
# This is allowed because survival goals depend on num_voters, not total replicas.
303+
304+
# SURVIVE REGION FAILURE with num_voters=5 should allow num_replicas=5 (removing read replicas).
305+
statement ok
306+
ALTER DATABASE "mr-zone-configs" ALTER LOCALITY GLOBAL CONFIGURE ZONE USING
307+
num_replicas = 5
308+
309+
query TT
310+
SHOW ZONE CONFIGURATION FOR DATABASE "mr-zone-configs"
311+
----
312+
DATABASE "mr-zone-configs" ALTER DATABASE "mr-zone-configs" CONFIGURE ZONE USING
313+
range_min_bytes = 134217728,
314+
range_max_bytes = 536870912,
315+
gc.ttlseconds = 14400,
316+
num_replicas = 9,
317+
num_voters = 5,
318+
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
319+
voter_constraints = '{+region=ca-central-1: 2}',
320+
lease_preferences = '[[+region=ca-central-1], [+region=ap-southeast-2]]'
321+
322+
statement ok
323+
ALTER DATABASE "mr-zone-configs" ALTER LOCALITY REGIONAL CONFIGURE ZONE USING
324+
num_replicas = 5
325+
326+
query TT
327+
SHOW ZONE CONFIGURATION FOR DATABASE "mr-zone-configs"
328+
----
329+
DATABASE "mr-zone-configs" ALTER DATABASE "mr-zone-configs" CONFIGURE ZONE USING
330+
range_min_bytes = 134217728,
331+
range_max_bytes = 536870912,
332+
gc.ttlseconds = 14400,
333+
num_replicas = 5,
334+
num_voters = 5,
335+
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
336+
voter_constraints = '{+region=ca-central-1: 2}',
337+
lease_preferences = '[[+region=ca-central-1], [+region=ap-southeast-2]]'
338+
339+
# num_replicas below num_voters should still fail.
340+
statement error pq: cannot set num_replicas below 5 \(num_voters required for survival goal REGION_FAILURE\), got 3
341+
ALTER DATABASE "mr-zone-configs" ALTER LOCALITY REGIONAL CONFIGURE ZONE USING
342+
num_replicas = 3
343+
344+
# Test SURVIVE ZONE FAILURE case.
345+
statement ok
346+
CREATE DATABASE "mr-zone-survival"
347+
primary region "ca-central-1"
348+
regions "ap-southeast-2", "us-east-1"
349+
survive zone failure
350+
351+
statement ok
352+
use "mr-zone-survival"
353+
354+
# SURVIVE ZONE with num_voters=3, num_replicas=5 by default.
355+
query TT
356+
SHOW ZONE CONFIGURATION FOR DATABASE "mr-zone-survival"
357+
----
358+
DATABASE "mr-zone-survival" ALTER DATABASE "mr-zone-survival" CONFIGURE ZONE USING
359+
range_min_bytes = 134217728,
360+
range_max_bytes = 536870912,
361+
gc.ttlseconds = 14400,
362+
num_replicas = 5,
363+
num_voters = 3,
364+
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
365+
voter_constraints = '[+region=ca-central-1]',
366+
lease_preferences = '[[+region=ca-central-1]]'
367+
368+
# Reduce num_replicas to num_voters (3) - removing read replicas.
369+
statement ok
370+
ALTER DATABASE "mr-zone-survival" ALTER LOCALITY REGIONAL CONFIGURE ZONE USING
371+
num_replicas = 3
372+
373+
query TT
374+
SHOW ZONE CONFIGURATION FOR DATABASE "mr-zone-survival"
375+
----
376+
DATABASE "mr-zone-survival" ALTER DATABASE "mr-zone-survival" CONFIGURE ZONE USING
377+
range_min_bytes = 134217728,
378+
range_max_bytes = 536870912,
379+
gc.ttlseconds = 14400,
380+
num_replicas = 3,
381+
num_voters = 3,
382+
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
383+
voter_constraints = '[+region=ca-central-1]',
384+
lease_preferences = '[[+region=ca-central-1]]'
385+
386+
# num_replicas below num_voters should still fail.
387+
statement error pq: cannot set num_replicas below 3 \(num_voters required for survival goal ZONE_FAILURE\), got 2
388+
ALTER DATABASE "mr-zone-survival" ALTER LOCALITY REGIONAL CONFIGURE ZONE USING
389+
num_replicas = 2

‎pkg/kv/kvserver/raftstorebench/bench.go‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,26 +28,32 @@ func Run(t T, cfg Config) Result {
2828

2929
q := makeReplicaQueue(cfg.NumReplicas)
3030

31-
var wg sync.WaitGroup
3231
s := newAggStats()
3332

34-
statsCtx, stopStats := context.WithCancel(context.Background())
35-
defer stopStats()
36-
go statsLoop(t, statsCtx, cfg, s, raftEng, smEng)
33+
statsCtx, cancelCtx := context.WithCancel(context.Background())
34+
defer cancelCtx()
35+
var bgWG sync.WaitGroup
36+
bgWG.Add(1)
37+
go func() {
38+
defer bgWG.Done()
39+
statsLoop(t, statsCtx, cfg, s, raftEng, smEng)
40+
}()
3741

3842
o := writeOptions{
3943
cfg: cfg, smEng: smEng, raftEng: raftEng,
4044
keyLen: keyLen, valueLen: valueLen, batchLen: batchLen,
4145
}
4246
var durabilityCallbackCount atomic.Int64
4347
tStartWorkers := timeutil.Now()
48+
49+
var workerWG sync.WaitGroup
4450
for i := 0; i < cfg.NumWorkers; i++ {
45-
wg.Add(1)
51+
workerWG.Add(1)
4652
w := &worker{
4753
t: t, s: s, o: o, rng: rand.New(rand.NewSource(int64(i))),
4854
durabilityCallbackCount: &durabilityCallbackCount,
4955
}
50-
go w.run(t, q, &wg)
56+
go w.run(t, q, &workerWG)
5157
}
5258
logf(t, "started workers")
5359

@@ -63,7 +69,9 @@ func Run(t T, cfg Config) Result {
6369
var bytesFlushed uint64
6470
var n int
6571
notifyCh := make(chan struct{}, 1)
72+
bgWG.Add(1)
6673
go func() {
74+
defer bgWG.Done()
6775
for {
6876
select {
6977
case <-statsCtx.Done():
@@ -87,8 +95,9 @@ func Run(t T, cfg Config) Result {
8795
})
8896
}
8997

90-
wg.Wait()
91-
stopStats()
98+
workerWG.Wait()
99+
cancelCtx()
100+
bgWG.Wait() // make sure all goroutines are stopped by time engine closes
92101
duration := timeutil.Since(tStartWorkers)
93102
logf(t, "done working")
94103

‎pkg/sql/catalog/lease/lease.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1567,6 +1567,11 @@ type Manager struct {
15671567
bytesMonitor *mon.BytesMonitor
15681568
// boundAccount tracks the memory usage from leased descriptors.
15691569
boundAccount *mon.ConcurrentBoundAccount
1570+
1571+
// TestingDisableRangeFeedCheckpoint is used to disable rangefeed checkpoints.
1572+
// Ideally, this would be inside StorageTestingKnobs, but those get copied into
1573+
// the lease manager.
1574+
TestingDisableRangeFeedCheckpoint atomic.Bool
15701575
}
15711576

15721577
const leaseConcurrencyLimit = 5
@@ -2506,7 +2511,7 @@ func (m *Manager) watchForUpdates(ctx context.Context) {
25062511
ctx context.Context, ev *kvpb.RangeFeedValue,
25072512
) {
25082513
// Skip updates if rangefeeds are disabled.
2509-
if m.testingKnobs.DisableRangeFeedCheckpoint {
2514+
if m.TestingDisableRangeFeedCheckpoint.Load() {
25102515
return
25112516
}
25122517
// Check first if it is the special descriptor metadata key used to track
@@ -2572,7 +2577,7 @@ func (m *Manager) watchForUpdates(ctx context.Context) {
25722577
}
25732578

25742579
handleCheckpoint := func(ctx context.Context, checkpoint *kvpb.RangeFeedCheckpoint) {
2575-
if m.testingKnobs.DisableRangeFeedCheckpoint {
2580+
if m.TestingDisableRangeFeedCheckpoint.Load() {
25762581
return
25772582
}
25782583
// Track checkpoints that occur from the rangefeed to make sure progress
@@ -3150,7 +3155,7 @@ func (m *Manager) TestingSetDisableRangeFeedCheckpointFn(disable bool) chan stru
31503155
m.mu.Lock()
31513156
defer m.mu.Unlock()
31523157
m.mu.rangeFeedCheckpoints = 0
3153-
m.testingKnobs.DisableRangeFeedCheckpoint = disable
3158+
m.TestingDisableRangeFeedCheckpoint.Store(disable)
31543159
if disable {
31553160
m.testingKnobs.RangeFeedResetChannel = make(chan struct{})
31563161
} else {

‎pkg/sql/catalog/lease/testutils.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,6 @@ type ManagerTestingKnobs struct {
6161
// To disable the deletion of orphaned leases at server startup.
6262
DisableDeleteOrphanedLeases bool
6363

64-
// DisableRangeFeedCheckpoint is used to disable rangefeed checkpoints.
65-
DisableRangeFeedCheckpoint bool
66-
6764
// RangeFeedReset channel is closed to indicate that the range feed
6865
// has been reset.
6966
RangeFeedResetChannel chan struct{}

‎pkg/sql/catalog/multiregion/region_config.go‎

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -183,27 +183,24 @@ func (r *RegionConfig) ZoneConfigExtensions() descpb.ZoneConfigExtensions {
183183
// extensions to the provided zone configuration, returning the updated config.
184184
func (r *RegionConfig) ExtendZoneConfigWithGlobal(zc zonepb.ZoneConfig) (zonepb.ZoneConfig, error) {
185185
if ext := r.zoneCfgExtensions.Global; ext != nil {
186-
var numVoters, numReplicas int32
186+
var numVoters int32
187187
if zc.NumVoters != nil {
188188
numVoters = *zc.NumVoters
189189
}
190-
if zc.NumReplicas != nil {
191-
numReplicas = *zc.NumReplicas
192-
}
193190
zc = extendZoneCfg(zc, *ext)
194191
// TODO(janexing): to ensure that the zone config extension won't break the
195192
// survival goal, here we only add restriction on the number of voters and
196193
// replicas. We may want to consider adding constraint to their distribution
197194
// across zones/regions as well.
198195
if zc.NumVoters != nil && *zc.NumVoters < numVoters {
199-
return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+
200-
"cannot set num_voters %v that is lower than the one required for the "+
201-
"survival goal: %v with goal %v\n", *zc.NumVoters, numVoters, r.SurvivalGoal())
196+
return zonepb.ZoneConfig{}, errors.Newf(
197+
"cannot set num_voters below %v (num_voters required for survival goal %v), got %v",
198+
numVoters, r.SurvivalGoal(), *zc.NumVoters)
202199
}
203-
if zc.NumReplicas != nil && *zc.NumReplicas < numReplicas {
204-
return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+
205-
"cannot set num_replicas %v that is lower than the one required for the "+
206-
"survival goal: %v with goal %v\n", *zc.NumReplicas, numReplicas, r.SurvivalGoal())
200+
if zc.NumReplicas != nil && *zc.NumReplicas < numVoters {
201+
return zonepb.ZoneConfig{}, errors.Newf(
202+
"cannot set num_replicas below %v (num_voters required for survival goal %v), got %v",
203+
numVoters, r.SurvivalGoal(), *zc.NumReplicas)
207204
}
208205
}
209206
return zc, nil
@@ -215,13 +212,10 @@ func (r *RegionConfig) ExtendZoneConfigWithGlobal(zc zonepb.ZoneConfig) (zonepb.
215212
func (r *RegionConfig) ExtendZoneConfigWithRegionalIn(
216213
zc zonepb.ZoneConfig, region catpb.RegionName,
217214
) (zonepb.ZoneConfig, error) {
218-
var numVoters, numReplicas int32
215+
var numVoters int32
219216
if zc.NumVoters != nil {
220217
numVoters = *zc.NumVoters
221218
}
222-
if zc.NumReplicas != nil {
223-
numReplicas = *zc.NumReplicas
224-
}
225219

226220
if ext := r.zoneCfgExtensions.Regional; ext != nil {
227221
if ext.LeasePreferences != nil {
@@ -242,14 +236,14 @@ func (r *RegionConfig) ExtendZoneConfigWithRegionalIn(
242236
// replicas. We may want to consider adding constraint to their distribution
243237
// across zones/regions as well.
244238
if zc.NumVoters != nil && *zc.NumVoters < numVoters {
245-
return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+
246-
"cannot set num_voters %v that is lower than the one required for the "+
247-
"survival goal: %v with goal %v\n", *zc.NumVoters, numVoters, r.SurvivalGoal())
248-
}
249-
if zc.NumReplicas != nil && *zc.NumReplicas < numReplicas {
250-
return zonepb.ZoneConfig{}, errors.Newf("zone config extension "+
251-
"cannot set num_replica %v that is lower than the one required for the "+
252-
"survival goal: %v with goal %v\n", *zc.NumReplicas, numReplicas, r.SurvivalGoal())
239+
return zonepb.ZoneConfig{}, errors.Newf(
240+
"cannot set num_voters below %v (num_voters required for survival goal %v), got %v",
241+
numVoters, r.SurvivalGoal(), *zc.NumVoters)
242+
}
243+
if zc.NumReplicas != nil && *zc.NumReplicas < numVoters {
244+
return zonepb.ZoneConfig{}, errors.Newf(
245+
"cannot set num_replicas below %v (num_voters required for survival goal %v), got %v",
246+
numVoters, r.SurvivalGoal(), *zc.NumReplicas)
253247
}
254248
return zc, nil
255249
}

0 commit comments

Comments
 (0)