Skip to content

Commit

Permalink
Merge cockroachdb#98148
Browse files Browse the repository at this point in the history 
98148: kv: exempt tenant from KV-side limiting based on capability r=stevendanna a=stevendanna

The KV-side rate limiting can, by design, substantially limit the performance of a tenant.  For UA clusters, we want the ability to designate a cluster exempt from the rate limiting.

Note that we still pass all requests through the limiter and keep statistics.

Epic: none

Release note: None

Co-authored-by: Steven Danna <[email protected]>
  • Loading branch information
@stevendanna
craig[bot] and stevendanna committed Mar 16, 2023
2 parents d68f5cd + 2738810 commit 4dc10b5
Show file tree
Hide file tree
Showing 21 changed files with 440 additions and 111 deletions.
135 changes: 86 additions & 49 deletions pkg/ccl/logictestccl/testdata/logic_test/tenant_capability
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,14 @@ CREATE TENANT "no-capabilities-tenant";
query TT colnames
SELECT capability_name, capability_value FROM [SHOW TENANT "no-capabilities-tenant" WITH CAPABILITIES]
----
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false

subtest end

Expand All @@ -51,27 +52,29 @@ ALTER TENANT "bool-capability-no-value-tenant" GRANT CAPABILITY can_admin_split
query TT colnames
SELECT capability_name, capability_value FROM [SHOW TENANT "bool-capability-no-value-tenant" WITH CAPABILITIES]
----
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false

statement ok
ALTER TENANT "bool-capability-no-value-tenant" REVOKE CAPABILITY can_admin_split

query TT colnames
SELECT capability_name, capability_value FROM [SHOW TENANT "bool-capability-no-value-tenant" WITH CAPABILITIES]
----
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false

subtest end

Expand All @@ -86,13 +89,14 @@ ALTER TENANT "bool-capability-with-value-tenant" GRANT CAPABILITY can_admin_spli
query TT colnames
SELECT capability_name, capability_value FROM [SHOW TENANT "bool-capability-with-value-tenant" WITH CAPABILITIES]
----
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false

subtest end

Expand All @@ -107,13 +111,14 @@ ALTER TENANT "bool-capability-with-expression-value-tenant" GRANT CAPABILITY can
query TT colnames
SELECT capability_name, capability_value FROM [SHOW TENANT "bool-capability-with-expression-value-tenant" WITH CAPABILITIES]
----
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false

subtest end

Expand All @@ -128,26 +133,58 @@ ALTER TENANT "multiple-capability-tenant" GRANT CAPABILITY can_admin_split, can_
query TT colnames
SELECT capability_name, capability_value FROM [SHOW TENANT "multiple-capability-tenant" WITH CAPABILITIES]
----
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info true
can_view_tsdb_metrics false
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split true
can_admin_unsplit false
can_view_node_info true
can_view_tsdb_metrics false
exempt_from_rate_limiting false

statement ok
ALTER TENANT "multiple-capability-tenant" REVOKE CAPABILITY can_admin_split, can_view_node_info

query TT colnames
SELECT capability_name, capability_value FROM [SHOW TENANT "multiple-capability-tenant" WITH CAPABILITIES]
----
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false

statement ok
ALTER TENANT "multiple-capability-tenant" GRANT CAPABILITY exempt_from_rate_limiting

query TT colnames
SELECT capability_name, capability_value FROM [SHOW TENANT "multiple-capability-tenant" WITH CAPABILITIES]
----
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting true

statement ok
ALTER TENANT "multiple-capability-tenant" REVOKE CAPABILITY exempt_from_rate_limiting

query TT colnames
SELECT capability_name, capability_value FROM [SHOW TENANT "multiple-capability-tenant" WITH CAPABILITIES]
----
capability_name capability_value
can_admin_relocate_range false
can_admin_scatter true
can_admin_split false
can_admin_unsplit false
can_view_node_info false
can_view_tsdb_metrics false
exempt_from_rate_limiting false

subtest end
3 changes: 3 additions & 0 deletions pkg/kv/kvserver/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,8 @@ go_library(
"//pkg/kv/kvserver/txnwait",
"//pkg/kv/kvserver/uncertainty",
"//pkg/multitenant",
"//pkg/multitenant/tenantcapabilities",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer",
"//pkg/multitenant/tenantcostmodel",
"//pkg/roachpb",
"//pkg/rpc",
Expand Down Expand Up @@ -387,6 +389,7 @@ go_test(
"//pkg/kv/kvserver/tscache",
"//pkg/kv/kvserver/txnwait",
"//pkg/kv/kvserver/uncertainty",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer",
"//pkg/roachpb",
"//pkg/rpc",
"//pkg/rpc/nodedialer",
Expand Down
11 changes: 10 additions & 1 deletion pkg/kv/kvserver/store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ import (
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/tscache"
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/txnrecovery"
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/txnwait"
"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities"
"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/rpc"
"github.com/cockroachdb/cockroach/pkg/rpc/nodedialer"
Expand Down Expand Up @@ -1392,7 +1394,14 @@ func NewStore(
int(concurrentRangefeedItersLimit.Get(&cfg.Settings.SV)))
})

s.tenantRateLimiters = tenantrate.NewLimiterFactory(&cfg.Settings.SV, &cfg.TestingKnobs.TenantRateKnobs)
var authorizer tenantcapabilities.Authorizer
if cfg.RPCContext != nil && cfg.RPCContext.TenantRPCAuthorizer != nil {
authorizer = cfg.RPCContext.TenantRPCAuthorizer
} else {
authorizer = tenantcapabilitiesauthorizer.NewNoopAuthorizer()
}

s.tenantRateLimiters = tenantrate.NewLimiterFactory(&cfg.Settings.SV, &cfg.TestingKnobs.TenantRateKnobs, authorizer)
s.metrics.registry.AddMetricStruct(s.tenantRateLimiters.Metrics())

s.systemConfigUpdateQueueRateLimiter = quotapool.NewRateLimiter(
Expand Down
17 changes: 11 additions & 6 deletions pkg/kv/kvserver/store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/rditer"
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/stateloader"
"github.com/cockroachdb/cockroach/pkg/kv/kvserver/txnwait"
"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/rpc"
"github.com/cockroachdb/cockroach/pkg/rpc/nodedialer"
Expand Down Expand Up @@ -180,12 +181,13 @@ func createTestStoreWithoutStart(

rpcContext := rpc.NewContext(ctx,
rpc.ContextOptions{
TenantID: roachpb.SystemTenantID,
Config: &base.Config{Insecure: true},
Clock: cfg.Clock.WallClock(),
ToleratedOffset: cfg.Clock.ToleratedOffset(),
Stopper: stopper,
Settings: cfg.Settings,
TenantID: roachpb.SystemTenantID,
Config: &base.Config{Insecure: true},
Clock: cfg.Clock.WallClock(),
ToleratedOffset: cfg.Clock.ToleratedOffset(),
Stopper: stopper,
Settings: cfg.Settings,
TenantRPCAuthorizer: tenantcapabilitiesauthorizer.NewNoopAuthorizer(),
})
stopper.SetTracer(cfg.AmbientCtx.Tracer)
server, err := rpc.NewServer(rpcContext) // never started
Expand All @@ -201,6 +203,9 @@ func createTestStoreWithoutStart(
if cfg.StorePool == nil {
cfg.StorePool = NewTestStorePool(*cfg)
}
if cfg.RPCContext == nil {
cfg.RPCContext = rpcContext
}
// Many tests using this test harness (as opposed to higher-level
// ones like multiTestContext or TestServer) want to micro-manage
// replicas and the background queues just get in the way. The
Expand Down
5 changes: 5 additions & 0 deletions pkg/kv/kvserver/tenantrate/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/multitenant",
"//pkg/multitenant/tenantcapabilities",
"//pkg/multitenant/tenantcostmodel",
"//pkg/roachpb",
"//pkg/settings",
Expand All @@ -36,6 +37,10 @@ go_test(
data = glob(["testdata/**"]),
deps = [
":tenantrate",
"//pkg/kv/kvpb",
"//pkg/multitenant/tenantcapabilities",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer",
"//pkg/multitenant/tenantcapabilities/tenantcapabilitiespb",
"//pkg/multitenant/tenantcostmodel",
"//pkg/roachpb",
"//pkg/settings/cluster",
Expand Down
29 changes: 19 additions & 10 deletions pkg/kv/kvserver/tenantrate/factory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ package tenantrate
import (
"context"

"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/settings"
"github.com/cockroachdb/cockroach/pkg/util/log"
Expand All @@ -32,7 +33,9 @@ type LimiterFactory struct {
knobs TestingKnobs
metrics Metrics
systemLimiter systemLimiter
mu struct {
authorizer tenantcapabilities.Authorizer

mu struct {
syncutil.RWMutex
config Config
tenants map[roachpb.TenantID]*refCountedLimiter
Expand All @@ -46,9 +49,12 @@ type refCountedLimiter struct {
}

// NewLimiterFactory constructs a new LimiterFactory.
func NewLimiterFactory(sv *settings.Values, knobs *TestingKnobs) *LimiterFactory {
func NewLimiterFactory(
sv *settings.Values, knobs *TestingKnobs, authorizer tenantcapabilities.Authorizer,
) *LimiterFactory {
rl := &LimiterFactory{
metrics: makeMetrics(),
metrics: makeMetrics(),
authorizer: authorizer,
}
if knobs != nil {
rl.knobs = *knobs
Expand Down Expand Up @@ -92,8 +98,9 @@ func (rl *LimiterFactory) GetTenant(
if closer != nil {
options = append(options, quotapool.WithCloser(closer))
}

rcLim = new(refCountedLimiter)
rcLim.lim.init(rl, tenantID, rl.mu.config, rl.metrics.tenantMetrics(tenantID), options...)
rcLim.lim.init(rl, tenantID, rl.mu.config, rl.metrics.tenantMetrics(tenantID), rl.authorizer, options...)
rl.mu.tenants[tenantID] = rcLim
log.Infof(
ctx, "tenant %s rate limiter initialized (rate: %g RU/s; burst: %g RU)",
Expand All @@ -109,20 +116,22 @@ func (rl *LimiterFactory) Release(lim Limiter) {
if _, isSystem := lim.(*systemLimiter); isSystem {
return
}

l := lim.(*limiter)
tenID := l.TenantID()

rl.mu.Lock()
defer rl.mu.Unlock()
rcLim, ok := rl.mu.tenants[l.tenantID]
rcLim, ok := rl.mu.tenants[tenID]
if !ok {
panic(errors.AssertionFailedf("expected to find entry for tenant %v", l.tenantID))
panic(errors.AssertionFailedf("expected to find entry for tenant %v", tenID))
}
if &rcLim.lim != lim {
panic(errors.AssertionFailedf("two limiters exist for tenant %v", l.tenantID))
panic(errors.AssertionFailedf("two limiters exist for tenant %v", tenID))
}
if rcLim.refCount--; rcLim.refCount == 0 {
l.metrics.unlink()
l.qp.Close("released")
delete(rl.mu.tenants, l.tenantID)
l.Release()
delete(rl.mu.tenants, tenID)
}
}

Expand Down
Loading

0 comments on commit 4dc10b5

Please sign in to comment.