cockroachdb / cockroachdb/cockroach

mma: handle decommissioned stores

Open
#172,212 2 comments 0 reactions 0 assignees View on GitHub
A-kv-distribution C-bug P-2
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

When a store decommissions, MMA never removes its entry from clusterState.stores, nodeState.stores, or constraintMatcher.stores/allStores. The decommissioned store's last-reported Load and Capacity remain frozen in its storeState indefinitely (no further StoreLoadMsg ever updates it).

These stale values are then averaged into the cluster-wide mean used by the MMA shedding decision in (*rebalanceEnv).rebalanceStores (cluster_state_rebalance_stores.go:220):

`clusterMeans, ok := re.meansMemo.getMeans(nil)`

```
  for _, ss := range re.stores {
      sls := re.meansMemo.getStoreLoadSummary(ctx, clusterMeans, storeID, ss.loadSeqNum)
      if sls.sls >= overloadSlow { /* add to shedding list */ }
  }
```

  There is no disposition filter between getMeans(nil) and the per-store classification at line 246. The classifier compares each live store against a mean that includes ghost load/capacity, so it can mis-classify live stores as over- or
  under-loaded depending on what the ghost was doing when it left.

**Why the disposition gate doesn't help here: dispositions are checked in two places:**
- Target evaluation (allocator_state.go:962-1002) — retainReadyReplicaTargetStoresOnly drops refusing stores, and the mean is recomputed over the filtered set. Safe.
- Shedding-from check (comment at cluster_state_rebalance_stores.go:230-233) — we don't try to shed from a fdDrain / fdDead / storeMembershipRemoving store.

  Neither path filters the cluster-wide mean itself. The shedding list is filtered downstream by disposition, but only after the polluted mean has already biased who got onto the list.

  Append-only maps

  - clusterState.stores: only written by insert; no delete anywhere on the top-level map.
  - nodeState.stores: only appended to at cluster_state.go:2361.
  - constraintMatcher.stores / allStores: only inserted at constraint_matcher.go:66. The header comment at constraint_matcher.go:18 references a cm.removeStore(...) API that was never implemented.

 **Impact**

  - Correctness: the cluster mean used to classify overload is biased by stale data from gone stores. The magnitude depends on cluster size and the ghost's frozen values, but the invariant "the mean reflects live cluster state" is silently
  violated.
  - Surfaced by: an observability metric introduced in #1009 (mma.store.cpu.immovable) uses len(ns.stores) as a divisor and becomes permanently wrong after a decommission until process restart.

**Fix**

Implement the missing removal path: when a store is decommissioned/removed, delete it from clusterState.stores, the relevant nodeState.stores, and constraintMatcher.stores/allStores. Wire this from the same path that flips the disposition to refusing.

Jira issue: CRDB-65377

Contributor guide

Open the contributing guide

Research direction

Trace the disposition-to-refusing path and the store insertions in cluster_state.go, cluster_state_rebalance_stores.go, allocator_state.go, and constraint_matcher.go. Verify how clusterState.stores, nodeState.stores, and constraintMatcher.stores/allStores are populated before deciding where removal must be wired. Done means decommissioned stores leave all named collections and no longer affect MMA means or the immovable-store metric.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.