protectedts: flip knob to enable multi-tenant PTS in clusters

[adityamaru]

This change switches the EnableProtectedTimestampForMultiTenant
testing knob to DisableProtectedTimestampForMultiTenant. This means
that all tests and clusters will now run with the ptpb.Target and spanconfig backed
protectedts infrastructure by default.

Informs: #73727

Release note: None

Release justification: high impact change to enable new functionality.

[cockroach-teamcity]

This change is 

[adityamaru]

@arulajmani most of the test changes were switching over from spans to targets, and waiting for KV to reconcile in some cases. The non-trivial changes were in TestStrictGCEnforcement and TestProtectedTimestamps, I don't know if those tests can be rewritten to be simpler, we can discuss this offline.

@samiskin I edited one of the tests you recently wrote for CDC. I also left a TODO for CDC which I'm happy to discuss and try to resolve with your help.

[adityamaru]

@samiskin @miretskiy I've had to rewrite some of the changefeed PTS tests to fit better with the new system where a PTS record written to the system table isn't enough to check a Protect. We actually have to check that the protection has made its way down to KV. I'd appreciate a look at the changes in changefeed_test.go. I'm trying to flip this knob before branch cut if possible so that everyone can start stressing the new subsystem during stability

[arulajmani]

I only have minor/nitty comments, the change looks . Thanks for doing this, it's exciting to see this wired up end to end, with integration tests!

I'll echo your comment from above, the changes to changefeed_test.go are non-trivial and I'm not too familiar with those tests. It's best to get someone from CDC to sign them off.

Reviewed 26 of 27 files at r1, 3 of 4 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @arulajmani, @dt, @miretskiy, and @samiskin)

---

pkg/spanconfig/spanconfigptsreader/adapter.go, line 97 at r2 (raw file):

	// Now ensure the KVSubscriber is fresh enough.
	testutils.SucceedsSoon(t, func() error {
		_, fresh, err := a.GetProtectionTimestamps(ctx, keys.EverythingSpan)

nit: I don't think this matters, does it?

---

pkg/kv/kvserver/client_replica_test.go, line 3458 at r2 (raw file):

// to the zone configs.
//
// TODO(adityamaru,arulajmani): Once the protectedts.Cache goes away this test

Do you have thoughts on how we'd change this once the Cache goes away? What you're doing here (replacing cache freshness by blocking the KVSubscriber to make progress) seems sane. I can't think of how we'd want to change this in the future, but maybe I'm missing something. Mind opening an issue with how you'd like to see this evolve once the Cache goes away? Alternatively, if you're happy with how this test is looking (I am!) then I wouldn't be sad seeing this TODO removed entirely.

---

pkg/kv/kvserver/client_replica_test.go, line 3597 at r2 (raw file):

			}
		}
		waitForProtectionAndReadProtectedTimestamps = func(t *testing.T, nodeID roachpb.NodeID,

Mind adding a comment for future us, once "waitForProtectionANdReadProtectedTimestamps" doesn't ring the same bell as it does today?

---

pkg/kv/kvserver/protectedts/ptcache/cache_test.go, line 283 at r2 (raw file):

	p := ptstorage.WithDatabase(ptstorage.New(s.ClusterSettings(),
		s.InternalExecutor().(sqlutil.InternalExecutor),
		&protectedts.TestingKnobs{DisableProtectedTimestampForMultiTenant: true}), s.DB())

nit: here, and below, formatting looks a bit off. Maybe:

	p := ptstorage.WithDatabase(
		ptstorage.New(
			s.ClusterSettings(),
			s.InternalExecutor().(sqlutil.InternalExecutor),
			&protectedts.TestingKnobs{DisableProtectedTimestampForMultiTenant: true},
		),
		s.DB(),
	)

---

pkg/kv/kvserver/protectedts/ptstorage/storage_test.go, line 676 at r2 (raw file):

		s := tc.Server(0)
		pts := s.ExecutorConfig().(sql.ExecutorConfig).ProtectedTimestampProvider

I noticed in a couple of places we've moved to using the TestCluster version of these interfaces (protectedts.Storage in this case) instead of constructing one with passed in testing knobs -- I'm curious if there was a reason for this switch?

[adityamaru]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @dt, @miretskiy, and @samiskin)

---

pkg/spanconfig/spanconfigptsreader/adapter.go, line 97 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: I don't think this matters, does it?

Does roachpb.Span{} and keys.EverythingSpan have the same effect? I was under the impression that by passing in EverythingSpan we would wait for all the frontier timestamps of the KVSubscribers rangefeed to be this fresh.

---

pkg/kv/kvserver/client_replica_test.go, line 3458 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Do you have thoughts on how we'd change this once the Cache goes away? What you're doing here (replacing cache freshness by blocking the KVSubscriber to make progress) seems sane. I can't think of how we'd want to change this in the future, but maybe I'm missing something. Mind opening an issue with how you'd like to see this evolve once the Cache goes away? Alternatively, if you're happy with how this test is looking (I am!) then I wouldn't be sad seeing this TODO removed entirely.

This is a stale comment, i rewrote the test a couple of times until i realized we no longer need waitForCacheAfter() anymore. Removed.

---

pkg/kv/kvserver/client_replica_test.go, line 3597 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Mind adding a comment for future us, once "waitForProtectionANdReadProtectedTimestamps" doesn't ring the same bell as it does today?

Done.

---

pkg/kv/kvserver/protectedts/ptstorage/storage_test.go, line 676 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I noticed in a couple of places we've moved to using the TestCluster version of these interfaces (protectedts.Storage in this case) instead of constructing one with passed in testing knobs -- I'm curious if there was a reason for this switch?

I felt that we were anyways spinning up a testcluster with the testing knobs, so didn't make sense to re-init a parallel pts.Storage. I'm unsure what the initial motivation to do that was.

[samiskin]

Changefeed test changes look good!

[adityamaru]

TFTRs!!

bors r=arulajmani,samiskin

[craig]

Build succeeded:

• GitHub CI (Cockroach)