Skip to content

Commit

Permalink
kvserver: avoid clobbering replica conf
Browse files Browse the repository at this point in the history
Fixes cockroachdb#75109. There are two ways to read the configuration applied over
a given replica:
  (a) the copy embedded in each replica struct
  (b) spanconfig.StoreReader, hanging off the store struct

The interface in (b) is implemented by both the span configs
infrastructure and the legacy system config span it's designed to
replace; it's typically used by KV queues (see cockroachdb#69172). When switching
between subsystems in KV (controlled by spanconfig.store.enabled), for
we transparently switch the source for (b). We also use then use the
right source to refresh (a) for every replica. Incremental updates
thereafter mutate (a) directly. As you'd expect, we need to take special
care that only one subsystem is writing to (a) at a given point in time,
a guarantee that was not upheld before this commit. This bug manifested
after we enabled span configs by default (see cockroachdb#73876), and likely
affected many tests.

To avoid the system config span from clobbering (a) when span configs
are enabled, we just need to check what spanconfig.store.enabled
holds at the right spot. To repro:

    # Reliably fails with 1-2m on my GCE worker before this patch,
    # doesn't after.
    dev test pkg/kv/kvserver \
      -f TestBackpressureNotAppliedWhenReducingRangeSize \
      -v --show-logs --timeout 15m --stress

Release note: None
  • Loading branch information
irfansharif committed Jan 21, 2022
1 parent 0a40796 commit 0a6897d
Showing 1 changed file with 7 additions and 3 deletions.
10 changes: 7 additions & 3 deletions pkg/kv/kvserver/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -1950,9 +1950,9 @@ func (s *Store) Start(ctx context.Context, stopper *stop.Stopper) error {
s.onSpanConfigUpdate(ctx, update)
})

// When toggling between the system config span and the span configs
// infrastructure, we want to re-apply configs on all replicas from
// whatever the new source is.
// When toggling between the system config span and the span
// configs infrastructure, we want to re-apply configs on all
// replicas from whatever the new source is.
spanconfigstore.EnabledSetting.SetOnChange(&s.ClusterSettings().SV, func(ctx context.Context) {
enabled := spanconfigstore.EnabledSetting.Get(&s.ClusterSettings().SV)
if enabled {
Expand Down Expand Up @@ -2281,6 +2281,10 @@ func (s *Store) removeReplicaWithRangefeed(rangeID roachpb.RangeID) {
// systemGossipUpdate is a callback for gossip updates to
// the system config which affect range split boundaries.
func (s *Store) systemGossipUpdate(sysCfg *config.SystemConfig) {
if !s.cfg.SpanConfigsDisabled && spanconfigstore.EnabledSetting.Get(&s.ClusterSettings().SV) {
return // nothing to do
}

ctx := s.AnnotateCtx(context.Background())
s.computeInitialMetrics.Do(func() {
// Metrics depend in part on the system config. Compute them as soon as we
Expand Down

0 comments on commit 0a6897d

Please sign in to comment.