repro for oxidecomputer/omicron#10658 - #71
Draft
sunshowers wants to merge 4 commits into
Draft
Conversation
iliana
reviewed
Jun 26, 2026
Comment on lines
+1
to
+9
| // 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. |
Collaborator
There was a problem hiding this comment.
Assuming we wrote this and it wasn't largely copied/pasted from another similar test:
Suggested change
| // 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. | |
| // Copyright 2026 Oxide Computer Company | |
| // | |
| // Use of this software is governed by the Apache License, Version 2.0, | |
| // included in the file licenses/APL.txt. |
If it was copied from another test please let me know the file so I can determine whether the changes were significant enough to warrant adding Oxide to this header.
Drive the effective replication factor exclusively by operator policy — the configured RF and operator decommissioning — never by transient cluster health. ComputeAction floors neededVoters at the range's own non-decommissioned voter count (capped at the configured RF), using the authoritative range descriptor. A dead-but-not-decommissioned voter still counts, so a cold liveness cache can no longer size a healthy range below the replicas it has and trim it (the omicron#10658 trigger). Lowering num_replicas or decommissioning (both operator policy) still reduce the effective RF. Unlike experiment 1 (dropping the downshift), this keeps GetNeededVoters intact, so genuinely small / bringing-up clusters stay happy (no purgatory churn) and decommissioning still completes. Verified: madrid trim becomes a no-op; a 5->3 decommission completes; small clusters and the dead-but-present control are correct. Two DynamicNumReplicas cases shift from trimming a healthy/dead-bearing even-4 range toward replacing/keeping replicas, per the don't-reduce-RF-for- transient-state invariant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…he gauges Two fixes from the policy-floor review (policy-floor-review.md in the findings repo): 1. Key the floor exclusion on liveness membership, not store-pool status. The floor subtracted decommissioningVoters, which keys on storeStatusDecommissioning and therefore only matches LIVE decommissioning nodes. A dead node under decommission reports DECOMMISSIONED (dead + non-active membership), which classifies as storeStatusDead, so it kept counting toward the floor: its replica could only ever be replaced, never shed, and on a cluster with no spare node the replacement has no allocation target, stalling the decommission in purgatory forever. This violated the floor's own principle: decommissioning is operator policy and must be able to lower the effective RF regardless of node health. Subtract replicas whose NodeLivenessStatus is DECOMMISSIONING or DECOMMISSIONED instead: membership is the policy bit, store status mixes health back in. UNAVAILABLE decommissioning nodes stay unsubtracted deliberately, since excluding them while they sit in neither the dead nor the decommissioning status set would open a liveness-blind RemoveVoter window; they resolve to live or dead within the store-dead threshold. The invariant is preserved: RemoveVoter still fires only when haveVoters exceeds the configured RF. 2. Apply the same floor in calcRangeCounter so the gauges agree with the allocator. Without it, a phantom-low count blinds ranges_underreplicated exactly when it matters, false-alarms ranges_overreplicated on every healthy RF-5 range, and reports states the allocator deliberately preserves (4 voters on 4 nodes after a shrink) as over-replicated forever. With it, a healthy range whose voters the leaseholder cannot account for reads under-replicated: the honest signal that the view is degraded. Genuine over-replication versus the configured RF is still detected via the cap. Verified: TestAllocator*, TestStorePool*, *Decommission*, TestReplicateQueue*, *Metrics* all pass. New coverage: dead-node decommission with and without a spare node (completes in both; settles at 4 voters without a spare, at the configured RF 5 with one), the unavailable-decommissioning pause, the even steady state, and the floored gauges.
No behavior change. The floor arithmetic was duplicated inline in computeAction and calcRangeCounter with two subtly different exclusion predicates, and could only be tested through the full allocator and metrics harnesses. Two copies of logic that must agree, drifting independently, is the same failure mode this incident is about, so consolidate: allocator_policyfloor.go now holds the whole concept: - policyFloorNeededVoters: the floor arithmetic as a pure function of (needed, have, policyRemoved, configured). - nodeLivenessStatusIsPolicyRemoved: the allocator-side predicate, an exhaustive switch over the status enum with a fail-safe default. - policyRemovedVoterCount: the metric-side count for IsLiveMap consumers, with the deliberate allocator/gauge divergence for unavailable-but-not-yet-dead decommissioning nodes documented in one place. computeAction, calcRangeCounter, and the StorePool helper reduce to calls into these; the long inline rationale comments move to the helpers. New unit tests, no cluster harness required: - TestPolicyFloorNeededVoters: named scenarios (madrid, decommission, dead-node decommission, even state, RF lowered, over-RF). - TestPolicyFloorNeededVotersInvariants: exhaustive enumeration over the input space (replica counts and RFs live in [0, 7], so full coverage is a few thousand cases and beats sampling): the floor never lowers the target, never exceeds max(needed, configured RF), never invents replicas, is monotone in policy removals, idempotent, and never sizes a healthy in-RF range below the replicas it already has (the madrid invariant). - TestNodeLivenessStatusIsPolicyRemoved: exhaustive over the enum via its generated name map, so a newly added status fails the test and forces an explicit policy classification instead of silently inheriting the default. - TestPolicyRemovedVoterCount: membership/absence cases for the gauge path. Verified unchanged behavior: TestAllocator*, TestStorePool*, *Decommission*, TestReplicateQueue*, *Metrics* all pass unmodified; go vet clean.
sunshowers
force-pushed
the
omicron-10658-repro
branch
from
July 6, 2026 23:27
111db48 to
ec55fa4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Run this with: