From 96175f14ca8a8b66ed1d7cad1b8d368b88b4fd66 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Tue, 21 Mar 2023 11:47:58 +0000 Subject: [PATCH 1/2] server: use no-op cost controller for shared-process tenants In the long run, all rate limiting of tenants should be controlled by a tenant capability. However, at the moment, we do not have the infrastructure to read tenant capabilities from the tenant process. Since, the main user of shared-process tenants is the new, experimental unified-architecture were we do not anticipate needing the cost controller for most users, this PR injects a no-op cost controller for shared-process tenants. Epic: CRDB-23559 Release note: None --- pkg/server/tenant.go | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/pkg/server/tenant.go b/pkg/server/tenant.go index 22c6bfc40786..e25d2b87e094 100644 --- a/pkg/server/tenant.go +++ b/pkg/server/tenant.go @@ -150,6 +150,8 @@ func (s *SQLServerWrapper) Drain( return s.drainServer.runDrain(ctx, verbose) } +type costControllerFactory func(*cluster.Settings, roachpb.TenantID, kvtenant.TokenBucketProvider) (multitenant.TenantSideCostController, error) + // NewSeparateProcessTenantServer creates a tenant-specific, SQL-only // server against a KV backend, with defaults appropriate for a // SQLServer that is not located in the same process as a KVServer. @@ -164,7 +166,8 @@ func NewSeparateProcessTenantServer( tenantNameContainer *roachpb.TenantNameContainer, ) (*SQLServerWrapper, error) { instanceIDContainer := baseCfg.IDContainer.SwitchToSQLIDContainerForStandaloneSQLInstance() - return newTenantServer(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, instanceIDContainer) + costControllerBuilder := NewTenantSideCostController + return newTenantServer(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, instanceIDContainer, costControllerBuilder) } // NewSharedProcessTenantServer creates a tenant-specific, SQL-only @@ -184,7 +187,15 @@ func NewSharedProcessTenantServer( return nil, errors.AssertionFailedf("programming error: NewSharedProcessTenantServer called before NodeID was assigned.") } instanceIDContainer := base.NewSQLIDContainerForNode(baseCfg.IDContainer) - return newTenantServer(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, instanceIDContainer) + // TODO(ssd): The cost controller should instead be able to + // read from the capability system and return immediately if + // the tenant is exempt. For now we are turning off the + // tenant-side cost controller for shared-memory tenants until + // we have the abilility to read capabilities tenant-side. + // + // https://github.com/cockroachdb/cockroach/issues/84586 + costControllerBuilder := NewNoopTenantSideCostController + return newTenantServer(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, instanceIDContainer, costControllerBuilder) } // newTenantServer constructs a SQLServerWrapper. @@ -197,6 +208,7 @@ func newTenantServer( sqlCfg SQLConfig, tenantNameContainer *roachpb.TenantNameContainer, instanceIDContainer *base.SQLIDContainer, + costControllerFactory costControllerFactory, ) (*SQLServerWrapper, error) { // TODO(knz): Make the license application a per-server thing // instead of a global thing. @@ -208,7 +220,7 @@ func newTenantServer( // Inform the server identity provider that we're operating // for a tenant server. baseCfg.idProvider.SetTenant(sqlCfg.TenantID) - args, err := makeTenantSQLServerArgs(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, instanceIDContainer) + args, err := makeTenantSQLServerArgs(ctx, stopper, baseCfg, sqlCfg, tenantNameContainer, instanceIDContainer, costControllerFactory) if err != nil { return nil, err } @@ -894,6 +906,7 @@ func makeTenantSQLServerArgs( sqlCfg SQLConfig, tenantNameContainer *roachpb.TenantNameContainer, instanceIDContainer *base.SQLIDContainer, + costControllerFactory costControllerFactory, ) (sqlServerArgs, error) { st := baseCfg.Settings @@ -986,7 +999,7 @@ func makeTenantSQLServerArgs( tenantKnobs.OverrideTokenBucketProvider != nil { provider = tenantKnobs.OverrideTokenBucketProvider(provider) } - costController, err := NewTenantSideCostController(st, sqlCfg.TenantID, provider) + costController, err := costControllerFactory(st, sqlCfg.TenantID, provider) if err != nil { return sqlServerArgs{}, err } @@ -1218,8 +1231,12 @@ func makeNextLiveInstanceIDFn( // NewTenantSideCostController is a hook for CCL code which implements the // controller. -var NewTenantSideCostController = func( - st *cluster.Settings, tenantID roachpb.TenantID, provider kvtenant.TokenBucketProvider, +var NewTenantSideCostController costControllerFactory = NewNoopTenantSideCostController + +// NewNoopTenantSideCostController returns a noop cost +// controller. Used by shared-process tenants. +func NewNoopTenantSideCostController( + *cluster.Settings, roachpb.TenantID, kvtenant.TokenBucketProvider, ) (multitenant.TenantSideCostController, error) { // Return a no-op implementation. return noopTenantSideCostController{}, nil From 9e05d1716d8f157f9b72d28ec7b6fd4d7e4c8267 Mon Sep 17 00:00:00 2001 From: Steven Danna Date: Tue, 21 Mar 2023 11:50:52 +0000 Subject: [PATCH 2/2] bench: exempt shared process tenants from rate limits This exempts the tenant from the KV-side tenant limiter. We expect this to be the default configuration of most shared-process tenants in the near term. Thus, setting this during benchmark setups provides more accurate benchmarks. Epic: none Release note: None --- pkg/bench/BUILD.bazel | 3 +++ pkg/bench/foreachdb.go | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/pkg/bench/BUILD.bazel b/pkg/bench/BUILD.bazel index 874ac8491284..5feb15dac094 100644 --- a/pkg/bench/BUILD.bazel +++ b/pkg/bench/BUILD.bazel @@ -13,12 +13,15 @@ go_library( deps = [ "//pkg/base", "//pkg/ccl", + "//pkg/multitenant/tenantcapabilities", "//pkg/roachpb", "//pkg/server", + "//pkg/testutils", "//pkg/testutils/serverutils", "//pkg/testutils/skip", "//pkg/testutils/sqlutils", "//pkg/testutils/testcluster", + "@com_github_cockroachdb_errors//:errors", "@com_github_go_sql_driver_mysql//:mysql", "@com_github_lib_pq//:pq", "@com_github_stretchr_testify//require", diff --git a/pkg/bench/foreachdb.go b/pkg/bench/foreachdb.go index efcbf8c8a675..6364dcc58362 100644 --- a/pkg/bench/foreachdb.go +++ b/pkg/bench/foreachdb.go @@ -23,12 +23,15 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" _ "github.com/cockroachdb/cockroach/pkg/ccl" + "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server" + "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" + "github.com/cockroachdb/errors" _ "github.com/go-sql-driver/mysql" // registers the MySQL driver to gosql _ "github.com/lib/pq" // registers the pg driver to gosql "github.com/stretchr/testify/require" @@ -78,6 +81,27 @@ func benchmarkSharedProcessTenantCockroach(b *testing.B, f BenchmarkFn) { _, err = db.Exec(`ALTER TENANT ALL SET CLUSTER SETTING "spanconfig.tenant_limit" = 10000000`) require.NoError(b, err) + // Exempt the tenant from rate limiting. We expect most + // shared-process tenants will run without rate limiting in + // the near term. + _, err = db.Exec(`ALTER TENANT benchtenant GRANT CAPABILITY exempt_from_rate_limiting`) + require.NoError(b, err) + + var tenantID uint64 + require.NoError(b, db.QueryRow(`SELECT id FROM [SHOW TENANT benchtenant]`).Scan(&tenantID)) + + err = testutils.SucceedsSoonError(func() error { + capabilities, found := s.(*server.TestServer).Server.TenantCapabilitiesReader().GetCapabilities(roachpb.MustMakeTenantID(tenantID)) + if !found { + return errors.Newf("capabilities not yet ready") + } + if !capabilities.GetBool(tenantcapabilities.ExemptFromRateLimiting) { + return errors.Newf("capabilities not yet ready") + } + return nil + }) + require.NoError(b, err) + _, err = tenantDB.Exec(`CREATE DATABASE bench`) require.NoError(b, err)