From b10a600409cf426e19075f785564aef5041c2e3d Mon Sep 17 00:00:00 2001 From: Andrew Werner Date: Wed, 9 Nov 2022 03:26:46 +0000 Subject: [PATCH] server: remove vestigial tenant-only testing knob This knob was being used by default to subvert the settings infrastructure in tenants on the local node. This lead to hazardous interactions with the settingswatcher behavior. That library tries quite hard to synchronous updates to settings and ensure that they do not regress. By setting the setting above that layer, we could very much see them regress. As far as I can tell, this code came about before tenants could actually manage settings for themselves. In practice, this code would run prior to the transaction writing the setting running, which generally meant that so long as you didn't flip settings back and forth, things would work out. Nevertheless, it was tech debt and is now removed. Fixes #87017 Informs #87201 Release note: None --- pkg/base/test_server_args.go | 4 --- .../tenantcostclient/tenant_side_test.go | 32 ++++++++----------- .../serverccl/diagnosticsccl/reporter_test.go | 5 ++- pkg/ccl/serverccl/server_sql_test.go | 4 +-- pkg/server/testserver.go | 31 ++++++------------ pkg/sql/exec_util.go | 4 --- pkg/sql/logictest/logic.go | 3 +- pkg/sql/set_cluster_setting.go | 9 ------ pkg/sql/sqltestutils/telemetry.go | 5 ++- 9 files changed, 30 insertions(+), 67 deletions(-) diff --git a/pkg/base/test_server_args.go b/pkg/base/test_server_args.go index 80a658673559..d5e95809490c 100644 --- a/pkg/base/test_server_args.go +++ b/pkg/base/test_server_args.go @@ -243,10 +243,6 @@ type TestTenantArgs struct { // tenant cluster. Settings *cluster.Settings - // AllowSettingClusterSettings, if true, allows the tenant to set in-memory - // cluster settings. - AllowSettingClusterSettings bool - // Stopper, if not nil, is used to stop the tenant manually otherwise the // TestServer stopper will be used. Stopper *stop.Stopper diff --git a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go index be1618c69f37..dd65cb135e27 100644 --- a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go +++ b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go @@ -855,9 +855,8 @@ func TestConsumption(t *testing.T) { testProvider := newTestProvider() _, tenantDB := serverutils.StartTenant(t, hostServer, base.TestTenantArgs{ - TenantID: serverutils.TestTenantID(), - Settings: st, - AllowSettingClusterSettings: true, + TenantID: serverutils.TestTenantID(), + Settings: st, TestingKnobs: base.TestingKnobs{ TenantTestingKnobs: &sql.TenantTestingKnobs{ OverrideTokenBucketProvider: func(kvtenant.TokenBucketProvider) kvtenant.TokenBucketProvider { @@ -932,9 +931,8 @@ func TestSQLLivenessExemption(t *testing.T) { slinstance.DefaultHeartBeat.Override(ctx, &st.SV, 10*time.Millisecond) _, tenantDB := serverutils.StartTenant(t, hostServer, base.TestTenantArgs{ - TenantID: tenantID, - Settings: st, - AllowSettingClusterSettings: true, + TenantID: tenantID, + Settings: st, }) r := sqlutils.MakeSQLRunner(tenantDB) @@ -994,9 +992,8 @@ func TestScheduledJobsConsumption(t *testing.T) { var tenantServer serverutils.TestTenantInterface var tenantDB *gosql.DB tenantServer, tenantDB = serverutils.StartTenant(t, hostServer, base.TestTenantArgs{ - TenantID: serverutils.TestTenantID(), - Settings: st, - AllowSettingClusterSettings: true, + TenantID: serverutils.TestTenantID(), + Settings: st, TestingKnobs: base.TestingKnobs{ TenantTestingKnobs: &sql.TenantTestingKnobs{ OverrideTokenBucketProvider: func(kvtenant.TokenBucketProvider) kvtenant.TokenBucketProvider { @@ -1081,9 +1078,8 @@ func TestConsumptionChangefeeds(t *testing.T) { testProvider := newTestProvider() _, tenantDB := serverutils.StartTenant(t, hostServer, base.TestTenantArgs{ - TenantID: serverutils.TestTenantID(), - Settings: st, - AllowSettingClusterSettings: true, + TenantID: serverutils.TestTenantID(), + Settings: st, TestingKnobs: base.TestingKnobs{ TenantTestingKnobs: &sql.TenantTestingKnobs{ OverrideTokenBucketProvider: func(kvtenant.TokenBucketProvider) kvtenant.TokenBucketProvider { @@ -1153,10 +1149,9 @@ func TestConsumptionExternalStorage(t *testing.T) { testProvider := newTestProvider() _, tenantDB := serverutils.StartTenant(t, hostServer, base.TestTenantArgs{ - TenantID: serverutils.TestTenantID(), - Settings: st, - AllowSettingClusterSettings: true, - ExternalIODir: dir, + TenantID: serverutils.TestTenantID(), + Settings: st, + ExternalIODir: dir, TestingKnobs: base.TestingKnobs{ Server: &server.TestingKnobs{ BlobClientFactory: blobClientFactory, @@ -1260,9 +1255,8 @@ func BenchmarkExternalIOAccounting(b *testing.B) { st := cluster.MakeTestingClusterSettings() tenantcostmodel.ExternalIOEgressCostPerMiB.Override(context.Background(), &st.SV, 0.0) tenantS, _ := serverutils.StartTenant(b, hostServer, base.TestTenantArgs{ - TenantID: serverutils.TestTenantID(), - Settings: st, - AllowSettingClusterSettings: true, + TenantID: serverutils.TestTenantID(), + Settings: st, }) nullsink.NullRequiresExternalIOAccounting = true diff --git a/pkg/ccl/serverccl/diagnosticsccl/reporter_test.go b/pkg/ccl/serverccl/diagnosticsccl/reporter_test.go index f9ddffac5b38..f02b64ae0cbd 100644 --- a/pkg/ccl/serverccl/diagnosticsccl/reporter_test.go +++ b/pkg/ccl/serverccl/diagnosticsccl/reporter_test.go @@ -53,9 +53,8 @@ func TestTenantReport(t *testing.T) { defer rt.Close() tenantArgs := base.TestTenantArgs{ - TenantID: serverutils.TestTenantID(), - AllowSettingClusterSettings: true, - TestingKnobs: rt.testingKnobs, + TenantID: serverutils.TestTenantID(), + TestingKnobs: rt.testingKnobs, } tenant, tenantDB := serverutils.StartTenant(t, rt.server, tenantArgs) reporter := tenant.DiagnosticsReporter().(*diagnostics.Reporter) diff --git a/pkg/ccl/serverccl/server_sql_test.go b/pkg/ccl/serverccl/server_sql_test.go index 018689a2ccba..9a408dc4b210 100644 --- a/pkg/ccl/serverccl/server_sql_test.go +++ b/pkg/ccl/serverccl/server_sql_test.go @@ -82,7 +82,7 @@ func TestTenantCannotSetClusterSetting(t *testing.T) { defer tc.Stopper().Stop(ctx) // StartTenant with the default permissions to - _, db := serverutils.StartTenant(t, tc.Server(0), base.TestTenantArgs{TenantID: serverutils.TestTenantID(), AllowSettingClusterSettings: false}) + _, db := serverutils.StartTenant(t, tc.Server(0), base.TestTenantArgs{TenantID: serverutils.TestTenantID()}) defer db.Close() _, err := db.Exec(`SET CLUSTER SETTING sql.defaults.vectorize=off`) require.NoError(t, err) @@ -109,7 +109,7 @@ func TestTenantCanUseEnterpriseFeatures(t *testing.T) { tc := serverutils.StartNewTestCluster(t, 1, base.TestClusterArgs{}) defer tc.Stopper().Stop(context.Background()) - _, db := serverutils.StartTenant(t, tc.Server(0), base.TestTenantArgs{TenantID: serverutils.TestTenantID(), AllowSettingClusterSettings: false}) + _, db := serverutils.StartTenant(t, tc.Server(0), base.TestTenantArgs{TenantID: serverutils.TestTenantID()}) defer db.Close() _, err := db.Exec(`BACKUP INTO 'userfile:///backup'`) diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go index 5166cdbd421f..42c1c4e190af 100644 --- a/pkg/server/testserver.go +++ b/pkg/server/testserver.go @@ -533,17 +533,16 @@ func (ts *TestServer) maybeStartDefaultTestTenant(ctx context.Context) error { params := base.TestTenantArgs{ // Currently, all the servers leverage the same tenant ID. We may // want to change this down the road, for more elaborate testing. - TenantID: serverutils.TestTenantID(), - MemoryPoolSize: ts.params.SQLMemoryPoolSize, - TempStorageConfig: &tempStorageConfig, - Locality: ts.params.Locality, - ExternalIODir: ts.params.ExternalIODir, - ExternalIODirConfig: ts.params.ExternalIODirConfig, - ForceInsecure: ts.Insecure(), - UseDatabase: ts.params.UseDatabase, - SSLCertsDir: ts.params.SSLCertsDir, - AllowSettingClusterSettings: true, - TestingKnobs: ts.params.Knobs, + TenantID: serverutils.TestTenantID(), + MemoryPoolSize: ts.params.SQLMemoryPoolSize, + TempStorageConfig: &tempStorageConfig, + Locality: ts.params.Locality, + ExternalIODir: ts.params.ExternalIODir, + ExternalIODirConfig: ts.params.ExternalIODirConfig, + ForceInsecure: ts.Insecure(), + UseDatabase: ts.params.UseDatabase, + SSLCertsDir: ts.params.SSLCertsDir, + TestingKnobs: ts.params.Knobs, } // Since we're creating a tenant, it doesn't make sense to pass through the @@ -895,16 +894,6 @@ func (ts *TestServer) StartTenant( baseCfg.HTTPAddr = newAddr baseCfg.HTTPAdvertiseAddr = newAddr } - if params.AllowSettingClusterSettings { - tenantKnobs, ok := baseCfg.TestingKnobs.TenantTestingKnobs.(*sql.TenantTestingKnobs) - if !ok { - tenantKnobs = &sql.TenantTestingKnobs{} - baseCfg.TestingKnobs.TenantTestingKnobs = tenantKnobs - } - if tenantKnobs.ClusterSettingsUpdater == nil { - tenantKnobs.ClusterSettingsUpdater = st.MakeUpdater() - } - } sw, err := NewTenantServer( ctx, stopper, diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 11f4776feeeb..70f1d6894d62 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -1522,10 +1522,6 @@ func (*PGWireTestingKnobs) ModuleTestingKnobs() {} // TenantTestingKnobs contains knobs for tenant behavior. type TenantTestingKnobs struct { - // ClusterSettingsUpdater is a field that if set, allows the tenant to set - // in-memory cluster settings. SQL tenants are otherwise prohibited from - // setting cluster settings. - ClusterSettingsUpdater settings.Updater // TenantIDCodecOverride overrides the tenant ID used to construct the SQL // server's codec, but nothing else (e.g. its certs). diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index 6ee73ec9a13a..b377d40dfc4b 100644 --- a/pkg/sql/logictest/logic.go +++ b/pkg/sql/logictest/logic.go @@ -1377,8 +1377,7 @@ func (t *logicTest) newCluster( t.tenantAddrs = make([]string, cfg.NumNodes) for i := 0; i < cfg.NumNodes; i++ { tenantArgs := base.TestTenantArgs{ - TenantID: serverutils.TestTenantID(), - AllowSettingClusterSettings: true, + TenantID: serverutils.TestTenantID(), TestingKnobs: base.TestingKnobs{ SQLExecutor: &sql.ExecutorTestingKnobs{ DeterministicExplain: true, diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index 11b88f7f2a28..1815b908d129 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -434,15 +434,6 @@ func writeNonDefaultSettingValue( } } - if knobs := execCfg.TenantTestingKnobs; knobs != nil && knobs.ClusterSettingsUpdater != nil { - encVal := settings.EncodedValue{ - Value: encoded, - Type: setting.Typ(), - } - if err := execCfg.TenantTestingKnobs.ClusterSettingsUpdater.Set(ctx, name, encVal); err != nil { - return reportedValue, expectedEncodedValue, err - } - } return reportedValue, expectedEncodedValue, nil } diff --git a/pkg/sql/sqltestutils/telemetry.go b/pkg/sql/sqltestutils/telemetry.go index 59153767f880..df75e7db47a3 100644 --- a/pkg/sql/sqltestutils/telemetry.go +++ b/pkg/sql/sqltestutils/telemetry.go @@ -175,9 +175,8 @@ func (tt *telemetryTest) Start(t *testing.T, serverArgs []base.TestServerArgs) { tt.prepareCluster(tt.serverDB) tt.tenant, tt.tenantDB = serverutils.StartTenant(tt.t, tt.server, base.TestTenantArgs{ - TenantID: serverutils.TestTenantID(), - AllowSettingClusterSettings: true, - TestingKnobs: mapServerArgs[0].Knobs, + TenantID: serverutils.TestTenantID(), + TestingKnobs: mapServerArgs[0].Knobs, }) tt.prepareCluster(tt.tenantDB) }