From d13499401348ac45ddb19ceef17d96f71ae00785 Mon Sep 17 00:00:00 2001 From: Jeff Date: Tue, 27 Jun 2023 00:36:07 +0000 Subject: [PATCH 1/2] tenantcostclient: support changing ru settings Previously, there was no way to change the RU cost settings for a stand alone sql server. The setting values are stored in the kv server and initialized by the settings watcher in the sql server. The cost client only respected the values of settings at the time of creating the cost client, which happened as part of creating the kv client, which is required by the settings watcher to read the settings from the kv layer. So there is no way to initialize the cost client with the actual settings value. Now, the tenantcostclient will update the model whenever the value of settings change, including when the settings are first read during sql server startup. Part of: https://cockroachlabs.atlassian.net/browse/CC-7140 Release Note: None --- .../tenantcostclient/BUILD.bazel | 1 + .../tenantcostclient/tenant_side.go | 28 ++-- .../tenantcostclient/tenant_side_test.go | 133 ++++++++++++++++++ 3 files changed, 153 insertions(+), 9 deletions(-) diff --git a/pkg/ccl/multitenantccl/tenantcostclient/BUILD.bazel b/pkg/ccl/multitenantccl/tenantcostclient/BUILD.bazel index 54a5a4c3c3a2..b145f0329add 100644 --- a/pkg/ccl/multitenantccl/tenantcostclient/BUILD.bazel +++ b/pkg/ccl/multitenantccl/tenantcostclient/BUILD.bazel @@ -71,6 +71,7 @@ go_test( "//pkg/security/securitytest", "//pkg/security/username", "//pkg/server", + "//pkg/settings", "//pkg/settings/cluster", "//pkg/sql", "//pkg/sql/catalog/systemschema", diff --git a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go index 6fc67e2de6c7..2a19528d6764 100644 --- a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go +++ b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go @@ -10,6 +10,7 @@ package tenantcostclient import ( "context" + "sync/atomic" "time" "github.com/cockroachdb/cockroach/pkg/base" @@ -180,7 +181,13 @@ func newTenantSideCostController( NewRate: initialRate, }) - c.costCfg = tenantcostmodel.ConfigFromSettings(&st.SV) + tenantcostmodel.SetOnChange(&st.SV, func(ctx context.Context) { + config := tenantcostmodel.ConfigFromSettings(&st.SV) + c.costCfg.Swap(&config) + }) + initialConfig := tenantcostmodel.ConfigFromSettings(&st.SV) + c.costCfg.CompareAndSwap(nil, &initialConfig) + c.modeMu.externalIORUAccountingMode = externalIORUAccountingModeFromString(ExternalIORUAccountingMode.Get(&st.SV)) ExternalIORUAccountingMode.SetOnChange(&st.SV, func(context.Context) { c.modeMu.Lock() @@ -243,7 +250,7 @@ type tenantSideCostController struct { timeSource timeutil.TimeSource testInstr TestInstrumentation settings *cluster.Settings - costCfg tenantcostmodel.Config + costCfg atomic.Pointer[tenantcostmodel.Config] tenantID roachpb.TenantID provider kvtenant.TokenBucketProvider limiter limiter @@ -428,12 +435,13 @@ func (c *tenantSideCostController) onTick(ctx context.Context, newTime time.Time deltaCPU = 0 } - ru := c.costCfg.PodCPUCost(deltaCPU) + costCfg := c.costCfg.Load() + ru := costCfg.PodCPUCost(deltaCPU) var deltaPGWireEgressBytes uint64 if newExternalUsage.PGWireEgressBytes > c.run.externalUsage.PGWireEgressBytes { deltaPGWireEgressBytes = newExternalUsage.PGWireEgressBytes - c.run.externalUsage.PGWireEgressBytes - ru += c.costCfg.PGWireEgressCost(int64(deltaPGWireEgressBytes)) + ru += costCfg.PGWireEgressCost(int64(deltaPGWireEgressBytes)) } // KV RUs are not included here, these metrics correspond only to the SQL pod. @@ -779,8 +787,9 @@ func (c *tenantSideCostController) OnResponseWait( } // Account for the cost of write requests and read responses. - writeRU := c.costCfg.RequestCost(req) - readRU := c.costCfg.ResponseCost(resp) + costCfg := c.costCfg.Load() + writeRU := costCfg.RequestCost(req) + readRU := costCfg.ResponseCost(resp) totalRU := writeRU + readRU // TODO(andyk): Consider breaking up huge acquisition requests into chunks @@ -859,8 +868,9 @@ func (c *tenantSideCostController) onExternalIO( return nil } - totalRU := c.costCfg.ExternalIOIngressCost(usage.IngressBytes) + - c.costCfg.ExternalIOEgressCost(usage.EgressBytes) + costCfg := c.costCfg.Load() + totalRU := costCfg.ExternalIOIngressCost(usage.IngressBytes) + + costCfg.ExternalIOEgressCost(usage.EgressBytes) if wait { if err := c.limiter.Wait(ctx, totalRU); err != nil { @@ -891,5 +901,5 @@ func (c *tenantSideCostController) GetCPUMovingAvg() float64 { // GetCostConfig is part of the multitenant.TenantSideCostController interface. func (c *tenantSideCostController) GetCostConfig() *tenantcostmodel.Config { - return &c.costCfg + return c.costCfg.Load() } diff --git a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go index 7e6962f80964..70338c0da274 100644 --- a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go +++ b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go @@ -39,6 +39,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcostmodel" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/username" + "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" @@ -1404,3 +1405,135 @@ func BenchmarkExternalIOAccounting(b *testing.B) { }) } } + +func TestRUSettingsChanged(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + ctx := context.Background() + + params := base.TestServerArgs{ + DefaultTestTenant: base.TestTenantDisabled, + } + + s, mainDB, _ := serverutils.StartServer(t, params) + defer s.Stopper().Stop(ctx) + sysDB := sqlutils.MakeSQLRunner(mainDB) + + tenantID := serverutils.TestTenantID() + tenant1, tenantDB1 := serverutils.StartTenant(t, s, base.TestTenantArgs{ + TenantID: tenantID, + }) + defer tenant1.Stopper().Stop(ctx) + defer tenantDB1.Close() + + costClient, err := tenantcostclient.NewTenantSideCostController(tenant1.ClusterSettings(), tenantID, nil) + require.NoError(t, err) + + initialModel := costClient.GetCostConfig() + + // Increase the RU cost of everything by 100x + settings := []*settings.FloatSetting{ + tenantcostmodel.ReadBatchCost, + tenantcostmodel.ReadRequestCost, + tenantcostmodel.ReadPayloadCostPerMiB, + tenantcostmodel.WriteBatchCost, + tenantcostmodel.WriteRequestCost, + tenantcostmodel.WritePayloadCostPerMiB, + tenantcostmodel.SQLCPUSecondCost, + tenantcostmodel.PgwireEgressCostPerMiB, + tenantcostmodel.ExternalIOEgressCostPerMiB, + tenantcostmodel.ExternalIOIngressCostPerMiB, + } + for _, setting := range settings { + sysDB.Exec(t, fmt.Sprintf("ALTER TENANT ALL SET CLUSTER SETTING %s = $1", setting.Key()), setting.Default()*100) + } + + // Check to make sure the cost of the query increased. Use SucceedsSoon + // because the settings propogation is async. + testutils.SucceedsSoon(t, func() error { + currentModel := costClient.GetCostConfig() + + expect100x := func(name string, getter func(model *tenantcostmodel.Config) tenantcostmodel.RU) error { + before := getter(initialModel) + after := getter(currentModel) + expect := before * 100 + if after != expect { + return errors.Newf("expected %s to be %f found %f", name, expect, after) + } + return nil + } + + err = expect100x("KVReadBatch", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.KVReadBatch + }) + if err != nil { + return err + } + + err = expect100x("KVReadRequest", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.KVReadRequest + }) + if err != nil { + return err + } + + err = expect100x("KVReadByte", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.KVReadByte + }) + if err != nil { + return err + } + + err = expect100x("KVWriteBatch", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.KVWriteBatch + }) + if err != nil { + return err + } + + err = expect100x("KVWriteRequest", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.KVWriteRequest + }) + if err != nil { + return err + } + + err = expect100x("KVWriteByte", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.KVWriteByte + }) + if err != nil { + return err + } + + err = expect100x("PodCPUSecond", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.PodCPUSecond + }) + if err != nil { + return err + } + + err = expect100x("PGWireEgressByte", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.PGWireEgressByte + }) + if err != nil { + return err + } + + err = expect100x("ExternalIOEgressByte", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.ExternalIOEgressByte + }) + if err != nil { + return err + } + + err = expect100x("ExternalIOIngressByte", func(m *tenantcostmodel.Config) tenantcostmodel.RU { + return m.ExternalIOIngressByte + }) + if err != nil { + return err + } + + return nil + }) +} From 4b0eb97b38e9e4f79451d359b07028b505804464 Mon Sep 17 00:00:00 2001 From: Andy Yang Date: Tue, 11 Jul 2023 14:37:26 -0700 Subject: [PATCH 2/2] sql/sem/builtins: deflake TestSerialNormalizationWithUniqueUnorderedID This patch increases the sample count for a probabilistic test that tests for uniform distribution so that it is less likely to flake. Release note: None --- pkg/sql/sem/builtins/builtins_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/sql/sem/builtins/builtins_test.go b/pkg/sql/sem/builtins/builtins_test.go index e534e2ee8512..70e639116d3f 100644 --- a/pkg/sql/sem/builtins/builtins_test.go +++ b/pkg/sql/sem/builtins/builtins_test.go @@ -124,7 +124,7 @@ CREATE TABLE t ( j INT )`) - numberOfRows := 10000 + numberOfRows := 100000 // Insert rows. tdb.Exec(t, fmt.Sprintf(`