kvserver,inspectz: add rac2 inspectz support

[kvoli]

The existing flow control tables are:

crdb_internal.kv_flow_controller
crdb_internal.kv_flow_control_handles
crdb_internal.kv_flow_token_deductions

Which are used in testing to assert on flow control stream state and
transitions.

Introduce another set of three tables, most of which are identical to the
exsting tables but populating the table data using rac2.

crdb_internal.kv_flow_controller_v2
crdb_internal.kv_flow_control_handles_v2
crdb_internal.kv_flow_token_deductions_v2

crdb_internal.kv_flow_controller_v2 has two additional columns,
tracking the amount of available (regular|elastic) send tokens. The
schema is:

CREATE TABLE crdb_internal.kv_flow_controller_v2 (
  tenant_id                     INT NOT NULL,
  store_id                      INT NOT NULL,
  available_eval_regular_tokens INT NOT NULL,
  available_eval_elastic_tokens INT NOT NULL,
  available_send_regular_tokens INT NOT NULL,
  available_send_elastic_tokens INT NOT NULL
);

Note that unless rac2 is enabled, the tables are unlikely to show
anything interesting. Both tables (v1 and v2) are kept for
compatibility, with an intent to replace the v1 tables with the v2 ones
after v1 replication flow control is removed entirely from the code.

Resolves: #128091
Release note: None

[cockroach-teamcity]

This change is 

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[kvoli]

Failure is from some missing crdb_internal datadriven test updates, I'll sort these out. Should be ready for a look.

[sumeerbhola]

Reviewed 6 of 6 files at r1, 13 of 13 files at r2, 7 of 7 files at r3, 4 of 4 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kvoli)

---

-- commits line 29 at r2:
nit: deductions.

---

-- commits line 83 at r5:
nit: existing

---

pkg/inspectz/inspectz.go line 148 at r4 (raw file):

// KVFlowHandlesV2 implements the InspectzServer interface.
func (s *Server) KVFlowHandlesV2(

This seems the same as Server.KVFlowHandles except for using handlesV2. Can this be unified into one function?

---

pkg/kv/kvserver/kvflowcontrol/kvflowcontrol.go line 323 at r3 (raw file):

	// Inspect returns the set of ranges that have an embedded
	// kvflowcontrol.Handle. It's used to power /inspectz.
	Inspect() []roachpb.RangeID

Now that we have a specialized lookup method for the inspectz path, do we need LookupInspect to return an InspectHandle or can we simply make it return (kvflowinspectpb.Handle, bool)?

---

pkg/kv/kvserver/kvflowcontrol/kvflowinspectpb/kvflowinspect.proto line 84 at r1 (raw file):

// Stream represents a given kvflowcontrol.Stream and the number of tokens
// available for it (as maintained by the node-level kvflowcontrol.Controller,
// or in >= v25.1, by the node level StreamTokenCounterProvider).

v24.3?

---

pkg/kv/kvserver/kvflowcontrol/kvflowinspectpb/kvflowinspect.proto line 93 at r1 (raw file):

    (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.StoreID"];
  // AvailableEvalRegularTokens represents the currently available tokens for
  // regular replication traffic, deducted before evaluation.

should this say "deducted for evaluation"?
(we don't actually deduct them before evaluation)

---

pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 422 at r2 (raw file):

func (rc *rangeController) Inspect(ctx context.Context) kvflowinspectpb.Handle {
	rc.mu.Lock()
	defer rc.mu.Unlock()

rc.mu does not protect replicaMap, IIRC.
We are relying on external synchronization, specifically raftMu for this.

As mentioned in https://cockroachlabs.slack.com/archives/C06UFBJ743F/p1726018900176919?thread_ts=1726008270.371049&cid=C06UFBJ743F, I think it is acceptable to rename this InspectRaftMuLocked and have the caller acquire raftMu. The higher latency is fine for inspectz and querying the internal tables.

---

pkg/kv/kvserver/kvflowcontrol/rac2/store_stream.go line 112 at r2 (raw file):

	slices.SortFunc(streams, func(a, b kvflowinspectpb.Stream) int { // for determinism
		return cmp.Or(
			cmp.Compare(a.TenantID.ToUint64(), b.TenantID.ToUint64()),

is there a reason this is sorting the tuple (tenant-id, store-id), while rangeController.Inspect is sorting (store-id, tenant-id)?

---

pkg/server/server.go line 1134 at r4 (raw file):

		cfg.BaseConfig.AmbientCtx,
		node.storeCfg.KVFlowHandles,
		kvserver.MakeStoresForRACv2(stores),

It just happens that MakeStoresForRACv2 is casting stores and does not have some additional state, so calling MakeStoresForRACv2 a second time does not accidentally create a second object when it is expected to be a singleton. We shouldn't rely on this. I think we already have storesForRACv2 as a local variable from earlier in this function, and should use it here.

---

pkg/kv/kvserver/kvflowcontrol/rac2/testdata/range_controller/inspect line 224 at r2 (raw file):

# Change the replica on s3 to StateSnapshot. This should untrack all entries
# and result in the stream token counts being updated accordingly.

I think we should not consider s3 to have a connected stream. Currently the code does

		if rs.sendStream == nil {
			continue
		}

and since it was in StateReplicate and transitioned to StateSnapshot we still have the replicaSendStream and print the state below. Up to you whether you want to change this now, or wait until the equivalent of #130168 where all transitions to StateSnapshot close the replicaSendStream.

[kvoli]

TYFTR! Addressed the comments, ptal.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kvoli and @sumeerbhola)

---

-- commits line 29 at r2:

Previously, sumeerbhola wrote…

nit: deductions.

Updated.

---

-- commits line 83 at r5:

Previously, sumeerbhola wrote…

nit: existing

Updated.

---

pkg/inspectz/inspectz.go line 148 at r4 (raw file):

Previously, sumeerbhola wrote…

This seems the same as Server.KVFlowHandles except for using handlesV2. Can this be unified into one function?

We need to have both methods for the internal tables to call. I've simplified the logic however to unify the logic into a function.

---

pkg/kv/kvserver/kvflowcontrol/kvflowcontrol.go line 323 at r3 (raw file):

Previously, sumeerbhola wrote…

Now that we have a specialized lookup method for the inspectz path, do we need LookupInspect to return an InspectHandle or can we simply make it return (kvflowinspectpb.Handle, bool)?

We can, updated.

---

pkg/kv/kvserver/kvflowcontrol/kvflowinspectpb/kvflowinspect.proto line 84 at r1 (raw file):

Previously, sumeerbhola wrote…

v24.3?

Should be v24.3, I've mixed these up a few times w/ the innovation release.

---

pkg/kv/kvserver/kvflowcontrol/kvflowinspectpb/kvflowinspect.proto line 93 at r1 (raw file):

Previously, sumeerbhola wrote…

should this say "deducted for evaluation"?
(we don't actually deduct them before evaluation)

It should. Updated.

---

pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 422 at r2 (raw file):

Previously, sumeerbhola wrote…

rc.mu does not protect replicaMap, IIRC.
We are relying on external synchronization, specifically raftMu for this.

As mentioned in https://cockroachlabs.slack.com/archives/C06UFBJ743F/p1726018900176919?thread_ts=1726008270.371049&cid=C06UFBJ743F, I think it is acceptable to rename this InspectRaftMuLocked and have the caller acquire raftMu. The higher latency is fine for inspectz and querying the internal tables.

It doesn't. Updated the method naming and removed the locking.

---

pkg/kv/kvserver/kvflowcontrol/rac2/store_stream.go line 112 at r2 (raw file):

Previously, sumeerbhola wrote…

is there a reason this is sorting the tuple (tenant-id, store-id), while rangeController.Inspect is sorting (store-id, tenant-id)?

There is no reason. I've updated rangeController to sort on tenant, store.

---

pkg/server/server.go line 1134 at r4 (raw file):

Previously, sumeerbhola wrote…

It just happens that MakeStoresForRACv2 is casting stores and does not have some additional state, so calling MakeStoresForRACv2 a second time does not accidentally create a second object when it is expected to be a singleton. We shouldn't rely on this. I think we already have storesForRACv2 as a local variable from earlier in this function, and should use it here.

Updated to use the local var.

---

pkg/kv/kvserver/kvflowcontrol/rac2/testdata/range_controller/inspect line 224 at r2 (raw file):

Previously, sumeerbhola wrote…

I think we should not consider s3 to have a connected stream. Currently the code does

		if rs.sendStream == nil {
			continue
		}

and since it was in StateReplicate and transitioned to StateSnapshot we still have the replicaSendStream and print the state below. Up to you whether you want to change this now, or wait until the equivalent of #130168 where all transitions to StateSnapshot close the replicaSendStream.

I'll wait until the equivalent of #130168, you're right it shouldn't be included.

[sumeerbhola]

Reviewed 1 of 27 files at r6, 5 of 16 files at r7, 6 of 9 files at r8, 3 of 4 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @kvoli)

---

pkg/inspectz/inspectz.go line 115 at r10 (raw file):

	ctx context.Context, request *kvflowinspectpb.ControllerRequest,
) (*kvflowinspectpb.ControllerResponse, error) {
	return &kvflowinspectpb.ControllerResponse{

can this use the kvFlowController method?

[kvoli]

TYFTR! I pushed a few more test file changes related to the new internal tables to appease logic tests which are affected.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mw5h and @sumeerbhola)

---

pkg/inspectz/inspectz.go line 115 at r10 (raw file):

Previously, sumeerbhola wrote…

can this use the kvFlowController method?

It can, I missed it. Done.

[kvoli]

bors r=sumeerbhola

[craig]

Build failed:

• acceptance

[kvoli]

bors r-

[kvoli]

Merge skew, going to rebase and go again.

[kvoli]

bors r=sumeerbhola

[craig]

Build succeeded:

• unit_tests
• docker_image_amd64
• macos_arm64_build
• check_generated_code
• linux_arm64_build
• local_roachtest
• linux_amd64_build
• macos_amd64_build
• linux_amd64_fips_build
• local_roachtest_fips
• windows_build
• acceptance
• examples_orms
• lint

[msbutler]

@kvoli it looks like this pr modified the crdb_internal_catalog logic test to verify the schema of every crdb_internal vtable. what was the motivation for adding this check? I ask because now every time anybody updates a crdb_internal table (which my team at least does all the time), they need to rewrite this test, which is a minor annoyance and could lead to merge conflicts.

[kvoli]

@msbutler that was unintentional, I assumed it was necessary from gleaning other PRs but from your message I am guessing thats not correct. We can and should remove it if its causing inconvenience.