Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: express tenant rate limits in compute units over time #55114

Closed
tbg opened this issue Oct 1, 2020 · 4 comments · Fixed by #62783
Closed

kvserver: express tenant rate limits in compute units over time #55114

tbg opened this issue Oct 1, 2020 · 4 comments · Fixed by #62783
Labels
A-kv-server Relating to the KV-level RPC server A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@tbg
Copy link
Member

tbg commented Oct 1, 2020

Is your feature request related to a problem? Please describe.

Rate limits are currently configured separately for each limiter, i.e. there is a limit for reads, a limit for writes, etc. - they are not interconnected. (There are also more limiters, but simplifying here for purpose of illustration).

Externally, we want to express resource consumption via "compute units" (abbreviated cu, at least in this issue), where 1cu = A*reads + B*writes, and A and B are constants that we chose experimentally.

For example, if A=2 and B=5 (i.e. five reads "cost" the same as two writes), then with 100cu, one could either perform 50 reads or 20 writes.

This requires changes to how the limiters maintain their quota - they need to share a token bucket.

Describe the solution you'd like
Change the rate limiters to use a shared bucket, and give each limiter with a weight (A, B). Currently, each rate limiter has its own defined rate and burst:

// Limit defines a rate in units per second.
type Limit float64
// LimitConfig configures the rate limit and burst limit for a given resource.
type LimitConfig struct {
Rate Limit
Burst int64
}
// LimitConfigs configures the rate limits.
// It is exported for convenience and testing.
// The values are derived from cluster settings.
type LimitConfigs struct {
ReadRequests LimitConfig
WriteRequests LimitConfig
ReadBytes LimitConfig
WriteBytes LimitConfig
}

I think instead we want this token bucket to deal in cu:

https://github.com/cockroachdb/cockroach/blob/4c9110f7040b1d911e5d02760fe7ceacc24413ca/pkg/kv/kvserver/tenantrate/limiter.go#L88-L935

The quota pool underlying the token bucket will deal in "compute units" and will refill at a specified rate (and change the settings to deal in that rate). Each limiter will pull from the pool with a given weight, i.e. in the above example a read would pull with a factor of two and a write with a factor of five. The compute units can thus be spent as required by the traffic pattern.

I am partially science dog on this and need a close reading of the code to get clarity on how the quota pool, resource, request, and token buckets work together here, but it looks flexible to make that happen.

Describe alternatives you've considered

Additional context
By switching to compute units everywhere, we also lose the ability to rate limit on individual dimensions. Say, for example, that we want to make a tenant "read-only". We would then severely limit the write rate (not to zero, to allow for descriptor lease acquisition, etc) but not the read rate limit.

It seems unappealing to maintain both the current and the new system side-by-side. Instead, we could also make the constants A, B configurable (per-tenant, as part of #53477). For example, A=1, B=1000 skews the cost model decisively towards reads, meaning that writes will consume huge amounts of quota. However - that is still undesirable, as in the read-only scenario we want the writes to be slowed down, but not consume all of the quota, as a base amount of writes is needed just for basic functions.

Unless there is another idiomatic solution here, I would say we punt on the problem. We can always add additional configuration on the tenant later that introduces this complexity; it does not have to be part of the initial solution.

@tbg tbg added A-kv-server Relating to the KV-level RPC server A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Oct 1, 2020
@tbg
Copy link
Member Author

tbg commented Oct 1, 2020

@ajwerner
Copy link
Contributor

ajwerner commented Oct 5, 2020

My sense is that if we have a good model of compute units, we could add compute units as an additional dimension in the multi-dimensional quotapool and then go and raise the limits in the other dimensions. Such an approach could provide both flexibility and recourse if the model proves to be a bad fit at the cost of some additional complexity.

Another question is how dynamic and flexible to make the compute unit model. Do we want A and B to be dynamic? How should we set their values?

@tbg
Copy link
Member Author

tbg commented Jan 13, 2021

My understanding is that they will be fixed in practice (though perhaps we'll find that we need to adjust them on a per-tenant basis sporadically, to account for accounts that are grandfathered in, manual exceptions, testing, etc) since they will also form part of the cost model, i.e. become part of documentation.

Just to make sure I understand your suggestion, you are saying that we compute for each request the "request units" and simply add the request units as a separate quota pool to draw from, but leave the other dimensions in place (but make them less strict)? I'm curious what the benefits of that are.

Paging this back in just now, if we move to distributed rate limiting, I could see how both dimensions make sense. The request units control the user's aggregate cost, but they don't necessarily protect the KV layer (if a node prefetches a gazillion request units, we still don't want to let the client go fast enough to overload the server). So we set limits for the existing dimensions solely to protect KV. Is that roughly what you were thinking?
Taking this to its logical conclusion though, the tenant rate limiter would only be concerned with controlling the user cost (i.e. deals in request units and goes as fast as the system will let it). The KV layer is then protected by admission control, which is sort of like a bucket shared by all tenants.

@ajwerner
Copy link
Contributor

I think you're right that I was too enthusiastic about the idea of maintaining the multi-dimensional nature of the rate limiter. I think with a good cost model, there's no reason for it (assuming this is all about billing).

Is that roughly what you were thinking?

I think you're being too generous to me. I think I had in my head the idea that our cost model might be stupid and that these simpler limits seems easier to not mess up. I guess, by mess up I did mean use too many resources.

If we want this rate limiter to also be protecting the usage of resources on the node then I suspect we'd need a somewhat different design whereby the node has some predetermined capacity to be shared among all of the tenants in some way.

RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Mar 30, 2021
Tenant rate limiting currently uses four separate token buckets, each
with its own rate and burst limits. In this commit we switch to a
single shared token bucket (see cockroachdb#55114 for motivation).

We use a "cost model" to map read and write requests to "KV Compute
Units". Requests have a base cost plus a per-byte cost. The details
are documented in settings.go. The values were chosen based on
experiments ran by Nathan:
https://docs.google.com/spreadsheets/d/1PPlIcKnusOqWtBoOZVd9xBEMPe5Ss1FgrJlYFgpQZaM/edit#gid=735409177

The rate was chosen so that it maps to 20% of 1 KV vCPU. This keeps
the rate limit on small requests roughly the same as before
(especially on mixed workloads).

The largest departure from the previous limits is that we allow much
more read bytes (the per-byte cost of reads is small in the cost
model). If we were to keep close to the previous limits, the value of
kv.tenant_rate_limiter.read_cost_per_megabyte would be 200 instead of
10.  Perhaps we want to be more conservative here and make this
value somewhere in-between?

Fixes cockroachdb#55114.

Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Apr 1, 2021
Tenant rate limiting currently uses four separate token buckets, each
with its own rate and burst limits. In this commit we switch to a
single shared token bucket (see cockroachdb#55114 for motivation).

We use a "cost model" to map read and write requests to "KV Compute
Units". Requests have a base cost plus a per-byte cost. The details
are documented in settings.go. The values were chosen based on
experiments ran by Nathan:
https://docs.google.com/spreadsheets/d/1PPlIcKnusOqWtBoOZVd9xBEMPe5Ss1FgrJlYFgpQZaM/edit#gid=735409177

The rate was chosen so that it maps to 20% of 1 KV vCPU. This keeps
the rate limit on small requests roughly the same as before
(especially on mixed workloads).

The largest departure from the previous limits is that we allow much
more read bytes (the per-byte cost of reads is small in the cost
model). If we were to keep close to the previous limits, the value of
kv.tenant_rate_limiter.read_cost_per_megabyte would be 200 instead of
10.  Perhaps we want to be more conservative here and make this
value somewhere in-between?

Fixes cockroachdb#55114.

Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Apr 8, 2021
Tenant rate limiting currently uses four separate token buckets, each
with its own rate and burst limits. In this commit we switch to a
single shared token bucket (see cockroachdb#55114 for motivation).

We use a "cost model" to map read and write requests to "KV Compute
Units". Requests have a base cost plus a per-byte cost. The details
are documented in settings.go. The values were chosen based on
experiments ran by Nathan:
https://docs.google.com/spreadsheets/d/1PPlIcKnusOqWtBoOZVd9xBEMPe5Ss1FgrJlYFgpQZaM/edit#gid=735409177

The rate was chosen so that it maps to 20% of 1 KV vCPU. This keeps
the rate limit on small requests roughly the same as before
(especially on mixed workloads).

The largest departure from the previous limits is that we allow much
more read bytes (the per-byte cost of reads is small in the cost
model). If we were to keep close to the previous limits, the value of
kv.tenant_rate_limiter.read_cost_per_megabyte would be 200 instead of
10.  Perhaps we want to be more conservative here and make this
value somewhere in-between?

Fixes cockroachdb#55114.

Release note: None
craig bot pushed a commit that referenced this issue Apr 14, 2021
62783: tenantrate: switch to a single token bucket r=RaduBerinde a=RaduBerinde

Tenant rate limiting currently uses four separate token buckets, each
with its own rate and burst limits. In this commit we switch to a
single shared token bucket (see #55114 for motivation).

We use a "cost model" to map read and write requests to "KV Compute
Units". Requests have a base cost plus a per-byte cost. The details
are documented in settings.go. The values were chosen based on
experiments ran by Nathan:
https://docs.google.com/spreadsheets/d/1PPlIcKnusOqWtBoOZVd9xBEMPe5Ss1FgrJlYFgpQZaM/edit#gid=735409177

The rate was chosen so that it maps to 20% of 1 KV vCPU. This keeps
the rate limit on small requests roughly the same as before
(especially on mixed workloads).

The largest departure from the previous limits is that we allow much
more read bytes (the per-byte cost of reads is small in the cost
model). If we were to keep close to the previous limits, the value of
kv.tenant_rate_limiter.read_cost_per_megabyte would be 200 instead of
10.  Perhaps we want to be more conservative here and make this
value somewhere in-between?

Fixes #55114.

Release note: None

Co-authored-by: Radu Berinde <[email protected]>
@craig craig bot closed this as completed in 5c560bd Apr 14, 2021
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue May 24, 2021
Tenant rate limiting currently uses four separate token buckets, each
with its own rate and burst limits. In this commit we switch to a
single shared token bucket (see cockroachdb#55114 for motivation).

We use a "cost model" to map read and write requests to "KV Compute
Units". Requests have a base cost plus a per-byte cost. The details
are documented in settings.go. The values were chosen based on
experiments ran by Nathan:
https://docs.google.com/spreadsheets/d/1PPlIcKnusOqWtBoOZVd9xBEMPe5Ss1FgrJlYFgpQZaM/edit#gid=735409177

The rate was chosen so that it maps to 20% of 1 KV vCPU. This keeps
the rate limit on small requests roughly the same as before
(especially on mixed workloads).

The largest departure from the previous limits is that we allow much
more read bytes (the per-byte cost of reads is small in the cost
model). If we were to keep close to the previous limits, the value of
kv.tenant_rate_limiter.read_cost_per_megabyte would be 200 instead of
10.  Perhaps we want to be more conservative here and make this
value somewhere in-between?

Fixes cockroachdb#55114.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-server Relating to the KV-level RPC server A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants