kvserver: make AdminVerifyProtectedTimestamp a no-op

As part of re-working the PTS subsystem to work with secondary tenants
we are dropping verification of protection records. None of the
existing users of `Verification` (currently only backup) would fail
non-destructively without it; this allows us to circumvent the
complexity involved in making `Verification` work in the new subsystem.

This patch removes code that was previously used to serve the
`AdminVerifyProtectedTimestamp` request; we instead vacuously
return true. This is to account for the {21.2, 22.1} mixed version case
where 22.1 binary nodes still need to be able to serve this request.
This can happen if the request is initiated on a 21.2 node and the
leaseholder of the range it is trying to verify is on a 22.1 node. By
always returning true we ensure the (backup) job upstream doesn't fail.
This is okay even if the PTS record being verified does not apply as
the failure mode is non-destructive (which is also why we're okay
removing the call to verification in 22.1).

This patch is required in preperation for the `ProtectedTSReader`.

Release note: None

Release justification: low risk, high benefit changes to existing
functionality.

pkg/kv/kvserver/BUILD.bazel

@@ -339,7 +339,6 @@ go_test(
         "//pkg/kv/kvserver/protectedts",
         "//pkg/kv/kvserver/protectedts/ptpb",
         "//pkg/kv/kvserver/protectedts/ptstorage",
-        "//pkg/kv/kvserver/protectedts/ptverifier",
         "//pkg/kv/kvserver/raftentry",
         "//pkg/kv/kvserver/rditer",
         "//pkg/kv/kvserver/readsummary/rspb",

pkg/kv/kvserver/client_protectedts_test.go

@@ -18,9 +18,9 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts/ptpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts/ptstorage"
-	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts/ptverifier"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -210,12 +210,13 @@ func TestProtectedTimestamps(t *testing.T) {
 	thresh := thresholdFromTrace(trace)
 	require.Truef(t, thresh.Less(ptsRec.Timestamp), "threshold: %v, protected %v %q", thresh, ptsRec.Timestamp, trace)
 
-	// Verify that the record indeed did apply as far as the replica is concerned.
-	ptv := ptverifier.New(s0.DB(), pts)
-	require.NoError(t, ptv.Verify(ctx, ptsRec.ID.GetUUID()))
-	ptsRecVerified, err := ptsWithDB.GetRecord(ctx, nil /* txn */, ptsRec.ID.GetUUID())
-	require.NoError(t, err)
-	require.True(t, ptsRecVerified.Verified)
+	// Verify that the record did indeed make its way down into KV where the
+	// replica can read it from.
+	ptp := tc.Server(0).ExecutorConfig().(sql.ExecutorConfig).ProtectedTimestampProvider
+	require.NoError(
+		t,
+		verifyProtectionTimestampExistsOnSpans(ctx, tc, ptp, ptsRec.Timestamp, ptsRec.DeprecatedSpans),
+	)
 
 	// Make a new record that is doomed to fail.
 	failedRec := ptsRec
@@ -226,17 +227,23 @@ func TestProtectedTimestamps(t *testing.T) {
 	_, err = ptsWithDB.GetRecord(ctx, nil /* txn */, failedRec.ID.GetUUID())
 	require.NoError(t, err)
 
-	// Verify that it indeed did fail.
-	verifyErr := ptv.Verify(ctx, failedRec.ID.GetUUID())
-	require.Regexp(t, "failed to verify protection", verifyErr)
+	// replica can read it from. We then verify (below) that the failed record
+	// does not affect the ability to GC.
+	require.NoError(
+		t,
+		verifyProtectionTimestampExistsOnSpans(ctx, tc, ptp, failedRec.Timestamp, failedRec.DeprecatedSpans),
+	)
 
 	// Add a new record that is after the old record.
 	laterRec := ptsRec
 	laterRec.ID = uuid.MakeV4().GetBytes()
 	laterRec.Timestamp = afterWrites
 	laterRec.Timestamp.Logical = 0
 	require.NoError(t, ptsWithDB.Protect(ctx, nil /* txn */, &laterRec))
-	require.NoError(t, ptv.Verify(ctx, laterRec.ID.GetUUID()))
+	require.NoError(
+		t,
+		verifyProtectionTimestampExistsOnSpans(ctx, tc, ptp, laterRec.Timestamp, laterRec.DeprecatedSpans),
+	)
 
 	// Release the record that had succeeded and ensure that GC eventually
 	// happens up to the protected timestamp of the new record.
@@ -263,3 +270,29 @@ func TestProtectedTimestamps(t *testing.T) {
 	require.Len(t, state.Records, 0)
 	require.Equal(t, int(state.NumRecords), len(state.Records))
 }
+
+func verifyProtectionTimestampExistsOnSpans(
+	ctx context.Context,
+	tc *testcluster.TestCluster,
+	provider protectedts.Provider,
+	pts hlc.Timestamp,
+	spans roachpb.Spans,
+) error {
+	if err := provider.Refresh(ctx, tc.Server(0).Clock().Now()); err != nil {
+		return err
+	}
+	for _, sp := range spans {
+		timestamps, _ := provider.GetProtectionTimestamps(ctx, sp)
+		found := false
+		for _, ts := range timestamps {
+			if ts.Equal(pts) {
+				found = true
+				break
+			}
+		}
+		if !found {
+			return errors.Newf("protection timestamp %s does not exist on span %s", pts, sp)
+		}
+	}
+	return nil
+}

pkg/kv/kvserver/client_replica_test.go

@@ -3579,6 +3579,17 @@ func TestStrictGCEnforcement(t *testing.T) {
 				r.ReadProtectedTimestamps(ctx)
 			}
 		}
+		refreshCacheAndUpdatePTSState = func(t *testing.T, nodeID roachpb.NodeID) {
+			for i := 0; i < tc.NumServers(); i++ {
+				if tc.Server(i).NodeID() != nodeID {
+					continue
+				}
+				ptp := tc.Server(i).ExecutorConfig().(sql.ExecutorConfig).ProtectedTimestampProvider
+				require.NoError(t, ptp.Refresh(ctx, tc.Server(i).Clock().Now()))
+				_, r := getFirstStoreReplica(t, tc.Server(i), tableKey)
+				r.ReadProtectedTimestamps(ctx)
+			}
+		}
 	)
 
 	{
@@ -3632,13 +3643,15 @@ func TestStrictGCEnforcement(t *testing.T) {
 		}))
 		assertScanRejected(t)
 
-		require.NoError(t, ptp.Verify(ctx, rec.ID.GetUUID()))
+		desc, err := tc.LookupRange(tableKey)
+		require.NoError(t, err)
+		target, err := tc.FindRangeLeaseHolder(desc, nil)
+		require.NoError(t, err)
+		refreshCacheAndUpdatePTSState(t, target.NodeID)
 		assertScanOk(t)
 
 		// Transfer the lease and demonstrate that the query succeeds because we're
 		// cautious in the face of lease transfers.
-		desc, err := tc.LookupRange(tableKey)
-		require.NoError(t, err)
 		require.NoError(t, tc.TransferRangeLease(desc, tc.Target(1)))
 		assertScanOk(t)
 	})

pkg/kv/kvserver/replica_command.go

@@ -3358,34 +3358,22 @@ func (r *Replica) adminScatter(
 	}, nil
 }
 
+// TODO(arul): AdminVerifyProtectedTimestampRequest can entirely go away in
+// 22.2.
 func (r *Replica) adminVerifyProtectedTimestamp(
-	ctx context.Context, args roachpb.AdminVerifyProtectedTimestampRequest,
+	ctx context.Context, _ roachpb.AdminVerifyProtectedTimestampRequest,
 ) (resp roachpb.AdminVerifyProtectedTimestampResponse, err error) {
-	var doesNotApplyReason string
-	resp.Verified, doesNotApplyReason, err = r.protectedTimestampRecordApplies(ctx, &args)
-	if err != nil {
-		return resp, err
-	}
-
-	// In certain cases we do not want to return an error even if we failed to
-	// verify the protected ts record. This ensures that executeAdminBatch adds
-	// the response to the batch, thereby allowing us to aggregate the
-	// verification failures across all AdminVerifyProtectedTimestampRequests and
-	// construct a more informative error to show to the user.
-	if doesNotApplyReason != "" {
-		if !resp.Verified {
-			desc := r.Desc()
-			failedRange := roachpb.AdminVerifyProtectedTimestampResponse_FailedRange{
-				RangeID:  int64(desc.GetRangeID()),
-				StartKey: desc.GetStartKey(),
-				EndKey:   desc.EndKey,
-				Reason:   doesNotApplyReason,
-			}
-			resp.VerificationFailedRanges = append(resp.VerificationFailedRanges, failedRange)
-			// TODO(adityamaru): This is here for compatibility with 20.2, remove in
-			// 21.2.
-			resp.DeprecatedFailedRanges = append(resp.DeprecatedFailedRanges, *r.Desc())
-		}
-	}
+	// AdminVerifyProtectedTimestampRequest is not supported starting from the
+	// 22.1 release. We expect nodes running a 22.1 binary to still service this
+	// request in a {21.2, 22.1} mixed version cluster. This can happen if the
+	// request is initiated on a 21.2 node and the leaseholder of the range it is
+	// trying to verify is on a 22.1 node.
+	//
+	// We simply return true without attempting to verify in such a case. This
+	// ensures upstream jobs (backups) don't fail as a result. It is okay to
+	// return true regardless even if the PTS record being verified does not apply
+	// as the failure mode is non-destructive. Infact, this is the reason we're
+	// no longer supporting Verification past 22.1.
+	resp.Verified = true
 	return resp, nil
 }

pkg/kv/kvserver/replica_protected_timestamp.go

@@ -12,11 +12,9 @@ package kvserver
 
 import (
 	"context"
-	"fmt"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/gc"
-	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts/ptpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -67,41 +65,6 @@ func (r *Replica) maybeUpdateCachedProtectedTS(ts *cachedProtectedTimestampState
 	}
 }
 
-// protectedTimestampRecordApplies returns true if it is this case that the
-// record which protects the `protected` timestamp. It returns false if it may
-// not. If the state of the cache  is not sufficiently new to determine whether
-// the record will apply, the cache is refreshed and then the check is performed
-// again. See r.protectedTimestampRecordCurrentlyApplies() for more details.
-func (r *Replica) protectedTimestampRecordApplies(
-	ctx context.Context, args *roachpb.AdminVerifyProtectedTimestampRequest,
-) (willApply bool, doesNotApplyReason string, _ error) {
-	// Check the state of the cache without a refresh.
-	willApply, cacheTooOld, doesNotApplyReason, err := r.protectedTimestampRecordCurrentlyApplies(
-		ctx, args)
-	if err != nil {
-		return false, doesNotApplyReason, err
-	}
-	if !cacheTooOld {
-		return willApply, doesNotApplyReason, nil
-	}
-	// Refresh the cache so that we know that the next time we come around we're
-	// certain to either see the record or see a timestamp for readAt that is
-	// greater than or equal to recordAliveAt.
-	if err := r.store.protectedtsCache.Refresh(ctx, args.RecordAliveAt); err != nil {
-		return false, doesNotApplyReason, err
-	}
-	willApply, cacheTooOld, doesNotApplyReason, err = r.protectedTimestampRecordCurrentlyApplies(
-		ctx, args)
-	if err != nil {
-		return false, doesNotApplyReason, err
-	}
-	if cacheTooOld {
-		return false, doesNotApplyReason, errors.AssertionFailedf(
-			"cache was not updated after being refreshed")
-	}
-	return willApply, doesNotApplyReason, nil
-}
-
 func (r *Replica) readProtectedTimestampsRLocked(
 	ctx context.Context, f func(r *ptpb.Record),
 ) (ts cachedProtectedTimestampState) {
@@ -131,123 +94,6 @@ func (r *Replica) readProtectedTimestampsRLocked(
 	return ts
 }
 
-// protectedTimestampRecordCurrentlyApplies determines whether a record with
-// the specified ID which protects `protected` and is known to exist at
-// `recordAliveAt` will apply given the current state of the cache. This method
-// is called by `r.protectedTimestampRecordApplies()`. It may be the case that
-// the current state of the cache is too old to determine whether the record
-// will apply. In such cases the cache should be refreshed to recordAliveAt and
-// then this method should be called again.
-// In certain cases we return a doesNotApplyReason explaining why the protected
-// ts record does not currently apply. We do not want to return an error so that
-// we can aggregate the reasons across multiple
-// AdminVerifyProtectedTimestampRequest, as explained in
-// adminVerifyProtectedTimestamp.
-func (r *Replica) protectedTimestampRecordCurrentlyApplies(
-	ctx context.Context, args *roachpb.AdminVerifyProtectedTimestampRequest,
-) (willApply, cacheTooOld bool, doesNotApplyReason string, _ error) {
-	// We first need to check that we're the current leaseholder.
-	// TODO(ajwerner): what other conditions with regards to time do we need to
-	// check? I don't think there are any. If the recordAliveAt is after our
-	// liveness expiration that's okay because we're either going to find the
-	// record or we're not and if we don't then we'll push the cache and re-assert
-	// that we're still the leaseholder. If somebody else becomes the leaseholder
-	// then they will have to go through the same process.
-	ls, pErr := r.redirectOnOrAcquireLease(ctx)
-	if pErr != nil {
-		return false, false, "", pErr.GoError()
-	}
-
-	// NB: It should be the case that the recordAliveAt timestamp
-	// is before the current time and that the above lease check means that
-	// the replica is the leaseholder at the current time. If recordAliveAt
-	// happened to be newer than the current time we'd need to make sure that
-	// the current Replica will be live at that time. Given that recordAliveAt
-	// has to be before the batch timestamp for this request and we should
-	// have forwarded the local clock to the batch timestamp this can't
-	// happen.
-	// TODO(ajwerner): do we need to assert that indeed the recordAliveAt precedes
-	// the batch timestamp? Probably not a bad sanity check.
-
-	// We may be reading the protected timestamp cache while we're holding
-	// the Replica.mu for reading. If we do so and find newer state in the cache
-	// then we want to, update the replica's cache of its state. The guarantee
-	// we provide is that if a record is successfully verified then the Replica's
-	// cachedProtectedTS will have a readAt value high enough to include that
-	// record.
-	var read cachedProtectedTimestampState
-	defer r.maybeUpdateCachedProtectedTS(&read)
-	r.mu.RLock()
-	defer r.mu.RUnlock()
-	defer read.clearIfNotNewer(r.mu.cachedProtectedTS)
-
-	// If the key that routed this request to this range is now out of this
-	// range's bounds, return an error for the client to try again on the
-	// correct range.
-	desc := r.descRLocked()
-	if !kvserverbase.ContainsKeyRange(desc, args.Key, args.EndKey) {
-		return false, false, "", roachpb.NewRangeKeyMismatchErrorWithCTPolicy(ctx, args.Key, args.EndKey, desc,
-			r.mu.state.Lease, r.closedTimestampPolicyRLocked())
-	}
-	if args.Protected.LessEq(*r.mu.state.GCThreshold) {
-		gcReason := fmt.Sprintf("protected ts: %s is less than equal to the GCThreshold: %s for the"+
-			" range %s - %s", args.Protected.String(), r.mu.state.GCThreshold.String(),
-			desc.StartKey.String(), desc.EndKey.String())
-		return false, false, gcReason, nil
-	}
-	if args.RecordAliveAt.Less(ls.Lease.Start.ToTimestamp()) {
-		return true, false, "", nil
-	}
-
-	// Now we're in the case where maybe it is possible that we're going to later
-	// attempt to set the GC threshold above our protected point so to prevent
-	// that we add some state to the replica.
-	r.protectedTimestampMu.Lock()
-	defer r.protectedTimestampMu.Unlock()
-	if args.Protected.Less(r.protectedTimestampMu.pendingGCThreshold) {
-		gcReason := fmt.Sprintf(
-			"protected ts: %s is less than the pending GCThreshold: %s for the range %s - %s",
-			args.Protected.String(), r.protectedTimestampMu.pendingGCThreshold.String(),
-			desc.StartKey.String(), desc.EndKey.String())
-		return false, false, gcReason, nil
-	}
-
-	var seen bool
-	read = r.readProtectedTimestampsRLocked(ctx, func(r *ptpb.Record) {
-		// Comparing record ID and the timestamp ensures that we find the record
-		// that we are verifying.
-		// A PTS record can be updated with a new Timestamp to protect, and so we
-		// need to ensure that we are not seeing the old version of the record in
-		// case the cache has not been updated.
-		if r.ID.GetUUID() == args.RecordID && args.Protected.LessEq(r.Timestamp) {
-			seen = true
-		}
-	})
-
-	// If we observed the record in question then we know that all future attempts
-	// to run GC will observe the Record if it still exists. The one hazard we
-	// need to avoid is a race whereby an attempt to run GC first checks the
-	// protected timestamp state and then attempts to increase the GC threshold.
-	// We set the minStateReadTimestamp here to avoid such races. The MVCC GC
-	// queue will call markPendingGC just prior to sending a request to update the
-	// GC threshold which will verify the safety of the new value relative to
-	// minStateReadTimestamp.
-	if seen {
-		r.protectedTimestampMu.minStateReadTimestamp = read.readAt
-		return true, false, "", nil
-	}
-
-	isCacheTooOld := read.readAt.Less(args.RecordAliveAt)
-	// Protected timestamp state has progressed past the point at which we
-	// should see this record. This implies that the record has been removed.
-	if !isCacheTooOld {
-		recordRemovedReason := "protected ts record has been removed"
-		return false, false, recordRemovedReason, nil
-	}
-	// Retry, since the cache is too old.
-	return false, true, "", nil
-}
-
 // checkProtectedTimestampsForGC determines whether the Replica can run GC. If
 // the Replica can run GC, this method returns the latest timestamp which can be
 // used to determine a valid new GCThreshold. The policy is passed in rather