reparentutil: order reparent candidates by GTID dominance for a consistent sort (#20728)

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

Author: timvaillancourt

changelog/25.0/25.0.0/summary.md

@@ -30,6 +30,7 @@
         - [Stricter PROXY protocol v1 header validation](#vtgate-proxy-protocol-v1-strictness)
     - **[Reparent](#minor-changes-reparent)**
         - [`EmergencyReparentShard` no longer waits on replicas that cannot win the election](#ers-lagging-relay-log-wait)
+        - [Reparent candidate ordering now respects partially ordered GTID histories](#reparent-gtid-candidate-ordering)
     - **[VTTablet](#minor-changes-vttablet)**
         - [Consolidator Reject on Waiter Cap](#vttablet-consolidator-reject-on-cap)
         - [Query timeout for state-changing statements on the streaming path](#vttablet-stream-query-timeout)
@@ -279,6 +280,14 @@ This mirrors a tradeoff `orchestrator` made before Vitess: it never gated dead-p
 
 See [#18529](https://github.com/vitessio/vitess/issues/18529).
 
+#### <a id="reparent-gtid-candidate-ordering"/>Reparent candidate ordering now respects partially ordered GTID histories</a>
+
+GTID containment is pairwise, so a candidate set can mix comparable and divergent histories: candidate A at `p:1-100,a:1-10` is strictly ahead of B at `p:1-100,a:1-5`, while C at `p:1-100,c:1-3` is incomparable with both. The reparent sorter that both `EmergencyReparentShard` and `PlannedReparentShard` use compared such candidates non-transitively, so ordering could depend on map iteration or RPC completion order, and `PlannedReparentShard` could select B even though A was known to be more advanced.
+
+Candidates are now ordered by GTID dominance before the existing promotion-rule, buffer-pool, and tablet-alias tiebreakers, so a dominated candidate can never rank ahead of its dominator regardless of input order. `EmergencyReparentShard` still rejects incomparable candidates as split brain, and `PlannedReparentShard` still chooses among incomparable maximal candidates. Positions that contain each other without being equal (possible with MariaDB GTIDs, where containment ignores the origin server) are now also rejected by `EmergencyReparentShard` as split brain, wherever the pair sits among the candidates; previously a leading pair failed with an internal sorting error, while a pair behind a more advanced candidate was not detected at all.
+
+See [#20579](https://github.com/vitessio/vitess/issues/20579).
+
 ### <a id="minor-changes-vttablet"/>VTTablet</a>
 
 #### <a id="vttablet-consolidator-reject-on-cap"/>Consolidator Reject on Waiter Cap</a>

go/vt/vtctl/reparentutil/emergency_reparenter.go

@@ -789,16 +789,23 @@ func (erp *EmergencyReparenter) findMostAdvanced(
 	// We have already removed the tablets with errant GTIDs before calling this function. At this point our winning position must be a
 	// superset of all the other valid positions. If any position is incomparable with it, then we have a split brain scenario, and we
 	// should cancel the ERS. Split brain is about divergent received history, so we only compare the Combined positions; the Executed
-	// positions can be transiently incomparable at an equal Combined position (multi-threaded apply gaps) without any divergence
+	// positions can be transiently incomparable at an equal Combined position (multi-threaded apply gaps) without any divergence.
+	// Reciprocally contained but unequal positions are divergent too, containment just can't order them (MariaDB GTID containment
+	// ignores the origin server), so they must also fail closed. The divergent pair can sit behind a candidate that dominates both
+	// of them, so reciprocal containment is checked between every pair of candidates, not just against the winning position
 	for i, position := range tabletPositions {
 		if haveIncomparablePositions(winningPosition.Combined, position.Combined) {
 			return nil, nil, vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "split brain detected between servers - %s and %s", topoproto.TabletAliasString(winningPrimaryTablet.Alias), topoproto.TabletAliasString(validTablets[i].Alias))
 		}
-		// The sort can't guarantee a maximum at index 0 when some positions are incomparable, so also reject a winner that
-		// another candidate dominates. This is an invariant check that should never fire, not an expected path
-		if hasDominantPosition(position.Combined, winningPosition.Combined) {
+		// Keep the sort's maximum-at-index-zero guarantee as a defense-in-depth invariant.
+		if hasDominantReparentPosition(position, winningPosition) {
 			return nil, nil, vterrors.Errorf(vtrpc.Code_INTERNAL, "candidate sorting error: %s has a more advanced position than the chosen candidate %s", topoproto.TabletAliasString(validTablets[i].Alias), topoproto.TabletAliasString(winningPrimaryTablet.Alias))
 		}
+		for j := i + 1; j < len(tabletPositions); j++ {
+			if haveReciprocallyContainedPositions(position.Combined, tabletPositions[j].Combined) {
+				return nil, nil, vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "split brain detected between servers - %s and %s", topoproto.TabletAliasString(validTablets[i].Alias), topoproto.TabletAliasString(validTablets[j].Alias))
+			}
+		}
 	}
 
 	// If we were requested to elect a particular primary, verify it's a valid

go/vt/vtctl/reparentutil/emergency_reparenter_test.go

@@ -6627,6 +6627,21 @@ func TestEmergencyReparenter_findMostAdvanced(t *testing.T) {
 		Executed: replication.Position{GTIDSet: replication.Mysql56GTIDSet{}},
 	}
 
+	// MariaDB GTID containment ignores the origin server, so these two positions contain
+	// each other while holding a different write for sequence 10
+	positionMariadbServer1 := &RelayLogPositions{
+		Combined: replication.MustParsePosition(replication.MariadbFlavorID, "0-1-10"),
+		Executed: replication.MustParsePosition(replication.MariadbFlavorID, "0-1-10"),
+	}
+	positionMariadbServer2 := &RelayLogPositions{
+		Combined: replication.MustParsePosition(replication.MariadbFlavorID, "0-2-10"),
+		Executed: replication.MustParsePosition(replication.MariadbFlavorID, "0-2-10"),
+	}
+	positionMariadbServer1Seq11 := &RelayLogPositions{
+		Combined: replication.MustParsePosition(replication.MariadbFlavorID, "0-1-11"),
+		Executed: replication.MustParsePosition(replication.MariadbFlavorID, "0-1-11"),
+	}
+
 	tests := []struct {
 		name                 string
 		validCandidates      map[string]*RelayLogPositions
@@ -6901,6 +6916,72 @@ func TestEmergencyReparenter_findMostAdvanced(t *testing.T) {
 				},
 			},
 			err: "split brain detected between servers",
+		}, {
+			// reciprocally contained but unequal positions (MariaDB GTIDs with the same
+			// domain and sequence from different origin servers) are divergent histories
+			// that containment can't order, so ERS must fail closed instead of picking
+			// a side of the divergence by tiebreak
+			name: "split brain detection on reciprocal but unequal mariadb positions",
+			validCandidates: map[string]*RelayLogPositions{
+				"zone1-0000000100": positionMariadbServer1,
+				"zone1-0000000101": positionMariadbServer2,
+			},
+			tabletMap: map[string]*topo.TabletInfo{
+				"zone1-0000000100": {
+					Tablet: &topodatapb.Tablet{
+						Alias: &topodatapb.TabletAlias{
+							Cell: "zone1",
+							Uid:  100,
+						},
+					},
+				},
+				"zone1-0000000101": {
+					Tablet: &topodatapb.Tablet{
+						Alias: &topodatapb.TabletAlias{
+							Cell: "zone1",
+							Uid:  101,
+						},
+					},
+				},
+			},
+			err: "split brain detected between servers",
+		}, {
+			// the divergent pair can sit behind a candidate that dominates both of them,
+			// so reciprocal containment must be checked between every pair of candidates,
+			// not just against the winning position
+			name: "split brain detection on reciprocal mariadb positions behind the winner",
+			validCandidates: map[string]*RelayLogPositions{
+				"zone1-0000000100": positionMariadbServer1Seq11,
+				"zone1-0000000101": positionMariadbServer1,
+				"zone1-0000000102": positionMariadbServer2,
+			},
+			tabletMap: map[string]*topo.TabletInfo{
+				"zone1-0000000100": {
+					Tablet: &topodatapb.Tablet{
+						Alias: &topodatapb.TabletAlias{
+							Cell: "zone1",
+							Uid:  100,
+						},
+					},
+				},
+				"zone1-0000000101": {
+					Tablet: &topodatapb.Tablet{
+						Alias: &topodatapb.TabletAlias{
+							Cell: "zone1",
+							Uid:  101,
+						},
+					},
+				},
+				"zone1-0000000102": {
+					Tablet: &topodatapb.Tablet{
+						Alias: &topodatapb.TabletAlias{
+							Cell: "zone1",
+							Uid:  102,
+						},
+					},
+				},
+			},
+			err: "split brain detected between servers",
 		},
 	}
 

go/vt/vtctl/reparentutil/reparent_sorter.go

@@ -29,17 +29,26 @@ import (
 // reparentSorter sorts tablets by GTID positions and Promotion rules aimed at finding the best
 // candidate for intermediate promotion in emergency reparent shard, and the new primary in planned reparent shard
 type reparentSorter struct {
-	tablets          []*topodatapb.Tablet
-	positions        []*RelayLogPositions
-	innodbBufferPool []int
-	durability       policy.Durabler
+	tablets                []*topodatapb.Tablet
+	positions              []*RelayLogPositions
+	combinedDominatedCount []int
+	executedDominatedCount []int
+	innodbBufferPool       []int
+	durability             policy.Durabler
 }
 
 // newReparentSorter creates a new reparentSorter
 func newReparentSorter(tablets []*topodatapb.Tablet, positions []*RelayLogPositions, innodbBufferPool []int, durability policy.Durabler) *reparentSorter {
 	return &reparentSorter{
-		tablets:          tablets,
-		positions:        positions,
+		tablets:   tablets,
+		positions: positions,
+		combinedDominatedCount: dominatedCountsForSort(tablets, positions, func(moreAdvanced, lessAdvanced *RelayLogPositions) bool {
+			return hasDominantPosition(moreAdvanced.Combined, lessAdvanced.Combined)
+		}),
+		executedDominatedCount: dominatedCountsForSort(tablets, positions, func(moreAdvanced, lessAdvanced *RelayLogPositions) bool {
+			return moreAdvanced.Combined.Equal(lessAdvanced.Combined) &&
+				hasDominantPosition(moreAdvanced.Executed, lessAdvanced.Executed)
+		}),
 		durability:       durability,
 		innodbBufferPool: innodbBufferPool,
 	}
@@ -52,6 +61,8 @@ func (rs *reparentSorter) Len() int { return len(rs.tablets) }
 func (rs *reparentSorter) Swap(i, j int) {
 	rs.tablets[i], rs.tablets[j] = rs.tablets[j], rs.tablets[i]
 	rs.positions[i], rs.positions[j] = rs.positions[j], rs.positions[i]
+	rs.combinedDominatedCount[i], rs.combinedDominatedCount[j] = rs.combinedDominatedCount[j], rs.combinedDominatedCount[i]
+	rs.executedDominatedCount[i], rs.executedDominatedCount[j] = rs.executedDominatedCount[j], rs.executedDominatedCount[i]
 	if len(rs.innodbBufferPool) != 0 {
 		rs.innodbBufferPool[i], rs.innodbBufferPool[j] = rs.innodbBufferPool[j], rs.innodbBufferPool[i]
 	}
@@ -71,33 +82,14 @@ func (rs *reparentSorter) Less(i, j int) bool {
 		return true
 	}
 
-	jPositions := rs.positions[j]
-	iPositions := rs.positions[i]
-
-	// sort by dominance of the combined positions first. GTID positions are partially
-	// ordered, so a pair can also be incomparable (disjoint UUIDs); those fall through to
-	// the tiebreakers below to keep the sort deterministic. this can't make the sort a
-	// total order, so findMostAdvanced re-checks the winner after sorting.
-	if hasDominantPosition(iPositions.Combined, jPositions.Combined) {
-		return true
-	}
-	if hasDominantPosition(jPositions.Combined, iPositions.Combined) {
-		return false
+	if rs.combinedDominatedCount[i] != rs.combinedDominatedCount[j] {
+		return rs.combinedDominatedCount[i] < rs.combinedDominatedCount[j]
 	}
 
-	// if the combined positions are equal, sort by the executed GTID positions. this
-	// prefers tablets with less SQL delay, which would otherwise slow down the reparent.
-	if iPositions.Combined.Equal(jPositions.Combined) {
-		if hasDominantPosition(iPositions.Executed, jPositions.Executed) {
-			return true
-		}
-		if hasDominantPosition(jPositions.Executed, iPositions.Executed) {
-			return false
-		}
+	if rs.executedDominatedCount[i] != rs.executedDominatedCount[j] {
+		return rs.executedDominatedCount[i] < rs.executedDominatedCount[j]
 	}
 
-	// at this point, neither tablet is ahead of the other
-	// so we check their promotion rules
 	jPromotionRule := policy.PromotionRule(rs.durability, rs.tablets[j])
 	iPromotionRule := policy.PromotionRule(rs.durability, rs.tablets[i])
 
@@ -123,6 +115,52 @@ func (rs *reparentSorter) Less(i, j int) bool {
 	return rs.tablets[i].Alias.Uid < rs.tablets[j].Alias.Uid
 }
 
+// dominatedCountsForSort returns, for each candidate, how many other candidates
+// strictly dominate it under the dominates predicate. The result is only meaningful
+// as a sort key: a lower count means more advanced, and the maximal candidates that
+// nothing dominates all have count 0.
+//
+// This count is what lets Less sort safely. dominates is only a partial order, so
+// comparing two candidates head-to-head is not transitive — with an incomparable
+// third candidate in play, sort.Sort can otherwise seat a dominated candidate above
+// the one that dominates it. Counting dominators sidesteps that, because dominates
+// itself is transitive: whatever dominates a candidate also dominates everything below
+// it, so a dominated candidate always has a strictly higher count than its dominator.
+// Ordering by ascending count therefore never places a candidate ahead of one that
+// dominates it.
+//
+// Counts are not unique. Incomparable candidates (e.g. divergent GTID histories) can
+// share a count; the caller breaks those ties with its remaining preferences.
+func dominatedCountsForSort(tablets []*topodatapb.Tablet, positions []*RelayLogPositions, dominates func(*RelayLogPositions, *RelayLogPositions) bool) []int {
+	dominatedCounts := make([]int, len(positions))
+	for i := range positions {
+		if tablets[i] == nil {
+			continue
+		}
+		for j := range positions {
+			if i == j || tablets[j] == nil {
+				continue
+			}
+			if dominates(positions[j], positions[i]) {
+				dominatedCounts[i]++ // one more candidate strictly dominates i
+			}
+		}
+	}
+	return dominatedCounts
+}
+
+// hasDominantReparentPosition reports whether moreAdvanced is strictly ahead of
+// lessAdvanced under the same two-level order the sorter uses: a strictly greater
+// received (Combined) history, or an equal received history with strictly more of it
+// applied (Executed). findMostAdvanced uses it as a defense-in-depth check that the
+// sort really did place the maximum at index 0 — it should never find a candidate that
+// dominates the chosen winner.
+func hasDominantReparentPosition(moreAdvanced, lessAdvanced *RelayLogPositions) bool {
+	return hasDominantPosition(moreAdvanced.Combined, lessAdvanced.Combined) ||
+		(moreAdvanced.Combined.Equal(lessAdvanced.Combined) &&
+			hasDominantPosition(moreAdvanced.Executed, lessAdvanced.Executed))
+}
+
 // sortTabletsForReparent sorts the tablets, given their positions for emergency reparent shard and planned reparent shard.
 // Tablets are sorted first by their replication positions, with ties broken by the promotion rules.
 func sortTabletsForReparent(tablets []*topodatapb.Tablet, positions []*RelayLogPositions, innodbBufferPool []int, durability policy.Durabler) error {