Merge #156572 #156639

156572: server: fix admin v1 health endpoint r=kyle-a-wong a=kyle-a-wong

PR #153929 migrated admin RPC use DRPC http instead of the grpc-gateway. This introduced a bug that made the
admin/v1/health endpoint require authentication when it previously did not require this.

This PR changes it to use an unauthenticated internal server.

Fixes: #155052
Epic: None
Release note: None

156639: kvstorage: separate engines in replica destruction r=arulajmani a=pav-kv

This PR decomposes clearing the unreplicated RangeID-local span in replica destruction funcs, and annotates them with the raft/state engine types.

Part of #152845
Epic: CRDB-55220

Co-authored-by: Kyle Wong <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>

Author: 3 people

pkg/kv/kvserver/client_merge_test.go

@@ -3963,25 +3963,27 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 				// doesn't yet exist in the engine, so we set it manually.
 				//
 				// The deletion also extends to the RangeTombstoneKey.
+				sl := kvstorage.MakeStateLoader(rangeID)
 				require.NoError(t, sst.ClearRawRange(
 					keys.RangeGCThresholdKey(rangeID),
-					keys.RangeTombstoneKey(rangeID),
+					sl.RangeTombstoneKey(),
 					true, false,
 				))
-				require.NoError(t, kvstorage.MakeStateLoader(rangeID).SetRangeTombstone(
+				require.NoError(t, sl.SetRangeTombstone(
 					context.Background(), &sst,
 					kvserverpb.RangeTombstone{NextReplicaID: math.MaxInt32},
 				))
-				{
-					// Ditto for the unreplicated version, where the first key happens to
-					// be the HardState.
-					sl := rditer.Select(rangeID, rditer.SelectOpts{
-						UnreplicatedByRangeID: true,
-					})
-					require.Len(t, sl, 1)
-					s := sl[0]
-					require.NoError(t, sst.ClearRawRange(keys.RaftHardStateKey(rangeID), s.EndKey, true, false))
-				}
+				// Ditto for the unreplicated RangeID keys. Note that it is also split
+				// into two range clears, to work around the RaftReplicaID key.
+				require.NoError(t, sst.ClearRawRange(
+					keys.RaftHardStateKey(rangeID), sl.RaftReplicaIDKey(), true, false,
+				))
+				require.NoError(t, sl.ClearRaftReplicaID(&sst))
+				require.NoError(t, sst.ClearRawRange(
+					sl.RaftTruncatedStateKey(),
+					keys.MakeRangeIDUnreplicatedPrefix(rangeID).PrefixEnd(),
+					true, false,
+				))
 
 				require.NoError(t, sst.Finish())
 				expectedSSTs = append(expectedSSTs, sstFile.Data())

pkg/kv/kvserver/kvstorage/destroy.go

@@ -82,35 +82,27 @@ type DestroyReplicaInfo struct {
 // This call issues all writes ordered by key. This is to support a large
 // variety of uses, including SSTable generation for snapshot application.
 func DestroyReplica(
-	ctx context.Context,
-	reader storage.Reader,
-	writer storage.Writer,
-	info DestroyReplicaInfo,
-	next roachpb.ReplicaID,
+	ctx context.Context, rw ReadWriter, info DestroyReplicaInfo, next roachpb.ReplicaID,
 ) error {
-	return destroyReplicaImpl(ctx, reader, writer, info, next)
+	return destroyReplicaImpl(ctx, rw, info, next)
 }
 
 func destroyReplicaImpl(
-	ctx context.Context,
-	reader storage.Reader,
-	writer storage.Writer,
-	info DestroyReplicaInfo,
-	next roachpb.ReplicaID,
+	ctx context.Context, rw ReadWriter, info DestroyReplicaInfo, next roachpb.ReplicaID,
 ) error {
 	if next <= info.ReplicaID {
 		return errors.AssertionFailedf("%v must not survive its own tombstone", info.FullReplicaID)
 	}
 	sl := MakeStateLoader(info.RangeID)
 	// Assert that the ReplicaID in storage matches the one being removed.
-	if loaded, err := sl.LoadRaftReplicaID(ctx, reader); err != nil {
+	if loaded, err := sl.LoadRaftReplicaID(ctx, rw.State.RO); err != nil {
 		return err
 	} else if id := loaded.ReplicaID; id != info.ReplicaID {
 		return errors.AssertionFailedf("%v has a mismatching ID %d", info.FullReplicaID, id)
 	}
 	// Assert that the provided tombstone moves the existing one strictly forward.
 	// A failure would indicate that something is wrong in the replica lifecycle.
-	if ts, err := sl.LoadRangeTombstone(ctx, reader); err != nil {
+	if ts, err := sl.LoadRangeTombstone(ctx, rw.State.RO); err != nil {
 		return err
 	} else if next <= ts.NextReplicaID {
 		return errors.AssertionFailedf("%v cannot rewind tombstone from %d to %d",
@@ -136,40 +128,64 @@ func destroyReplicaImpl(
 	// First, clear all RangeID-local replicated keys. Also, include all
 	// RangeID-local unreplicated keys < RangeTombstoneKey as a drive-by, since we
 	// decided that these (currently none) belong to the state machine engine.
-	tombstoneKey := keys.RangeTombstoneKey(info.RangeID)
+	span := roachpb.Span{
+		Key:    keys.MakeRangeIDReplicatedPrefix(info.RangeID),
+		EndKey: keys.RangeTombstoneKey(info.RangeID),
+	}
 	if err := storage.ClearRangeWithHeuristic(
-		ctx, reader, writer,
-		keys.MakeRangeIDReplicatedPrefix(info.RangeID),
-		tombstoneKey,
-		ClearRangeThresholdPointKeys(),
+		ctx, rw.State.RO, rw.State.WO,
+		span.Key, span.EndKey, ClearRangeThresholdPointKeys(),
 	); err != nil {
 		return err
 	}
 	// Save a tombstone to ensure that replica IDs never get reused.
-	if err := sl.SetRangeTombstone(ctx, writer, kvserverpb.RangeTombstone{
+	if err := sl.SetRangeTombstone(ctx, rw.State.WO, kvserverpb.RangeTombstone{
 		NextReplicaID: next, // NB: NextReplicaID > 0
 	}); err != nil {
 		return err
 	}
-	// Clear the rest of the RangeID-local unreplicated keys.
-	// TODO(pav-kv): decompose this further to delete raft and state machine keys
-	// separately. Currently, all the remaining keys in this span belong to the
-	// raft engine except the RaftReplicaID.
+
+	// Clear the rest of the RangeID-local unreplicated keys. These are all raft
+	// engine keys, except for RaftReplicaIDKey. Make a stop at the latter, and
+	// clear it manually (note that it always exists for an existing replica, and
+	// we have asserted that above).
+	//
+	// TODO(pav-kv): make a helper for piece-wise clearing, instead of using a
+	// series of ClearRangeWithHeuristic.
+	span = roachpb.Span{
+		Key:    span.EndKey.Next(), // RangeTombstoneKey.Next()
+		EndKey: keys.RaftReplicaIDKey(info.RangeID),
+	}
+	// TODO(#152845): with separated raft storage, clear only the unapplied suffix
+	// of the raft log, which is in this span.
+	if err := storage.ClearRangeWithHeuristic(
+		ctx, rw.Raft.RO, rw.Raft.WO,
+		span.Key, span.EndKey, ClearRangeThresholdPointKeys(),
+	); err != nil {
+		return err
+	}
+	if err := sl.ClearRaftReplicaID(rw.State.WO); err != nil {
+		return err
+	}
+	span = roachpb.Span{
+		Key:    span.EndKey.Next(), // RaftReplicaIDKey.Next()
+		EndKey: keys.MakeRangeIDUnreplicatedPrefix(info.RangeID).PrefixEnd(),
+	}
 	if err := storage.ClearRangeWithHeuristic(
-		ctx, reader, writer,
-		tombstoneKey.Next(),
-		keys.MakeRangeIDUnreplicatedPrefix(info.RangeID).PrefixEnd(),
-		ClearRangeThresholdPointKeys(),
+		ctx, rw.Raft.RO, rw.Raft.WO,
+		span.Key, span.EndKey, ClearRangeThresholdPointKeys(),
 	); err != nil {
 		return err
 	}
+
 	// Finally, clear all the user keys (MVCC keyspace and the corresponding
 	// system and lock table keys), if info.Keys is not empty.
 	for _, span := range rditer.Select(info.RangeID, rditer.SelectOpts{
 		Ranged: rditer.SelectAllRanged(info.Keys),
 	}) {
 		if err := storage.ClearRangeWithHeuristic(
-			ctx, reader, writer, span.Key, span.EndKey, ClearRangeThresholdPointKeys(),
+			ctx, rw.State.RO, rw.State.WO,
+			span.Key, span.EndKey, ClearRangeThresholdPointKeys(),
 		); err != nil {
 			return err
 		}
@@ -185,14 +201,14 @@ func destroyReplicaImpl(
 // function clears.
 // TODO(pav-kv): get rid of SelectOpts.
 func SubsumeReplica(
-	ctx context.Context, reader storage.Reader, writer storage.Writer, info DestroyReplicaInfo,
+	ctx context.Context, rw ReadWriter, info DestroyReplicaInfo,
 ) (rditer.SelectOpts, error) {
 	// Forget about the user keys.
 	info.Keys = roachpb.RSpan{}
 	return rditer.SelectOpts{
 		ReplicatedByRangeID:   true,
 		UnreplicatedByRangeID: true,
-	}, destroyReplicaImpl(ctx, reader, writer, info, MergedTombstoneReplicaID)
+	}, destroyReplicaImpl(ctx, rw, info, MergedTombstoneReplicaID)
 }
 
 // RemoveStaleRHSFromSplit removes all replicated data for the RHS replica of a

pkg/kv/kvserver/kvstorage/destroy_test.go

@@ -68,7 +68,8 @@ func TestDestroyReplica(t *testing.T) {
 	})
 	mutate("destroy", func(rw storage.ReadWriter) {
 		require.NoError(t, DestroyReplica(
-			ctx, rw, rw, DestroyReplicaInfo{FullReplicaID: r.id, Keys: r.keys}, r.id.ReplicaID+1,
+			ctx, TODOReadWriter(rw),
+			DestroyReplicaInfo{FullReplicaID: r.id, Keys: r.keys}, r.id.ReplicaID+1,
 		))
 	})
 

pkg/kv/kvserver/kvstorage/stateloader.go

@@ -352,6 +352,11 @@ func (s StateLoader) SetRaftReplicaID(
 	)
 }
 
+// ClearRaftReplicaID clears the RaftReplicaID key.
+func (s StateLoader) ClearRaftReplicaID(stateWO StateWO) error {
+	return stateWO.ClearUnversioned(s.RaftReplicaIDKey(), storage.ClearOptions{})
+}
+
 // LoadRangeTombstone loads the RangeTombstone of the range.
 func (s StateLoader) LoadRangeTombstone(
 	ctx context.Context, stateRO StateRO,

pkg/kv/kvserver/kvstorage/testdata/TestDestroyReplica.txt

@@ -26,7 +26,7 @@ Delete (Sized at 43): 0,0 /Local/RangeID/123/u/RaftLog/logIndex:12 (0x0169f67b75
 Delete (Sized at 43): 0,0 /Local/RangeID/123/u/RaftLog/logIndex:13 (0x0169f67b757266746c000000000000000d00): 
 Delete (Sized at 43): 0,0 /Local/RangeID/123/u/RaftLog/logIndex:14 (0x0169f67b757266746c000000000000000e00): 
 Delete (Sized at 43): 0,0 /Local/RangeID/123/u/RaftLog/logIndex:15 (0x0169f67b757266746c000000000000000f00): 
-Delete (Sized at 31): 0,0 /Local/RangeID/123/u/RaftReplicaID (0x0169f67b757266747200): 
+Delete: 0,0 /Local/RangeID/123/u/RaftReplicaID (0x0169f67b757266747200): 
 Delete (Sized at 33): 0,0 /Local/RangeID/123/u/RaftTruncatedState (0x0169f67b757266747400): 
 Delete (Sized at 34): 0,0 /Local/RangeID/123/u/RangeLastReplicaGCTimestamp (0x0169f67b75726c727400): 
 Delete (Sized at 20): 0.000000001,0 /Local/Range"a"/RangeDescriptor (0x016b126100017264736300000000000000000109): 

pkg/kv/kvserver/replica_app_batch.go

@@ -364,7 +364,7 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		rhsRepl.readOnlyCmdMu.Unlock()
 
 		if _, err := kvstorage.SubsumeReplica(
-			ctx, b.batch, b.batch, rhsRepl.destroyInfoRaftMuLocked(),
+			ctx, kvstorage.TODOReadWriter(b.batch), rhsRepl.destroyInfoRaftMuLocked(),
 		); err != nil {
 			return errors.Wrapf(err, "unable to subsume replica before merge")
 		}
@@ -453,7 +453,8 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		// above, and DestroyReplica will also add a range tombstone to the
 		// batch, so that when we commit it, the removal is finalized.
 		if err := kvstorage.DestroyReplica(
-			ctx, b.batch, b.batch, b.r.destroyInfoRaftMuLocked(), change.NextReplicaID(),
+			ctx, kvstorage.TODOReadWriter(b.batch),
+			b.r.destroyInfoRaftMuLocked(), change.NextReplicaID(),
 		); err != nil {
 			return errors.Wrapf(err, "unable to destroy replica before removal")
 		}

pkg/kv/kvserver/replica_destroy.go

@@ -91,7 +91,8 @@ func (r *Replica) destroyRaftMuLocked(ctx context.Context, nextReplicaID roachpb
 
 	// TODO(sep-raft-log): need both engines separately here.
 	if err := kvstorage.DestroyReplica(
-		ctx, r.store.TODOEngine(), batch, r.destroyInfoRaftMuLocked(), nextReplicaID,
+		ctx, kvstorage.TODOReaderWriter(r.store.TODOEngine(), batch),
+		r.destroyInfoRaftMuLocked(), nextReplicaID,
 	); err != nil {
 		return err
 	}

pkg/kv/kvserver/snapshot_apply_prepare.go

@@ -149,7 +149,9 @@ func (s *snapWriteBuilder) clearSubsumedReplicaDiskData(ctx context.Context) err
 	for _, sub := range s.subsume {
 		// We have to create an SST for the subsumed replica's range-id local keys.
 		if err := s.writeSST(ctx, func(ctx context.Context, w storage.Writer) error {
-			opts, err := kvstorage.SubsumeReplica(ctx, reader, w, sub)
+			opts, err := kvstorage.SubsumeReplica(
+				ctx, kvstorage.TODOReaderWriter(reader, w), sub,
+			)
 			s.cleared = append(s.cleared, rditer.Select(sub.RangeID, opts)...)
 			return err
 		}); err != nil {

pkg/kv/kvserver/testdata/TestPrepareSnapApply.txt

@@ -7,10 +7,10 @@ Put: 0,0 /Local/RangeID/123/u/RaftReplicaID (0x0169f67b757266747200): replica_id
 Put: 0,0 /Local/RangeID/123/u/RaftTruncatedState (0x0169f67b757266747400): index:100 term:20 
 >> sst:
 Put: 0,0 /Local/RangeID/101/u/RangeTombstone (0x0169ed757266746200): next_replica_id:2147483647 
-Delete (Sized at 30): 0,0 /Local/RangeID/101/u/RaftReplicaID (0x0169ed757266747200): 
+Delete: 0,0 /Local/RangeID/101/u/RaftReplicaID (0x0169ed757266747200): 
 >> sst:
 Put: 0,0 /Local/RangeID/102/u/RangeTombstone (0x0169ee757266746200): next_replica_id:2147483647 
-Delete (Sized at 30): 0,0 /Local/RangeID/102/u/RaftReplicaID (0x0169ee757266747200): 
+Delete: 0,0 /Local/RangeID/102/u/RaftReplicaID (0x0169ee757266747200): 
 >> sst:
 Delete (Sized at 27): /Local/Lock"y\xff" 0300000000000000000000000000000000 (0x017a6b1279ff000100030000000000000000000000000000000012): 
 >> sst:

pkg/server/server_http.go

@@ -237,7 +237,7 @@ func (s *httpServer) setupRoutes(
 
 	// Exempt the 2nd health check endpoint from authentication.
 	// (This simply mirrors /health and exists for backward compatibility.)
-	s.mux.Handle(apiconstants.AdminHealth, authenticatedAPIInternalServer)
+	s.mux.Handle(apiconstants.AdminHealth, unauthenticatedAPIInternalServer)
 	// The /_status/vars and /metrics endpoint is not authenticated either. Useful for monitoring.
 	s.mux.Handle(apiconstants.StatusVars, http.HandlerFunc(varsHandler{metricSource, s.cfg.Settings, false /* useStaticLabels */}.handleVars))
 	s.mux.Handle(apiconstants.MetricsPath, http.HandlerFunc(varsHandler{metricSource, s.cfg.Settings, true /* useStaticLabels */}.handleVars))