multitenant: only charge for pgwire egress and update cost model settings by RaduBerinde · Pull Request #71094 · cockroachdb/cockroach

RaduBerinde · 2021-10-04T19:48:48Z

tenantrate: revive KV Compute Units

Sharing the same cost model for tenant costing and for KV-side tenant
limiting was a mistake - the latter is meant to estimate CPU usage as
much as possible whereas we want to take arbitrary freedoms with
tenant costing.

This commit revives the kv.tenant_rate_limiter.* settings for the
cost model and reverts to using the concept of "KV Compute Units"
inside the rate limiter.

Release note: None

Release justification: Necessary fix for the distributed rate limiting
functionality, which is vital for the upcoming Serverless MVP release.
It allows CRDB to throttle clusters that have run out of free or paid
request units (which measure CPU and I/O usage). This functionality is
only enabled in multi-tenant scenarios and should have no impact on
our dedicated customers.

multitenant: only charge for pgwire egress

This commit changes the tenant cost accounting for pgwire bytes to
only take into account egress; cloud providers don't charge for
ingress. In the future, we can plumb the ingress bytes separately if
needed - as of now we don't plan to charge for ingress.

Release note: None

Release justification: Necessary fix for the distributed rate limiting
functionality, which is vital for the upcoming Serverless MVP release.
It allows CRDB to throttle clusters that have run out of free or paid
request units (which measure CPU and I/O usage). This functionality is
only enabled in multi-tenant scenarios and should have no impact on
our dedicated customers.

tenantcostmodel: update settings

This commit updates settings for the tenant cost model. These values
are derived from the Tenant Pricing Model spreadsheet.

Release note: None

Release justification: Necessary fix for the distributed rate limiting
functionality, which is vital for the upcoming Serverless MVP release.
It allows CRDB to throttle clusters that have run out of free or paid
request units (which measure CPU and I/O usage). This functionality is
only enabled in multi-tenant scenarios and should have no impact on
our dedicated customers.

cockroach-teamcity · 2021-10-04T19:48:57Z

This change is

RaduBerinde · 2021-10-04T19:59:46Z

pkg/kv/kvserver/tenantrate/settings.go, line 55 at r2 (raw file):

		"per-tenant rate limit in Request Units per second if positive, "+
			"or Request Units per second per CPU if negative",
		-500,

Not very sure how to go about setting this. Definitely seems like we should increase it, now that writes are 5x more expensive and reads are 2x.

RaduBerinde · 2021-10-05T03:59:44Z

Updated - I added a commit that revives the separate tenantrate limiter settings; cost model changes no longer affect that subsystem.

andy-kimball

Do you foresee any problems if we upgrade from the previous code to this code in our Staging/Production environments?

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

pkg/kv/kvserver/tenantrate/settings.go, line 52 at r5 (raw file):

// seconds), so it doesn't need to be adjusted in concert with the rate.
var (
	// kvcuRateLimit was initially set to an absolute value in KV Compute Units/s,

NIT: Somewhere in here, can you add a comment explaining the difference between the constants used in distributed rate limiting vs. the constants used in KV tenant throttling?

RaduBerinde

TFTR! There should be no problem upgrading.

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

pkg/kv/kvserver/tenantrate/settings.go, line 52 at r5 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

NIT: Somewhere in here, can you add a comment explaining the difference between the constants used in distributed rate limiting vs. the constants used in KV tenant throttling?

Done, PTAL

Sharing the same cost model for tenant costing and for KV-side tenant limiting was a mistake - the latter is meant to estimate CPU usage as much as possible whereas we want to take arbitrary freedoms with tenant costing. This commit revives the `kv.tenant_rate_limiter.*` settings for the cost model and reverts to using the concept of "KV Compute Units" inside the rate limiter. Release note: None Release justification: Necessary fix for the distributed rate limiting functionality, which is vital for the upcoming Serverless MVP release. It allows CRDB to throttle clusters that have run out of free or paid request units (which measure CPU and I/O usage). This functionality is only enabled in multi-tenant scenarios and should have no impact on our dedicated customers.

This commit changes the tenant cost accounting for pgwire bytes to only take into account egress; cloud providers don't charge for ingress. In the future, we can plumb the ingress bytes separately if needed - as of now we don't plan to charge for ingress. Release note: None Release justification: Necessary fix for the distributed rate limiting functionality, which is vital for the upcoming Serverless MVP release. It allows CRDB to throttle clusters that have run out of free or paid request units (which measure CPU and I/O usage). This functionality is only enabled in multi-tenant scenarios and should have no impact on our dedicated customers.

This commit updates settings for the tenant cost model. These values are derived from the Tenant Pricing Model spreadsheet. Release note: None Release justification: Necessary fix for the distributed rate limiting functionality, which is vital for the upcoming Serverless MVP release. It allows CRDB to throttle clusters that have run out of free or paid request units (which measure CPU and I/O usage). This functionality is only enabled in multi-tenant scenarios and should have no impact on our dedicated customers.

RaduBerinde · 2021-10-06T18:55:33Z

bors r+

craig · 2021-10-06T20:06:35Z

Build succeeded:

GitHub CI (Cockroach)

blathers-crl · 2021-10-06T20:06:46Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 56b8c5d to blathers/backport-release-21.2-71094: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.

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

RaduBerinde requested a review from andy-kimball October 4, 2021 19:48

RaduBerinde requested a review from a team as a code owner October 4, 2021 19:48

RaduBerinde changed the title ~~tenantcostmodel: update settings~~ multitenant: only chagre for pgwire egress and update cost model settings Oct 5, 2021

RaduBerinde changed the title ~~multitenant: only chagre for pgwire egress and update cost model settings~~ multitenant: only charge for pgwire egress and update cost model settings Oct 5, 2021

RaduBerinde force-pushed the egress-only branch from 6e3fc19 to 4c92953 Compare October 5, 2021 03:58

RaduBerinde requested a review from ajwerner October 5, 2021 04:06

RaduBerinde added the backport-21.2.x label Oct 5, 2021

andy-kimball approved these changes Oct 5, 2021

View reviewed changes

RaduBerinde force-pushed the egress-only branch from 4c92953 to 7641be8 Compare October 5, 2021 15:46

RaduBerinde commented Oct 5, 2021

View reviewed changes

RaduBerinde added 3 commits October 5, 2021 12:36

RaduBerinde force-pushed the egress-only branch from 7641be8 to 28ff523 Compare October 5, 2021 19:37

RaduBerinde mentioned this pull request Oct 5, 2021

multitenant: tasklist for cost control MVP #68479

Closed

15 tasks

craig bot merged commit 41ee2e9 into cockroachdb:master Oct 6, 2021

RaduBerinde mentioned this pull request Oct 7, 2021

release-21.2: multitenant: only charge for pgwire egress and update cost model settings #71286

Merged

RaduBerinde deleted the egress-only branch October 12, 2021 03:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multitenant: only charge for pgwire egress and update cost model settings#71094

multitenant: only charge for pgwire egress and update cost model settings#71094
craig[bot] merged 3 commits intocockroachdb:masterfrom
RaduBerinde:egress-only

RaduBerinde commented Oct 4, 2021 •

edited

Loading

Uh oh!

cockroach-teamcity commented Oct 4, 2021

Uh oh!

RaduBerinde commented Oct 4, 2021

Uh oh!

RaduBerinde commented Oct 5, 2021

Uh oh!

andy-kimball left a comment

Uh oh!

RaduBerinde left a comment

Uh oh!

RaduBerinde commented Oct 6, 2021

Uh oh!

craig bot commented Oct 6, 2021

Uh oh!

blathers-crl bot commented Oct 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

RaduBerinde commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

tenantrate: revive KV Compute Units

multitenant: only charge for pgwire egress

tenantcostmodel: update settings

Uh oh!

cockroach-teamcity commented Oct 4, 2021

Uh oh!

RaduBerinde commented Oct 4, 2021

Uh oh!

RaduBerinde commented Oct 5, 2021

Uh oh!

andy-kimball left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde commented Oct 6, 2021

Uh oh!

craig bot commented Oct 6, 2021

Uh oh!

blathers-crl bot commented Oct 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RaduBerinde commented Oct 4, 2021 •

edited

Loading