Skip to content

server,kvserver: set tenant weights for admission control#77575

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
sumeerbhola:tenant2
Mar 11, 2022
Merged

server,kvserver: set tenant weights for admission control#77575
craig[bot] merged 1 commit intocockroachdb:masterfrom
sumeerbhola:tenant2

Conversation

@sumeerbhola
Copy link
Copy Markdown
Collaborator

The weights are used in ordering tenants, such that tenant i is
preferred over tenant j if used_i/weight_i < used_j/weight_j,
where the used values represent usage of slots or tokens. It
allows for a primitive form of weighted fair sharing. This
was added in a preceding PR.

This PR integrates this existing support for weighting, by

  • making Node provide these weights by counting the number
    of ranges for each tenant, both per store and across the
    whole node.
  • making KVAdmissionControllerImpl periodically poll for these
    weights (at a 10min interval) and provide these to the
    single KV WorkQueue and each of the kv-stores WorkQueues.

An alternative would be to put the polling logic inside the
admission control package. The difficulty there is that we
want to synchronize the computaton across the whole node,
instead of doing it once per WorkQueue.
KVAdmissionControllerImpl provides a convenient place to place
this synchronized computation.

The cluster settings for using weights are set to a default
that disables weights.

Informs #77358

Release justification: Low-risk update to new functionality.
Even though inter-tenant isolation was included in v21.2,
it has only been used in CockroachDB serverless recently,
and there is consensus to include weighting for v22.1.
Additionally, weights are disabled by default.

Release note (ops change): The cluster settings
admission.kv.tenant_weights.enabled and
admission.kv.stores.tenant_weights.enabled can be used to
enable tenant weights in multi-tenant storage servers. The
default is disabled.

@sumeerbhola sumeerbhola requested review from a team as code owners March 9, 2022 22:16
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach)

Copy link
Copy Markdown
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/server/node_test.go, line 716 at r1 (raw file):

	})
	// Get weights and check.
	// TODO(sumeer): enhance test to have multiple tenants.

@ajwerner Any quick ideas on how to create ranges belonging to different tenants?

Copy link
Copy Markdown
Contributor

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:LGTM:

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner and @cucaroach)

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @sumeerbhola)


pkg/server/node_test.go, line 716 at r1 (raw file):

Previously, sumeerbhola wrote…

@ajwerner Any quick ideas on how to create ranges belonging to different tenants?

You can do this:

const tenantID = 10
prefix := keys.MakeTenantPrefix(roachpb.MakeTenant(tenantID))
require.NoError(t, kvDB.AdminSplit(ctx, prefix, hlc.MaxTimestamp))

Copy link
Copy Markdown
Contributor

@nvb nvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @sumeerbhola)


pkg/util/admission/work_queue.go, line 223 at r1 (raw file):

// Note that currently there are no weights associated with tenants -- one
// could imagine using weights and comparing used/weight values for
// inter-tenant fairness.

Is it time to remove this comment?

Copy link
Copy Markdown
Contributor

@nvb nvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @sumeerbhola)


pkg/util/admission/work_queue.go, line 1124 at r1 (raw file):

ordered in increasing order of tenantInfo.used

Is this still accurate, as of 8f385b4?

The weights are used in ordering tenants, such that tenant i is
preferred over tenant j if used_i/weight_i < used_j/weight_j,
where the used values represent usage of slots or tokens. It
allows for a primitive form of weighted fair sharing. This
was added in a preceding PR.

This PR integrates this existing support for weighting, by
- making Node provide these weights by counting the number
  of ranges for each tenant, both per store and across the
  whole node.
- making KVAdmissionControllerImpl periodically poll for these
  weights (at a 10min interval) and provide these to the
  single KV WorkQueue and each of the kv-stores WorkQueues.

An alternative would be to put the polling logic inside the
admission control package. The difficulty there is that we
want to synchronize the computaton across the whole node,
instead of doing it once per WorkQueue.
KVAdmissionControllerImpl provides a convenient place to place
this synchronized computation.

The cluster settings for using weights are set to a default
that disables weights.

Informs cockroachdb#77358

Release justification: Low-risk update to new functionality.
Even though inter-tenant isolation was included in v21.2,
it has only been used in CockroachDB serverless recently,
and there is consensus to include weighting for v22.1.
Additionally, weights are disabled by default.

Release note (ops change): The cluster settings
admission.kv.tenant_weights.enabled and
admission.kv.stores.tenant_weights.enabled can be used to
enable tenant weights in multi-tenant storage servers. The
default is disabled.
Copy link
Copy Markdown
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTRs!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @ajwerner, @cucaroach, and @nvanbenschoten)


pkg/util/admission/work_queue.go, line 223 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is it time to remove this comment?

ah, yes. Removed and adjusted the comment above.


pkg/util/admission/work_queue.go, line 1124 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

ordered in increasing order of tenantInfo.used

Is this still accurate, as of 8f385b4?

Good catch. Fixed.


pkg/server/node_test.go, line 716 at r1 (raw file):

Previously, ajwerner wrote…

You can do this:

const tenantID = 10
prefix := keys.MakeTenantPrefix(roachpb.MakeTenant(tenantID))
require.NoError(t, kvDB.AdminSplit(ctx, prefix, hlc.MaxTimestamp))

Thanks!

@sumeerbhola
Copy link
Copy Markdown
Collaborator Author

bors r=ajwerner,nvanbenschoten,cucaroach

@craig craig bot merged commit 0fb2c0c into cockroachdb:master Mar 11, 2022
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 11, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants