Merge cockroachdb#106396

106396: server: de-flake a decommission test race condition r=abarganier a=knz

Informs cockroachdb#103698 (will fix when backported to 23.1).
Epic: CRDB-28893

When a Decommission request is sent that addresses multiple nodes simultaneously, a race condition existed in the code that logs the decommission event to the event log.

This is because the `sql.InsertEventRecords` API expects to take ownership over the events. The `Decommission` handler was violating the expectation by passing the same event references to multiple subsequent calls.

This was not visible in practice however, because the racy writes were always overwriting the same value to the same field.

This patch fixes it by allocating a new event for each subsequent node decommission.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>

Author: craig[bot]

pkg/server/decommission.go

@@ -347,25 +347,26 @@ func (s *Server) Decommission(
 		nodeIDs = orderedNodeIDs
 	}
 
-	var event logpb.EventPayload
-	var nodeDetails *eventpb.CommonNodeDecommissionDetails
-	if targetStatus.Decommissioning() {
-		ev := &eventpb.NodeDecommissioning{}
-		nodeDetails = &ev.CommonNodeDecommissionDetails
-		event = ev
-	} else if targetStatus.Decommissioned() {
-		ev := &eventpb.NodeDecommissioned{}
-		nodeDetails = &ev.CommonNodeDecommissionDetails
-		event = ev
-	} else if targetStatus.Active() {
-		ev := &eventpb.NodeRecommissioned{}
-		nodeDetails = &ev.CommonNodeDecommissionDetails
-		event = ev
-	} else {
-		panic("unexpected target membership status")
+	newEvent := func() (event logpb.EventPayload, nodeDetails *eventpb.CommonNodeDecommissionDetails) {
+		if targetStatus.Decommissioning() {
+			ev := &eventpb.NodeDecommissioning{}
+			nodeDetails = &ev.CommonNodeDecommissionDetails
+			event = ev
+		} else if targetStatus.Decommissioned() {
+			ev := &eventpb.NodeDecommissioned{}
+			nodeDetails = &ev.CommonNodeDecommissionDetails
+			event = ev
+		} else if targetStatus.Active() {
+			ev := &eventpb.NodeRecommissioned{}
+			nodeDetails = &ev.CommonNodeDecommissionDetails
+			event = ev
+		} else {
+			panic(errors.AssertionFailedf("unexpected target membership status: %v", targetStatus))
+		}
+		event.CommonDetails().Timestamp = timeutil.Now().UnixNano()
+		nodeDetails.RequestingNodeID = int32(s.NodeID())
+		return event, nodeDetails
 	}
-	event.CommonDetails().Timestamp = timeutil.Now().UnixNano()
-	nodeDetails.RequestingNodeID = int32(s.NodeID())
 
 	for _, nodeID := range nodeIDs {
 		statusChanged, err := s.nodeLiveness.SetMembershipStatus(ctx, nodeID, targetStatus)
@@ -377,6 +378,7 @@ func (s *Server) Decommission(
 			return grpcstatus.Errorf(codes.Internal, err.Error())
 		}
 		if statusChanged {
+			event, nodeDetails := newEvent()
 			nodeDetails.TargetNodeID = int32(nodeID)
 			// Ensure an entry is produced in the external log in all cases.
 			log.StructuredEvent(ctx, event)

pkg/sql/event_log.go

@@ -481,6 +481,9 @@ const (
 // of the provided transaction, using the provided internal executor.
 //
 // This converts to a call to insertEventRecords() with just 1 entry.
+//
+// Note: it is not safe to pass the same entry references to multiple
+// subsequent calls (it causes a race condition).
 func InsertEventRecords(
 	ctx context.Context, execCfg *ExecutorConfig, dst LogEventDestination, info ...logpb.EventPayload,
 ) {
@@ -516,6 +519,9 @@ func InsertEventRecords(
 //   - if there's at txn, after the txn commit time (i.e. we don't log
 //     if the txn ends up aborting), using a txn commit trigger.
 //   - otherwise (no txn), immediately.
+//
+// Note: it is not safe to pass the same entry references to multiple
+// subsequent calls (it causes a race condition).
 func insertEventRecords(
 	ctx context.Context,
 	execCfg *ExecutorConfig,