Merge #40415

40415: storage: fix two botched migrations r=nvanbenschoten a=tbg

See the discussion in:
#39460 (comment).

I'm running another 10 instances of tpcc/mixed-headroom now but am confident
that they will pass.

Closes #39460.

Release note (bug fix): Two issues that could lead to corruption and crashes
while upgrading a cluster from 19.1 into a 19.2 alpha/beta were fixed. The
first issue would manifest itself via a fatal error

> on-disk and in-memory state diverged: [Lease.Replica.Type: &roachpb.ReplicaType(0) != nil]

while the second one would leave ranges in a permanently unresponsive state and
lots of log messages of the form "unable to look up replica".

These errors are not easy to recover from, so the affected systems should be
recreated from a backup.

Co-authored-by: Tobias Schottdorf <[email protected]>

Author: craig[bot]

pkg/roachpb/metadata.go

@@ -145,7 +145,12 @@ func (r *RangeDescriptor) SetReplicaType(
 		desc := &r.InternalReplicas[i]
 		if desc.StoreID == storeID && desc.NodeID == nodeID {
 			prevTyp := desc.GetType()
-			desc.Type = &typ
+			if typ != VOTER_FULL {
+				desc.Type = &typ
+			} else {
+				// For 19.1 compatibility.
+				desc.Type = nil
+			}
 			return *desc, prevTyp, true
 		}
 	}
@@ -157,11 +162,16 @@ func (r *RangeDescriptor) SetReplicaType(
 func (r *RangeDescriptor) AddReplica(
 	nodeID NodeID, storeID StoreID, typ ReplicaType,
 ) ReplicaDescriptor {
+	var typPtr *ReplicaType
+	// For 19.1 compatibility, use nil instead of VOTER_FULL.
+	if typ != VOTER_FULL {
+		typPtr = &typ
+	}
 	toAdd := ReplicaDescriptor{
 		NodeID:    nodeID,
 		StoreID:   storeID,
 		ReplicaID: r.NextReplicaID,
-		Type:      &typ,
+		Type:      typPtr,
 	}
 	rs := r.Replicas()
 	rs.AddReplica(toAdd)

pkg/storage/batcheval/cmd_end_transaction.go

@@ -1109,15 +1109,16 @@ func changeReplicasTrigger(
 
 	var desc roachpb.RangeDescriptor
 	if change.Desc != nil {
+		// Trigger proposed by a 19.2+ node (and we're a 19.2+ node as well).
 		desc = *change.Desc
 	} else {
+		// Trigger proposed by a 19.1 node. Reconstruct descriptor from deprecated
+		// fields.
 		desc = *rec.Desc()
 		desc.SetReplicas(roachpb.MakeReplicaDescriptors(change.DeprecatedUpdatedReplicas))
 		desc.NextReplicaID = change.DeprecatedNextReplicaID
 	}
 
-	// TODO(tschottdorf): duplication of Desc with the trigger below, should
-	// likely remove it from the trigger.
 	pd.Replicated.State = &storagepb.ReplicaState{
 		Desc: &desc,
 	}

pkg/storage/replica_command.go

@@ -1266,9 +1266,12 @@ func (r *Replica) addReplicaLegacyPreemptiveSnapshot(
 	// complete. See #10409.
 	{
 		preemptiveRepDesc := roachpb.ReplicaDescriptor{
-			NodeID:    target.NodeID,
-			StoreID:   target.StoreID,
-			Type:      roachpb.ReplicaTypeVoterFull(),
+			NodeID:  target.NodeID,
+			StoreID: target.StoreID,
+			// NB: if we're still sending preemptive snapshot, the recipient is
+			// very likely a 19.1 node and does not understand this field. It
+			// won't matter to set it here, but don't anyway.
+			Type:      nil,
 			ReplicaID: 0, // intentional
 		}
 		if err := r.sendSnapshot(ctx, preemptiveRepDesc, SnapshotRequest_PREEMPTIVE, priority); err != nil {
@@ -1403,9 +1406,10 @@ func execChangeReplicasTxn(
 			deprecatedRepDesc = removed[0]
 		}
 		crt = &roachpb.ChangeReplicasTrigger{
-			DeprecatedChangeType: deprecatedChangeType,
-			DeprecatedReplica:    deprecatedRepDesc,
-			Desc:                 &updatedDesc,
+			DeprecatedChangeType:      deprecatedChangeType,
+			DeprecatedReplica:         deprecatedRepDesc,
+			DeprecatedUpdatedReplicas: updatedDesc.Replicas().All(),
+			DeprecatedNextReplicaID:   updatedDesc.NextReplicaID,
 		}
 	} else {
 		crt = &roachpb.ChangeReplicasTrigger{