diff --git a/pkg/kv/kvserver/allocator.go b/pkg/kv/kvserver/allocator.go index 5ad1572660..dc2c5b76f0 100644 --- a/pkg/kv/kvserver/allocator.go +++ b/pkg/kv/kvserver/allocator.go @@ -689,6 +689,25 @@ func (a *Allocator) computeAction( // decommissioning/decommissioned nodes. clusterNodes := a.storePool.ClusterNodeCount() neededVoters := GetNeededVoters(conf.GetNumVoters(), clusterNodes) + + // TODO-RAINCLAUDE: experiment 2 for omicron#10658 — apply the policy floor + // to the allocator's target; see allocator_policyfloor.go for the rule and + // the helpers' invariants. clusterNodes above is the leaseholder's + // cache-based node count and can read phantom-low when the liveness cache + // is cold (the omicron#10658 trigger); the floor keeps a transient count + // from sizing a range below the voters it already holds on nodes the + // operator has not decommissioned, so the liveness-blind RemoveVoter branch + // below fires only when haveVoters exceeds the configured RF. The excluded + // set deliberately differs from decommissioningVoters above (status-keyed, + // live nodes only): a policy-removed voter is either live-decommissioning + // (handled by the RemoveDecommissioningVoter branch) or dead (handled by + // RemoveDeadVoter, whose removal candidates the replicate queue restricts + // to dead replicas). The range descriptor is authoritative + // (Raft-replicated), so this needs no KV scan. + policyRemovedVoters := a.storePool.decommissioningOrDecommissionedReplicas(voterReplicas) + neededVoters = policyFloorNeededVoters( + neededVoters, haveVoters, len(policyRemovedVoters), int(conf.GetNumVoters())) + desiredQuorum := computeQuorum(neededVoters) quorum := computeQuorum(haveVoters) diff --git a/pkg/kv/kvserver/allocator_policyfloor.go b/pkg/kv/kvserver/allocator_policyfloor.go new file mode 100644 index 0000000000..22d57fa1fe --- /dev/null +++ b/pkg/kv/kvserver/allocator_policyfloor.go @@ -0,0 +1,111 @@ +// Copyright 2014 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package kvserver + +import ( + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" + "github.com/cockroachdb/cockroach/pkg/roachpb" +) + +// TODO-RAINCLAUDE: omicron#10658 — the policy floor, in one place. The rule: +// the effective replication factor is a function of operator policy alone +// (the configured num_replicas and operator decommissioning), never of +// transient cluster health (a dead node, a cold liveness cache, a phantom-low +// node count). computeAction applies the floor to the allocator's target and +// calcRangeCounter applies it to the under-/over-replication gauges, both +// through the pure helpers below so the two sites cannot drift and the logic +// is directly unit-testable (see the TestPolicyFloor* tests). + +// TODO-RAINCLAUDE: the floor arithmetic. Returns neededVoters floored at the +// range's own voters on nodes the operator has not directed out of the +// cluster (haveVoters - policyRemovedVoters), capped at the configured RF. +// Pure; TestPolicyFloorNeededVotersInvariants proves the following properties +// by exhaustive enumeration: +// +// - The result never drops below neededVoters, so the downshift's +// small-cluster and decommission-completion behavior is preserved. +// - The result never exceeds max(neededVoters, configuredVoters), so +// lowering num_replicas still down-replicates. +// - The result never exceeds max(neededVoters, haveVoters - +// policyRemovedVoters): the floor cannot invent replicas. +// - With no policy-removed voters and haveVoters within the configured RF, +// the result is at least haveVoters — the madrid invariant: no phantom-low +// node count can size a healthy range below the replicas it already has, +// which is what authorized the omicron#10658 trim. +func policyFloorNeededVoters( + neededVoters, haveVoters, policyRemovedVoters, configuredVoters int, +) int { + policyFloor := haveVoters - policyRemovedVoters + if policyFloor > configuredVoters { + policyFloor = configuredVoters + } + if policyFloor > neededVoters { + return policyFloor + } + return neededVoters +} + +// TODO-RAINCLAUDE: the allocator-side exclusion predicate: is this node's +// liveness status the result of operator policy (decommissioning), such that +// its replica should not count toward the policy floor? True for exactly +// DECOMMISSIONING (live + non-active membership) and DECOMMISSIONED (dead + +// non-active membership; this is what a dead node under decommission reports, +// and it classifies as storeStatusDead — which is why the floor cannot key on +// storeStatusDecommissioning). Everything else is false, and false is always +// the fail-safe direction: an unexcluded replica keeps the floor high, which +// can pause a decommission but can never remove a live replica. In +// particular: UNKNOWN (no liveness record — the cold-cache state that +// triggered omicron#10658) and UNAVAILABLE (expired but not yet past the dead +// threshold; LivenessStatus reports this even for a decommissioning node, and +// excluding it here would open a liveness-blind RemoveVoter window in +// computeAction while the node sits in neither the dead nor the +// decommissioning status set). The default arm makes any future status +// fail-safe too; TestNodeLivenessStatusIsPolicyRemoved forces an explicit +// classification when a status is added. +func nodeLivenessStatusIsPolicyRemoved(status livenesspb.NodeLivenessStatus) bool { + switch status { + case livenesspb.NodeLivenessStatus_DECOMMISSIONING, + livenesspb.NodeLivenessStatus_DECOMMISSIONED: + return true + case livenesspb.NodeLivenessStatus_UNKNOWN, + livenesspb.NodeLivenessStatus_DEAD, + livenesspb.NodeLivenessStatus_UNAVAILABLE, + livenesspb.NodeLivenessStatus_LIVE, + livenesspb.NodeLivenessStatus_DRAINING: + return false + default: + return false + } +} + +// TODO-RAINCLAUDE: the metric-side exclusion count for calcRangeCounter, +// which has an IsLiveMap rather than a NodeLivenessFunc. Keyed on raw +// liveness membership because the map carries no dead-threshold +// classification; this diverges from nodeLivenessStatusIsPolicyRemoved only +// while a decommissioning node is unavailable-but-not-yet-dead (the allocator +// pauses; the gauge already treats the node as policy-removed), which is +// acceptable for an estimated gauge. A voter absent from the map does not +// count as removed — an unverifiable replica must not lower the target — so +// during a cold-cache window a healthy range reads as under-replicated (the +// honest signal that the leaseholder cannot account for its replicas) rather +// than over-replicated. +func policyRemovedVoterCount( + voters []roachpb.ReplicaDescriptor, livenessMap liveness.IsLiveMap, +) int { + removed := 0 + for _, rd := range voters { + if entry, ok := livenessMap[rd.NodeID]; ok && !entry.Membership.Active() { + removed++ + } + } + return removed +} diff --git a/pkg/kv/kvserver/allocator_policyfloor_test.go b/pkg/kv/kvserver/allocator_policyfloor_test.go new file mode 100644 index 0000000000..8bfcc755f5 --- /dev/null +++ b/pkg/kv/kvserver/allocator_policyfloor_test.go @@ -0,0 +1,467 @@ +// Copyright 2014 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package kvserver + +import ( + "context" + "testing" + "time" + + "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/stretchr/testify/require" +) + +// TODO-RAINCLAUDE: experiment 2 for omicron#10658. Verifies the policy-floor: +// the effective RF is driven only by operator policy (configured RF + +// decommissioning), never by a transient/cold-cache node count. Contrast with +// experiment 1 (drop the downshift): the floor keeps the downshift's *good* +// behavior — a genuinely small cluster stays happy instead of churning — while +// still neutralizing the madrid trim. The floor is keyed on liveness membership +// (DECOMMISSIONING or DECOMMISSIONED), not store-pool status, so that a DEAD +// node under decommission (which reports DECOMMISSIONED -> storeStatusDead) +// still lowers it; see the dead-node decommission scenarios. +func TestAllocatorPolicyFloorScenarios(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + var numNodes int + stopper, _, _, sp, _ := createTestStorePool(ctx, + TestTimeUntilStoreDeadOff, false, /* deterministic */ + func() int { return numNodes }, + livenesspb.NodeLivenessStatus_LIVE) + defer stopper.Stop(ctx) + a := MakeAllocator(sp, func(string) (time.Duration, bool) { + return 0, true + }, nil /* knobs */, nil /* storeMetrics */) + conf := roachpb.SpanConfig{NumReplicas: 5} + + type scenario struct { + name string + storeList []roachpb.StoreID + live []roachpb.StoreID + unavailable []roachpb.StoreID + dead []roachpb.StoreID + decommissioning []roachpb.StoreID + // decommissioned mocks NodeLivenessStatus_DECOMMISSIONED, which is what a + // DEAD node with DECOMMISSIONING (or DECOMMISSIONED) membership reports — + // it classifies as storeStatusDead, not storeStatusDecommissioning. + decommissioned []roachpb.StoreID + nodeCount int + wantAction AllocatorAction + // what experiment 1 (bare drop of the downshift) produced, for contrast + exp1Action AllocatorAction + // what the original downshifting code produced + oldAction AllocatorAction + } + + scenarios := []scenario{ + { + name: "madrid cold cache: 5 live healthy, 0 decommissioning, phantom count 3", + storeList: []roachpb.StoreID{1, 2, 3, 4, 5}, + live: []roachpb.StoreID{1, 2, 3, 4, 5}, + nodeCount: 3, + wantAction: AllocatorConsiderRebalance, // FIXED (floored to 5) + exp1Action: AllocatorConsiderRebalance, + oldAction: AllocatorRemoveVoter, // the bug + }, + { + name: "decommission: range has 5 voters, 2 decommissioning, count 3", + storeList: []roachpb.StoreID{1, 2, 3, 4, 5}, + live: []roachpb.StoreID{1, 2, 3}, + decommissioning: []roachpb.StoreID{4, 5}, + nodeCount: 3, + wantAction: AllocatorRemoveDecommissioningVoter, // proceeds + exp1Action: AllocatorReplaceDecommissioningVoter, + oldAction: AllocatorRemoveDecommissioningVoter, + }, + { + name: "small cluster: RF5 range on 3 nodes, only 3 exist, 0 decommissioning", + storeList: []roachpb.StoreID{1, 2, 3}, + live: []roachpb.StoreID{1, 2, 3}, + nodeCount: 3, + wantAction: AllocatorConsiderRebalance, // happy — NO churn (floor==downshift==3) + exp1Action: AllocatorAddVoter, // experiment 1 churned here + oldAction: AllocatorConsiderRebalance, + }, + { + name: "dead-but-present (control): 2 dead, warm cache counts 5", + storeList: []roachpb.StoreID{1, 2, 3, 4, 5}, + live: []roachpb.StoreID{1, 2, 3}, + dead: []roachpb.StoreID{4, 5}, + nodeCount: 5, + wantAction: AllocatorReplaceDeadVoter, + exp1Action: AllocatorReplaceDeadVoter, + oldAction: AllocatorReplaceDeadVoter, + }, + { + name: "dead + cold cache: 2 dead (NOT decommissioned), phantom count 3", + storeList: []roachpb.StoreID{1, 2, 3, 4, 5}, + live: []roachpb.StoreID{1, 2, 3}, + dead: []roachpb.StoreID{4, 5}, + nodeCount: 3, + wantAction: AllocatorReplaceDeadVoter, // floored to 5: keep trying to maintain RF, never trim + exp1Action: AllocatorReplaceDeadVoter, + oldAction: AllocatorRemoveDeadVoter, // old code trimmed toward 3 + }, + { + // A node died permanently and the operator decommissions it. The + // membership-keyed floor excludes it (floor 4, not 5), so the dead + // replica is REMOVED and the decommission completes even with no spare + // node. The status-keyed floor (the original experiment 2 formula) kept + // needed at 5 and produced ReplaceDeadVoter, which has no allocation + // target on the 4-node remainder: purgatory forever, and the + // decommission never reaches zero replicas. + name: "dead-node decommission, no spare: 1 decommissioned(dead), count 4", + storeList: []roachpb.StoreID{1, 2, 3, 4, 5}, + live: []roachpb.StoreID{1, 2, 3, 4}, + decommissioned: []roachpb.StoreID{5}, + nodeCount: 4, + wantAction: AllocatorRemoveDeadVoter, + exp1Action: AllocatorReplaceDeadVoter, // stalls: no 5th node + oldAction: AllocatorRemoveDeadVoter, + }, + { + // Same, but a spare node 6 exists (the Oxide add-then-decommission + // flow). The warm count keeps needed at 5 above the floor of 4, so the + // dead replica is replaced onto the spare and the range stays at the + // configured RF. + name: "dead-node decommission, spare exists: count 5", + storeList: []roachpb.StoreID{1, 2, 3, 4, 5}, + live: []roachpb.StoreID{1, 2, 3, 4, 6}, + decommissioned: []roachpb.StoreID{5}, + nodeCount: 5, + wantAction: AllocatorReplaceDeadVoter, + exp1Action: AllocatorReplaceDeadVoter, + oldAction: AllocatorReplaceDeadVoter, + }, + { + // A decommissioning node that is transiently unreachable reports + // UNAVAILABLE (LivenessStatus collapses membership for expired-but-not- + // yet-dead nodes) -> storeStatusUnknown. It is deliberately NOT excluded + // from the floor: the decommission pauses until the node resolves to + // live (remove decommissioning) or dead (remove dead). Excluding it + // would open a liveness-blind RemoveVoter window. Note the old code + // trimmed a LIVE voter here — the madrid mechanism without any cache + // coldness on the affected node. + name: "decommissioning node transiently unavailable, phantom count 4", + storeList: []roachpb.StoreID{1, 2, 3, 4, 5}, + live: []roachpb.StoreID{1, 2, 3, 4}, + unavailable: []roachpb.StoreID{5}, + nodeCount: 4, + wantAction: AllocatorConsiderRebalance, // pause, fail-safe + exp1Action: AllocatorConsiderRebalance, + oldAction: AllocatorRemoveVoter, // blind trim of a live voter + }, + { + // The even steady state after a 5->4 shrink: the floor holds 4 healthy + // voters on a 4-node cluster rather than trimming to 3. The even-quorum + // nicety yields to the don't-trim-healthy-replicas invariant; the + // operator escape hatch is lowering num_replicas (policy, honored via + // the cap). + name: "even steady state: 4 healthy voters, 4 nodes", + storeList: []roachpb.StoreID{1, 2, 3, 4}, + live: []roachpb.StoreID{1, 2, 3, 4}, + nodeCount: 4, + wantAction: AllocatorConsiderRebalance, + exp1Action: AllocatorAddVoter, // wants a 5th node that does not exist + oldAction: AllocatorRemoveVoter, // trimmed the even 4 to 3 + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + numNodes = s.nodeCount + mockStorePool(sp, s.live, s.unavailable, s.dead, s.decommissioning, s.decommissioned, nil) + desc := makeDescriptor(s.storeList) + desc.EndKey = roachpb.RKey(keys.SystemPrefix) + action, _ := a.ComputeAction(ctx, conf, &desc) + require.Equalf(t, s.wantAction.String(), action.String(), + "policy-floor wants %s (experiment 1 gave %s; original downshift gave %s)", + s.wantAction, s.exp1Action, s.oldAction) + }) + } +} + +// TODO-RAINCLAUDE: experiment 2 — a full decommission of two nodes from a 5-node +// RF-5 cluster must still complete under the policy floor. Each step removes a +// decommissioning voter until the range settles at 3 and is happy. +func TestAllocatorPolicyFloorDecommissionCompletes(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + var numNodes int + stopper, _, _, sp, _ := createTestStorePool(ctx, + TestTimeUntilStoreDeadOff, false, /* deterministic */ + func() int { return numNodes }, + livenesspb.NodeLivenessStatus_LIVE) + defer stopper.Stop(ctx) + a := MakeAllocator(sp, func(string) (time.Duration, bool) { + return 0, true + }, nil /* knobs */, nil /* storeMetrics */) + conf := roachpb.SpanConfig{NumReplicas: 5} + + // Operator decommissions nodes 4 and 5 of a 5-node cluster. 3 nodes remain + // (count excludes decommissioning), so the cluster genuinely shrinks below RF. + numNodes = 3 + + steps := []struct { + storeList []roachpb.StoreID + decommissioning []roachpb.StoreID + live []roachpb.StoreID + wantAction AllocatorAction + }{ + {[]roachpb.StoreID{1, 2, 3, 4, 5}, []roachpb.StoreID{4, 5}, []roachpb.StoreID{1, 2, 3}, AllocatorRemoveDecommissioningVoter}, + {[]roachpb.StoreID{1, 2, 3, 5}, []roachpb.StoreID{5}, []roachpb.StoreID{1, 2, 3}, AllocatorRemoveDecommissioningVoter}, + {[]roachpb.StoreID{1, 2, 3}, nil, []roachpb.StoreID{1, 2, 3}, AllocatorConsiderRebalance}, // settled + } + + for i, s := range steps { + mockStorePool(sp, s.live, nil, nil, s.decommissioning, nil, nil) + desc := makeDescriptor(s.storeList) + desc.EndKey = roachpb.RKey(keys.SystemPrefix) + action, _ := a.ComputeAction(ctx, conf, &desc) + require.Equalf(t, s.wantAction.String(), action.String(), "decommission step %d", i) + } +} + +// TODO-RAINCLAUDE: membership-keyed floor fix — decommissioning a DEAD node +// must complete. Without a spare node the dead replica is removed and the range +// settles at 4 voters (the even state the floor preserves); with a spare it is +// replaced and the range stays at the configured RF 5. Before the fix the +// no-spare path returned ReplaceDeadVoter forever (no allocation target -> +// purgatory) and the node could never finish decommissioning. +func TestAllocatorPolicyFloorDeadNodeDecommission(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + var numNodes int + stopper, _, _, sp, _ := createTestStorePool(ctx, + TestTimeUntilStoreDeadOff, false, /* deterministic */ + func() int { return numNodes }, + livenesspb.NodeLivenessStatus_LIVE) + defer stopper.Stop(ctx) + a := MakeAllocator(sp, func(string) (time.Duration, bool) { + return 0, true + }, nil /* knobs */, nil /* storeMetrics */) + conf := roachpb.SpanConfig{NumReplicas: 5} + + steps := []struct { + name string + storeList []roachpb.StoreID + live []roachpb.StoreID + decommissioned []roachpb.StoreID + nodeCount int + wantAction AllocatorAction + }{ + // No spare: node 5 is dead and being decommissioned on what is now a + // 4-node cluster. The dead replica is removed, then the range settles at + // the even 4-voter state. + { + name: "no spare: shed the dead replica", + storeList: []roachpb.StoreID{1, 2, 3, 4, 5}, + live: []roachpb.StoreID{1, 2, 3, 4}, + decommissioned: []roachpb.StoreID{5}, + nodeCount: 4, + wantAction: AllocatorRemoveDeadVoter, + }, + { + name: "no spare: settled at 4", + storeList: []roachpb.StoreID{1, 2, 3, 4}, + live: []roachpb.StoreID{1, 2, 3, 4}, + decommissioned: []roachpb.StoreID{5}, + nodeCount: 4, + wantAction: AllocatorConsiderRebalance, + }, + // With a spare (the Oxide add-then-decommission flow): the dead replica is + // replaced onto node 6, then the range is settled at the configured RF. + { + name: "spare: replace the dead replica", + storeList: []roachpb.StoreID{1, 2, 3, 4, 5}, + live: []roachpb.StoreID{1, 2, 3, 4, 6}, + decommissioned: []roachpb.StoreID{5}, + nodeCount: 5, + wantAction: AllocatorReplaceDeadVoter, + }, + { + name: "spare: settled at RF 5", + storeList: []roachpb.StoreID{1, 2, 3, 4, 6}, + live: []roachpb.StoreID{1, 2, 3, 4, 6}, + decommissioned: []roachpb.StoreID{5}, + nodeCount: 5, + wantAction: AllocatorConsiderRebalance, + }, + } + + for _, s := range steps { + t.Run(s.name, func(t *testing.T) { + numNodes = s.nodeCount + mockStorePool(sp, s.live, nil, nil, nil, s.decommissioned, nil) + desc := makeDescriptor(s.storeList) + desc.EndKey = roachpb.RKey(keys.SystemPrefix) + action, _ := a.ComputeAction(ctx, conf, &desc) + require.Equalf(t, s.wantAction.String(), action.String(), "step %q", s.name) + }) + } +} + +// TODO-RAINCLAUDE: unit tests for the pure helpers in +// allocator_policyfloor.go. Named scenarios below; the invariants are proved +// separately by exhaustive enumeration in +// TestPolicyFloorNeededVotersInvariants. +func TestPolicyFloorNeededVoters(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + cases := []struct { + name string + needed, have, policyRemoved, configured, want int + }{ + {"madrid: healthy RF-5 range, phantom count downshifted needed to 3", 3, 5, 0, 5, 5}, + {"small cluster: RF-5 range with 3 voters on 3 nodes", 3, 3, 0, 5, 3}, + {"under-replicated: the floor never lowers the target", 5, 3, 0, 5, 5}, + {"live decommission 2 of 5, phantom count", 3, 5, 2, 5, 3}, + {"dead-node decommission 1 of 5", 3, 5, 1, 5, 4}, + {"even steady state: 4 healthy voters on 4 nodes", 3, 4, 0, 5, 4}, + {"over-replicated beyond the configured RF", 5, 6, 0, 5, 5}, + {"over-replicated beyond the configured RF, phantom count", 3, 6, 0, 5, 5}, + {"num_replicas lowered 5 -> 3: the cap lets the trim proceed", 3, 5, 0, 3, 3}, + {"all voters policy-removed", 3, 5, 5, 5, 3}, + {"no voters", 3, 0, 0, 5, 3}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + require.Equal(t, c.want, + policyFloorNeededVoters(c.needed, c.have, c.policyRemoved, c.configured)) + }) + } +} + +// TODO-RAINCLAUDE: the floor's invariants, proved by exhaustive enumeration. +// Real replica counts and RFs live in [0, 7], so full coverage of the +// meaningful input space is a few thousand cases — strictly stronger than +// sampling and dependency-free. +func TestPolicyFloorNeededVotersInvariants(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + const max = 7 + for needed := 0; needed <= max; needed++ { + for have := 0; have <= max; have++ { + for removed := 0; removed <= have; removed++ { + for configured := 0; configured <= max; configured++ { + got := policyFloorNeededVoters(needed, have, removed, configured) + args := []interface{}{needed, have, removed, configured} + + // The floor only ever raises the target: the downshift's + // small-cluster and decommission behavior is preserved. + require.GreaterOrEqualf(t, got, needed, + "floor lowered the target (needed=%d have=%d removed=%d configured=%d)", args...) + if got > needed { + // When it raises, it raises to no more than the configured RF + // (lowering num_replicas still down-replicates)... + require.LessOrEqualf(t, got, configured, + "floor exceeded the configured RF (needed=%d have=%d removed=%d configured=%d)", args...) + // ...and to no more than the voters actually held on + // policy-active nodes (the floor cannot invent replicas). + require.LessOrEqualf(t, got, have-removed, + "floor invented replicas (needed=%d have=%d removed=%d configured=%d)", args...) + } + // The madrid invariant: with no policy-removed voters and + // haveVoters within the configured RF, a range is never sized + // below the replicas it already has — no phantom-low node count + // can authorize a trim. + if removed == 0 && have <= configured { + require.GreaterOrEqualf(t, got, have, + "healthy in-RF range sized below itself (needed=%d have=%d removed=%d configured=%d)", args...) + } + // Monotone: one more policy-removed voter never raises the target. + if removed < have { + require.GreaterOrEqualf(t, got, + policyFloorNeededVoters(needed, have, removed+1, configured), + "extra policy removal raised the target (needed=%d have=%d removed=%d configured=%d)", args...) + } + // Idempotent: re-applying the floor changes nothing. + require.Equalf(t, got, policyFloorNeededVoters(got, have, removed, configured), + "floor is not idempotent (needed=%d have=%d removed=%d configured=%d)", args...) + } + } + } + } +} + +// TODO-RAINCLAUDE: exhaustive over the NodeLivenessStatus enum via its +// generated name map, so adding a status to the proto forces an explicit +// policy-floor classification here instead of silently inheriting the +// fail-safe default. +func TestNodeLivenessStatusIsPolicyRemoved(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + want := map[livenesspb.NodeLivenessStatus]bool{ + livenesspb.NodeLivenessStatus_UNKNOWN: false, // no liveness record (cold cache): fail-safe + livenesspb.NodeLivenessStatus_DEAD: false, // deadness is health, not policy + livenesspb.NodeLivenessStatus_UNAVAILABLE: false, // excluding it would open a blind RemoveVoter window + livenesspb.NodeLivenessStatus_LIVE: false, + livenesspb.NodeLivenessStatus_DECOMMISSIONING: true, // operator policy, node live + livenesspb.NodeLivenessStatus_DECOMMISSIONED: true, // operator policy, node dead + livenesspb.NodeLivenessStatus_DRAINING: false, // restart in progress, not removal + } + + for value, name := range livenesspb.NodeLivenessStatus_name { + status := livenesspb.NodeLivenessStatus(value) + expected, ok := want[status] + require.Truef(t, ok, + "NodeLivenessStatus %s has no policy-floor classification; decide whether it represents operator removal", + name) + require.Equalf(t, expected, nodeLivenessStatusIsPolicyRemoved(status), "status %s", name) + } + require.Len(t, want, len(livenesspb.NodeLivenessStatus_name)) +} + +func TestPolicyRemovedVoterCount(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + voters := []roachpb.ReplicaDescriptor{ + {NodeID: 1, StoreID: 10, ReplicaID: 1}, + {NodeID: 2, StoreID: 20, ReplicaID: 2}, + {NodeID: 3, StoreID: 30, ReplicaID: 3}, + {NodeID: 4, StoreID: 40, ReplicaID: 4}, + {NodeID: 5, StoreID: 50, ReplicaID: 5}, + } + livenessMap := liveness.IsLiveMap{ + // Node 1 is deliberately absent from the map: an unverifiable replica + // counts toward the floor, not as removed. + 2: {IsLive: true}, // membership active, live + 3: {IsLive: false}, // membership active, dead: health, not policy + 4: { // live decommissioning + Liveness: livenesspb.Liveness{Membership: livenesspb.MembershipStatus_DECOMMISSIONING}, + IsLive: true, + }, + 5: { // dead and fully decommissioned + Liveness: livenesspb.Liveness{Membership: livenesspb.MembershipStatus_DECOMMISSIONED}, + IsLive: false, + }, + } + + require.Equal(t, 2, policyRemovedVoterCount(voters, livenessMap)) + require.Equal(t, 0, policyRemovedVoterCount(nil, livenessMap)) + require.Equal(t, 0, policyRemovedVoterCount(voters, liveness.IsLiveMap{})) +} diff --git a/pkg/kv/kvserver/allocator_rf_collapse_test.go b/pkg/kv/kvserver/allocator_rf_collapse_test.go new file mode 100644 index 0000000000..69770e8051 --- /dev/null +++ b/pkg/kv/kvserver/allocator_rf_collapse_test.go @@ -0,0 +1,133 @@ +// Copyright 2014 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package kvserver + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" + "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" + "github.com/cockroachdb/cockroach/pkg/util/hlc" + "github.com/cockroachdb/cockroach/pkg/util/leaktest" + "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/stretchr/testify/require" +) + +// Test demonstrating the behaviors that led to omicron#10658. +func TestAllocatorDownReplicatesOnColdLivenessCache(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + + var coldLiveness *liveness.NodeLiveness + stopper, g, mc, sp, _ := createTestStorePool(ctx, + TestTimeUntilStoreDeadOff, false, /* deterministic */ + func() int { + if coldLiveness == nil { + return 0 // not reached in this test; nodeCountFn is unused before construction + } + return coldLiveness.GetNodeCount() + }, + livenesspb.NodeLivenessStatus_LIVE) + defer stopper.Stop(ctx) + + // We don't Start() the NodeLiveness, since we are about only the cache and + // GetNodeCount(), not the heartbeat loop. + clock := hlc.NewClock(mc.UnixNano, time.Nanosecond) + coldLiveness = liveness.NewNodeLiveness(liveness.NodeLivenessOptions{ + AmbientCtx: log.MakeTestingAmbientContext(stopper.Tracer()), + Stopper: stopper, + Settings: cluster.MakeTestingClusterSettings(), + Gossip: g, + Clock: clock, + LivenessThreshold: time.Minute, + RenewalDuration: time.Second, + HistogramWindowInterval: time.Minute, + }) + + a := MakeAllocator(sp, func(string) (time.Duration, bool) { + return 0, true + }, nil /* knobs */, nil /* storeMetrics */) + + // RF 5, with 5 healthy replicas on 5 live stores. + conf := roachpb.SpanConfig{NumReplicas: 5} + allFive := []roachpb.StoreID{1, 2, 3, 4, 5} + + // TODO-RAINCLAUDE: experiment 2 for omicron#10658 (policy-floor the effective + // RF). Note the difference from experiment 1 (which removed the downshift from + // GetNeededVoters): here GetNeededVoters is UNCHANGED, so `expectedNumReplicas` + // still reflects the downshift — a cold cache still computes an effective RF of + // 3. What changed is ComputeAction: it floors the target at the range's own + // non-decommissioned replica count (5 here), so a healthy RF-5 range is no + // longer trimmed even though GetNeededVoters reports 3. The bug was the + // *removal*, and the floor blocks it while leaving the downshift formula (and + // thus the small-cluster and decommission behavior) intact. `oldExpectedAction` + // is the original buggy action. + testCases := []struct { + cacheRecords int + expectedNumReplicas int + oldExpectedAction AllocatorAction + expectedAction AllocatorAction + }{ + // GetNeededVoters still downshifts to 3 on a cold cache, but ComputeAction + // floors to the 5 live non-decommissioned replicas and does nothing. + {cacheRecords: 0, expectedNumReplicas: 3, oldExpectedAction: AllocatorRemoveVoter, expectedAction: AllocatorConsiderRebalance}, + {cacheRecords: 1, expectedNumReplicas: 3, oldExpectedAction: AllocatorRemoveVoter, expectedAction: AllocatorConsiderRebalance}, + {cacheRecords: 2, expectedNumReplicas: 3, oldExpectedAction: AllocatorRemoveVoter, expectedAction: AllocatorConsiderRebalance}, + {cacheRecords: 3, expectedNumReplicas: 3, oldExpectedAction: AllocatorRemoveVoter, expectedAction: AllocatorConsiderRebalance}, + {cacheRecords: 4, expectedNumReplicas: 3, oldExpectedAction: AllocatorRemoveVoter, expectedAction: AllocatorConsiderRebalance}, + // A warm cache (all 5 records) was correct before and after. + {cacheRecords: 5, expectedNumReplicas: 5, oldExpectedAction: AllocatorConsiderRebalance, expectedAction: AllocatorConsiderRebalance}, + } + + nextNode := roachpb.NodeID(1) + for _, c := range testCases { + t.Run(fmt.Sprintf("cacheRecords=%d", c.cacheRecords), func(t *testing.T) { + // Grow the real cache through maybeUpdate. + for coldLiveness.GetNodeCount() < c.cacheRecords { + coldLiveness.TestingMaybeUpdate(ctx, liveness.Record{ + Liveness: livenesspb.Liveness{ + NodeID: nextNode, + Epoch: 1, + Membership: livenesspb.MembershipStatus_ACTIVE, + }, + }) + nextNode++ + } + require.Equalf(t, c.cacheRecords, coldLiveness.GetNodeCount(), + "the real liveness cache should hold exactly %d of 5 records", c.cacheRecords) + + mockStorePool(sp, allFive, nil, nil, nil, nil, nil) + desc := makeDescriptor(allFive) + + clusterNodes := a.storePool.ClusterNodeCount() + require.Equalf(t, c.cacheRecords, clusterNodes, + "ClusterNodeCount() should flow from the real NodeLiveness.GetNodeCount() (%d records)", + c.cacheRecords) + + effectiveNumReplicas := GetNeededVoters(conf.NumReplicas, clusterNodes) + require.Equalf(t, c.expectedNumReplicas, effectiveNumReplicas, + "GetNeededVoters(5, %d) sizes a healthy RF-5 range to %d replicas", + clusterNodes, effectiveNumReplicas) + + action, _ := a.ComputeAction(ctx, conf, &desc) + require.Equalf(t, c.expectedAction.String(), action.String(), + "5 live healthy replicas + a liveness cache of %d/5 records -> allocator action %s", + c.cacheRecords, action) + }) + } +} diff --git a/pkg/kv/kvserver/allocator_test.go b/pkg/kv/kvserver/allocator_test.go index add12a9cb0..111f16789d 100644 --- a/pkg/kv/kvserver/allocator_test.go +++ b/pkg/kv/kvserver/allocator_test.go @@ -6683,13 +6683,15 @@ func TestAllocatorComputeActionDynamicNumReplicas(t *testing.T) { decommissioning: []roachpb.StoreID{1, 2, 3}, }, { - // Four live stores and one dead one, so the effective replication - // factor would be even (four), in which case we drop down one more - // to three. Then the right thing becomes removing the dead replica - // from the range at hand, rather than trying to replace it. + // Four replicas, one dead, none decommissioning. GetNeededVoters still + // downshifts to 3 (expectedNumReplicas), but the policy floor (experiment + // 2) holds the effective target at the 4 non-decommissioned replicas, so + // ComputeAction *replaces* the dead voter rather than trimming to 3. + // Deadness is transient and is not operator policy, so the RF is not + // reduced for it. (Was: AllocatorRemoveDeadVoter.) storeList: []roachpb.StoreID{1, 2, 3, 4}, expectedNumReplicas: 3, - expectedAction: AllocatorRemoveDeadVoter, + expectedAction: AllocatorReplaceDeadVoter, live: []roachpb.StoreID{1, 2, 3, 5}, unavailable: nil, dead: []roachpb.StoreID{4}, @@ -6741,10 +6743,16 @@ func TestAllocatorComputeActionDynamicNumReplicas(t *testing.T) { decommissioning: nil, }, { - // Three again, on account of avoiding the even four. + // Four healthy replicas, none decommissioning. The original code + // trimmed the even four down to three (GetNeededVoters still reports 3, + // expectedNumReplicas). The policy floor (experiment 2) holds the target + // at the 4 non-decommissioned replicas, so ComputeAction leaves the + // range alone: it will not remove a healthy replica to satisfy a + // transiently-derived target. The even-quorum nicety yields to the + // don't-trim-healthy-replicas invariant. (Was: AllocatorRemoveVoter.) storeList: []roachpb.StoreID{1, 2, 3, 4}, expectedNumReplicas: 3, - expectedAction: AllocatorRemoveVoter, + expectedAction: AllocatorConsiderRebalance, live: []roachpb.StoreID{1, 2, 3, 4}, unavailable: nil, dead: nil, diff --git a/pkg/kv/kvserver/replica_metrics.go b/pkg/kv/kvserver/replica_metrics.go index 7f0e8e6353..07be581a29 100644 --- a/pkg/kv/kvserver/replica_metrics.go +++ b/pkg/kv/kvserver/replica_metrics.go @@ -180,6 +180,16 @@ func calcRangeCounter( // unavailable ranges for each range based on the liveness table. if rangeCounter { neededVoters := GetNeededVoters(numVoters, clusterNodes) + // TODO-RAINCLAUDE: omicron#10658 — the same policy floor computeAction + // applies, so the gauges agree with the allocator; see + // allocator_policyfloor.go. Without it, a phantom-low clusterNodes makes + // ranges_underreplicated go blind exactly when it matters and false-alarms + // ranges_overreplicated on every healthy RF-5 range, and states the + // allocator deliberately preserves (e.g. 4 voters on 4 nodes after a + // decommission) read over-replicated forever. + voterDescs := desc.Replicas().VoterDescriptors() + neededVoters = policyFloorNeededVoters(neededVoters, len(voterDescs), + policyRemovedVoterCount(voterDescs, livenessMap), int(numVoters)) neededNonVoters := GetNeededNonVoters(int(numVoters), int(numReplicas-numVoters), clusterNodes) status := desc.Replicas().ReplicationStatus(func(rDesc roachpb.ReplicaDescriptor) bool { return livenessMap[rDesc.NodeID].IsLive diff --git a/pkg/kv/kvserver/replica_metrics_test.go b/pkg/kv/kvserver/replica_metrics_test.go index ff2b840217..f8ecd18271 100644 --- a/pkg/kv/kvserver/replica_metrics_test.go +++ b/pkg/kv/kvserver/replica_metrics_test.go @@ -15,6 +15,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness" + "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -138,6 +139,128 @@ func TestCalcRangeCounterIsLiveMap(t *testing.T) { } } +// TODO-RAINCLAUDE: omicron#10658 — the policy floor applied to the gauges, so +// they agree with the allocator. Covers: the madrid phantom window (healthy +// range no longer reads over-replicated; a range whose voters the leaseholder +// cannot account for reads under-replicated instead of quiet), the +// dead-decommission and even-steady states (no longer permanently +// over-replicated), and genuine over-replication versus the configured RF +// (still detected — the floor is capped at numVoters). +func TestCalcRangeCounterPolicyFloor(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + leaseStatus := kvserverpb.LeaseStatus{ + Lease: roachpb.Lease{ + Replica: roachpb.ReplicaDescriptor{ + NodeID: 1, + StoreID: 10, + }, + }, + State: kvserverpb.LeaseState_VALID, + } + + fiveVoters := roachpb.NewRangeDescriptor(123, roachpb.RKeyMin, roachpb.RKeyMax, + roachpb.MakeReplicaSet([]roachpb.ReplicaDescriptor{ + {NodeID: 1, StoreID: 10, ReplicaID: 1, Type: roachpb.ReplicaTypeVoterFull()}, + {NodeID: 2, StoreID: 20, ReplicaID: 2, Type: roachpb.ReplicaTypeVoterFull()}, + {NodeID: 3, StoreID: 30, ReplicaID: 3, Type: roachpb.ReplicaTypeVoterFull()}, + {NodeID: 4, StoreID: 40, ReplicaID: 4, Type: roachpb.ReplicaTypeVoterFull()}, + {NodeID: 5, StoreID: 50, ReplicaID: 5, Type: roachpb.ReplicaTypeVoterFull()}, + })) + + fourVoters := roachpb.NewRangeDescriptor(124, roachpb.RKeyMin, roachpb.RKeyMax, + roachpb.MakeReplicaSet([]roachpb.ReplicaDescriptor{ + {NodeID: 1, StoreID: 10, ReplicaID: 1, Type: roachpb.ReplicaTypeVoterFull()}, + {NodeID: 2, StoreID: 20, ReplicaID: 2, Type: roachpb.ReplicaTypeVoterFull()}, + {NodeID: 3, StoreID: 30, ReplicaID: 3, Type: roachpb.ReplicaTypeVoterFull()}, + {NodeID: 4, StoreID: 40, ReplicaID: 4, Type: roachpb.ReplicaTypeVoterFull()}, + })) + + live := liveness.IsLiveMapEntry{IsLive: true} + // A dead node under operator decommission: non-active membership, not live. + deadDecommissioning := liveness.IsLiveMapEntry{ + Liveness: livenesspb.Liveness{Membership: livenesspb.MembershipStatus_DECOMMISSIONING}, + IsLive: false, + } + + { + // The madrid phantom window: 5 healthy voters, all live and + // membership-active, but clusterNodes reads phantom-low. The floor holds + // needed at 5, so the range is neither over- nor under-replicated. + // Without the floor this read over-replicated (needed 3 < live 5). + ctr, down, under, over := calcRangeCounter(10, fiveVoters, leaseStatus, liveness.IsLiveMap{ + 1: live, 2: live, 3: live, 4: live, 5: live, + }, 5 /* numVoters */, 5 /* numReplicas */, 3 /* clusterNodes */) + + require.True(t, ctr) + require.False(t, down) + require.False(t, under) + require.False(t, over) + } + + { + // Phantom window with two voters absent from the liveness map (the actual + // madrid state on n1). Absent voters still count toward the floor, so + // needed stays 5 against 3 live: under-replicated — the honest signal that + // the leaseholder cannot account for two of its replicas. Without the + // floor this read quiet (needed 3 == live 3): blind exactly when it + // mattered. + ctr, down, under, over := calcRangeCounter(10, fiveVoters, leaseStatus, liveness.IsLiveMap{ + 1: live, 2: live, 3: live, + }, 5 /* numVoters */, 5 /* numReplicas */, 3 /* clusterNodes */) + + require.True(t, ctr) + require.False(t, down) + require.True(t, under) + require.False(t, over) + } + + { + // Dead-node decommission in progress: the non-active membership excludes + // node 5 from the floor (4, matching the allocator's target), so the range + // reads neither over- nor under-replicated while the dead replica is shed. + // Without the floor this read over-replicated (needed 3 < live 4) while + // the allocator was still repairing. + ctr, down, under, over := calcRangeCounter(10, fiveVoters, leaseStatus, liveness.IsLiveMap{ + 1: live, 2: live, 3: live, 4: live, 5: deadDecommissioning, + }, 5 /* numVoters */, 5 /* numReplicas */, 4 /* clusterNodes */) + + require.True(t, ctr) + require.False(t, down) + require.False(t, under) + require.False(t, over) + } + + { + // The even steady state: 4 healthy voters on a 4-node cluster. The floor + // holds needed at 4, so the state the allocator deliberately preserves is + // not reported as permanently over-replicated (needed 3 < live 4 before). + ctr, down, under, over := calcRangeCounter(10, fourVoters, leaseStatus, liveness.IsLiveMap{ + 1: live, 2: live, 3: live, 4: live, + }, 5 /* numVoters */, 5 /* numReplicas */, 4 /* clusterNodes */) + + require.True(t, ctr) + require.False(t, down) + require.False(t, under) + require.False(t, over) + } + + { + // Genuine over-replication versus the configured RF is still detected: + // the floor is capped at numVoters, so 5 live voters against a configured + // RF of 3 reads over-replicated. + ctr, down, under, over := calcRangeCounter(10, fiveVoters, leaseStatus, liveness.IsLiveMap{ + 1: live, 2: live, 3: live, 4: live, 5: live, + }, 3 /* numVoters */, 3 /* numReplicas */, 5 /* clusterNodes */) + + require.True(t, ctr) + require.False(t, down) + require.False(t, under) + require.True(t, over) + } +} + func TestCalcRangeCounterLeaseHolder(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/kv/kvserver/store_pool.go b/pkg/kv/kvserver/store_pool.go index 751f59264a..a95da32210 100644 --- a/pkg/kv/kvserver/store_pool.go +++ b/pkg/kv/kvserver/store_pool.go @@ -598,6 +598,27 @@ func (sp *StorePool) decommissioningReplicas( return } +// TODO-RAINCLAUDE: omicron#10658 policy floor — the replicas the operator has +// directed out of the cluster, per nodeLivenessStatusIsPolicyRemoved (see +// allocator_policyfloor.go for the predicate's rationale and fail-safe +// properties). Distinct from decommissioningReplicas above, which keys on +// storeStatusDecommissioning and therefore only matches LIVE decommissioning +// nodes; a dead node under decommission classifies as storeStatusDead but is +// still policy-removed. +func (sp *StorePool) decommissioningOrDecommissionedReplicas( + repls []roachpb.ReplicaDescriptor, +) (inactive []roachpb.ReplicaDescriptor) { + now := sp.clock.Now().GoTime() + timeUntilStoreDead := TimeUntilStoreDead.Get(&sp.st.SV) + + for _, repl := range repls { + if nodeLivenessStatusIsPolicyRemoved(sp.nodeLivenessFn(repl.NodeID, now, timeUntilStoreDead)) { + inactive = append(inactive, repl) + } + } + return inactive +} + // ClusterNodeCount returns the number of nodes that are possible allocation // targets. This includes dead nodes, but not decommissioning or decommissioned // nodes.