sql, server: add skeleton TokenBucket connector and tenant resource limits configuration APIs

[RaduBerinde]

This PR is a scaled back version of #67508 where we don't use the system table at all. It's meant to put some of the infrastructure pieces in place and provide a stub API for reconfiguration.

The plan is to add consumption metrics on top of this soon so that CC can develop in parallel.

---

server: add TokenBucket connector API

This change adds the TokenBucket API proposed in the RFC (#66436), a
stub implementation and client for it, and the corresponding KV
connector interface.

The client and server-side code lives in
ccl/multitenantccl/tenantcostclient and tenantcostserver.

Release note: None

sql: tenant resource limits configuration API

This commit adds a crdb_internal.update_tenant_resource_limits
internal SQL function (to be used by the system tenant) which updates
the token bucket configuration for a specific tenant.

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

@kernfeld-cockroach please take a look at the configuration function:
https://github.com/cockroachdb/cockroach/pull/67768/files#diff-7c2fd288d1311559143c751945897a99a885f3f20df93d95fa1dee3c4b5c6049R5088

I have dropped the operation UUID from there. We have an alternate proposal for the internal state and we won't support that operation UUID anymore.

[kernfeld-cockroach]

Thanks for the heads-up. I am good with the current proposed function signature.

Since we're planning to call this function every 10 seconds, I don't think it matters that much to be able to reject out-of-date or duplicate requests. But if we wanted to leave forwards compatibility for that functionality, I would suggest that we include a monotonically increasing 64-bit integer nonce value. CRDB would ignore it for the MVP. But also, if there's not going to be a reasonable place to store the value anyways then there's likely no point adding it.

[ajwerner]

But if we wanted to leave forwards compatibility for that functionality, I would suggest that we include a monotonically increasing 64-bit integer nonce value. CRDB would ignore it for the MVP. But also, if there's not going to be a reasonable place to store the value anyways then there's likely no point adding it.

I assumed that the implementation of the client library would be tracking such a nonce inside the implementation of TenantSideCostController. Such a thing will need to exist. The SQL pod shouldn't be calling the gRPC method directly IIUC.

Reviewed 32 of 32 files at r1, 12 of 12 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

---

pkg/ccl/kvccl/kvtenantccl/connector.go, line 396 at r1 (raw file):

Quoted 5 lines of code…

	for ctx.Err() == nil {
		client, err := c.getClient(ctx)
		if err != nil {
			continue
		}

No logging or backoff?

---

pkg/ccl/kvccl/kvtenantccl/connector.go, line 399 at r1 (raw file):

		resp, err := client.TokenBucket(ctx, in)
		if err != nil {
			log.Warningf(ctx, "error issuing RangeLookup RPC: %v", err)

I see now that this is copy-pasta. Should we refactor out a helper?

---

pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go, line 41 at r1 (raw file):

	if err := stopper.RunAsyncTask(context.Background(), "refresher", func(ctx context.Context) {
		c.mainLoop(ctx)
	}); err != nil {

Thoughts on a start method? I generally dislike goroutines running under a stopper before calls to Start. I know they exist but I don't like them.

---

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 55 at r1 (raw file):

Quoted 4 lines of code…

		if err := updateTenantUsageState(ctx, s.executor, txn, tenantID, state); err != nil {
			return err
		}
		return nil

nit: return updateTenantUsageState(ctx, s.executor, txn, tenantID, state)

---

pkg/multitenant/tenant_usage.go, line 11 at r2 (raw file):

// licenses/APL.txt.

package multitenant

nit: add a doc.go or a package comment here

---

pkg/server/tenant.go, line 392 at r1 (raw file):

Quoted 5 lines of code…

// TenantSideCostController is an interface through which tenants report and
// throttle resource usage.
type TenantSideCostController interface {
	// TODO(radu)
}

Why define this here and not in the multitenant package I was going to suggest you create but you already did in the second commit?

[andy-kimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

---

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

	result := &roachpb.TokenBucketResponse{}
	if err := s.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {

How come it isn't possible to make this a single SQL statement that selects the current state and then updates it?

---

pkg/ccl/multitenantccl/tenantcostserver/tenanttokenbucket/tenant_token_bucket.go, line 23 at r2 (raw file):

type State struct {
	// Burst limit in RUs.
	RUBurstLimit float64

super nit: idiomatic Go would use field names as the first word in each comment.

[RaduBerinde]

TFTRs!

I assumed that the implementation of the client library would be tracking such a nonce inside the implementation of TenantSideCostController

Correct, the above is referring to the host-side reconfiguration API (for changing the refill rate etc)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andy-kimball)

---

pkg/ccl/kvccl/kvtenantccl/connector.go, line 399 at r1 (raw file):

Previously, ajwerner wrote…

I see now that this is copy-pasta. Should we refactor out a helper?

I tried but couldn't find a clean way to do it; other functions have some "hard error" return paths that are hard to incorporate in a func. I agree it should be done but it will require some back and forth so I'll leave that out of this PR.

Fixed the RPC name.

---

pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go, line 41 at r1 (raw file):

Previously, ajwerner wrote…

	if err := stopper.RunAsyncTask(context.Background(), "refresher", func(ctx context.Context) {
		c.mainLoop(ctx)
	}); err != nil {

Thoughts on a start method? I generally dislike goroutines running under a stopper before calls to Start. I know they exist but I don't like them.

Good point, done.

---

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

How come it isn't possible to make this a single SQL statement that selects the current state and then updates it?

How? By having some internal SQL function?

I don't know how we could make that work with the new proposal, where we have to interact with two rows.

In any case, I will start with a simple implementation and take it from there.

---

pkg/ccl/multitenantccl/tenantcostserver/tenanttokenbucket/tenant_token_bucket.go, line 23 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

super nit: idiomatic Go would use field names as the first word in each comment.

Done.

---

pkg/server/tenant.go, line 392 at r1 (raw file):

Previously, ajwerner wrote…

// TenantSideCostController is an interface through which tenants report and
// throttle resource usage.
type TenantSideCostController interface {
	// TODO(radu)
}

Why define this here and not in the multitenant package I was going to suggest you create but you already did in the second commit?

Done.

[andy-kimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andy-kimball, and @RaduBerinde)

---

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, RaduBerinde wrote…

How? By having some internal SQL function?

I don't know how we could make that work with the new proposal, where we have to interact with two rows.

In any case, I will start with a simple implementation and take it from there.

I just mean an INSERT INTO foo SELECT ... FROM foo kind of thing. Doesn't the code you have here run into serializability conflicts when multiple SQL pods call at the same time? If it were a single INSERT/SELECT statement that used FOR UPDATE locking, we'd never hit conflicts (I suppose we could use FOR UPDATE in readTenantUsage as well). But maybe I'm not understanding exactly what readTenantUsageState and updateTenantUsageState will be doing - my assumption is that we read 1 row, then use the values in that row to create another (new) row.

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andy-kimball)

---

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I just mean an INSERT INTO foo SELECT ... FROM foo kind of thing. Doesn't the code you have here run into serializability conflicts when multiple SQL pods call at the same time? If it were a single INSERT/SELECT statement that used FOR UPDATE locking, we'd never hit conflicts (I suppose we could use FOR UPDATE in readTenantUsage as well). But maybe I'm not understanding exactly what readTenantUsageState and updateTenantUsageState will be doing - my assumption is that we read 1 row, then use the values in that row to create another (new) row.

I will write up the new approach in the RFC when I get a chance (Andrew has a nice writeup in there, though we improved it a bit offline). We will be reading and updating two rows (but with no secondary index). We will use SELECT FOR UPDATE.

[andy-kimball]

I'm not too familiar with the surrounding code, but it with some small comments/questions. I'm approving now because I don't want you to get blocked too much longer, but hope that someone else in SQL can approve soon as well.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @andy-kimball, and @RaduBerinde)

---

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, RaduBerinde wrote…

I will write up the new approach in the RFC when I get a chance (Andrew has a nice writeup in there, though we improved it a bit offline). We will be reading and updating two rows (but with no secondary index). We will use SELECT FOR UPDATE.

OK, I just want to confirm we're understanding each other: I'm suggesting that all this code could just be a single SQL statement that round-trips just once to the DB rather than twice, thus reducing how long we hold locks:

INSERT INTO the_tenant_usage_table
SELECT u.Seq+1, u.RU+$1, u.ReadRequests+$2, u.ReadBytes+$3, ...
FROM the_tenant_usage_table u
WHERE its_the_existing_last_row()

And you're saying that this approach won't work for some reason, or maybe that it's too complex, right? It's fine if that's the case, I just want to make sure we're not mis-communicating.

---

pkg/server/server.go, line 639 at r6 (raw file):

		tenantUsage,
	)
	node.admissionQ = gcoord.GetWorkQueue(admission.KVWork)

Looks like admissionQ is getting set twice here, once in the constructor, once after.

Also, is it possible to not need to create a TenantUsageServer if this is not a multi-tenant cluster? Or will it be cheap enough that it doesn't matter?

[ajwerner]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @andy-kimball, and @RaduBerinde)

---

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Quoted 4 lines of code…

INSERT INTO the_tenant_usage_table
SELECT u.Seq+1, u.RU+$1, u.ReadRequests+$2, u.ReadBytes+$3, ...
FROM the_tenant_usage_table u
WHERE its_the_existing_last_row()

Whether you do it in a single statement and write it with the logic in SQL or do it in two statements, one which reads and the other which writes, it's roughly the same number of round-trips to the store. The advantage of the single-statement formulation is that I think we'll tell KV to commit in batch and thus hit the pure 1PC path with no intents.

[andy-kimball]

---

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, ajwerner wrote…

INSERT INTO the_tenant_usage_table
SELECT u.Seq+1, u.RU+$1, u.ReadRequests+$2, u.ReadBytes+$3, ...
FROM the_tenant_usage_table u
WHERE its_the_existing_last_row()

Whether you do it in a single statement and write it with the logic in SQL or do it in two statements, one which reads and the other which writes, it's roughly the same number of round-trips to the store. The advantage of the single-statement formulation is that I think we'll tell KV to commit in batch and thus hit the pure 1PC path with no intents.

Yes, I was thinking of round-trips from an app tier to SQL, which doesn't apply here, since this would all be running in a built-in function on a SQL node. However, I think there's actually 3 round-trips instead of 2, since committing the explicit transaction requires one extra round-trip.

[RaduBerinde]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @andy-kimball)

---

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Yes, I was thinking of round-trips from an app tier to SQL, which doesn't apply here, since this would all be running in a built-in function on a SQL node. However, I think there's actually 3 round-trips instead of 2, since committing the explicit transaction requires one extra round-trip.

Yeah I understand, I'll keep it in mind and see if we can do something like that once we're further in the implementation.

It would be nice if we had a way to set up the transaction to autocommit on the UPDATE statement.

---

pkg/server/server.go, line 639 at r6 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Looks like admissionQ is getting set twice here, once in the constructor, once after.

Also, is it possible to not need to create a TenantUsageServer if this is not a multi-tenant cluster? Or will it be cheap enough that it doesn't matter?

Fixed the admissionQ.

Is there an easy way to tell if this is a multi-tenant cluster?

I don't think this server will do any background work if there are no tenant requests coming through, so it shouldn't be a problem.

[RaduBerinde]

bors r+

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[RaduBerinde]

bors r-

[craig]

Canceled.

[RaduBerinde]

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)