From 587ffb38274f20682568a7f957ce19c66f004a27 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 5 Sep 2023 11:07:52 +0200 Subject: [PATCH 1/4] sqlutils: make failure errors on zone pb comparisons more readable Release note: None --- pkg/testutils/sqlutils/BUILD.bazel | 1 + pkg/testutils/sqlutils/zone.go | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/pkg/testutils/sqlutils/BUILD.bazel b/pkg/testutils/sqlutils/BUILD.bazel index 1c11e2c08f52..9f5752225be1 100644 --- a/pkg/testutils/sqlutils/BUILD.bazel +++ b/pkg/testutils/sqlutils/BUILD.bazel @@ -24,6 +24,7 @@ go_library( "//pkg/sql/lexbase", "//pkg/sql/parser", "//pkg/sql/pgwire/pgerror", + "//pkg/sql/protoreflect", "//pkg/sql/sem/catconstants", "//pkg/sql/sem/tree", "//pkg/testutils", diff --git a/pkg/testutils/sqlutils/zone.go b/pkg/testutils/sqlutils/zone.go index 0f9af3506e82..484eaba92a4c 100644 --- a/pkg/testutils/sqlutils/zone.go +++ b/pkg/testutils/sqlutils/zone.go @@ -17,7 +17,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/config/zonepb" "github.com/cockroachdb/cockroach/pkg/sql/lexbase" - "github.com/cockroachdb/cockroach/pkg/util/protoutil" + "github.com/cockroachdb/cockroach/pkg/sql/protoreflect" ) // ZoneRow represents a row returned by SHOW ZONE CONFIGURATION. @@ -27,13 +27,17 @@ type ZoneRow struct { } func (row ZoneRow) sqlRowString() ([]string, error) { - configProto, err := protoutil.Marshal(&row.Config) + // Make the JSON comparable with the output of crdb_internal.pb_to_json. + configJSON, err := protoreflect.MessageToJSON( + &row.Config, + protoreflect.FmtFlags{EmitDefaults: false, EmitRedacted: false}, + ) if err != nil { return nil, err } return []string{ fmt.Sprintf("%d", row.ID), - string(configProto), + configJSON.String(), }, nil } @@ -83,8 +87,8 @@ func VerifyZoneConfigForTarget(t testing.TB, sqlDB *SQLRunner, target string, ro t.Fatal(err) } sqlDB.CheckQueryResults(t, fmt.Sprintf(` -SELECT zone_id, raw_config_protobuf - FROM [SHOW ZONE CONFIGURATION FOR %s]`, target), +SELECT zone_id, crdb_internal.pb_to_json('cockroach.config.zonepb.ZoneConfig', raw_config_protobuf)::STRING +FROM [SHOW ZONE CONFIGURATION FOR %s]`, target), [][]string{sqlRow}) } @@ -100,7 +104,9 @@ func VerifyAllZoneConfigs(t testing.TB, sqlDB *SQLRunner, rows ...ZoneRow) { t.Fatal(err) } } - sqlDB.CheckQueryResults(t, `SELECT zone_id, raw_config_protobuf FROM crdb_internal.zones`, expected) + sqlDB.CheckQueryResults(t, ` +SELECT zone_id, crdb_internal.pb_to_json('cockroach.config.zonepb.ZoneConfig', raw_config_protobuf)::STRING +FROM crdb_internal.zones`, expected) } // ZoneConfigExists returns whether a zone config with the provided name exists. From fd6c05c7edfcb8e4b2f99f3c3b6c627ea6f0674b Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 5 Sep 2023 11:40:37 +0200 Subject: [PATCH 2/4] server: inherit default zone config from KV layer in TestServer Before this patch, the DefaultZoneConfig field was taken from the global default. This is incorrect. This patch fixes it. Note that this patch is not complete - we also need to update the non-test version of tenant configs, by communicating the default config from the KV layer to the SQL service over the network (using tenant connector). Release note: None --- pkg/server/server_controller_new_server.go | 1 + pkg/server/testserver.go | 1 + 2 files changed, 2 insertions(+) diff --git a/pkg/server/server_controller_new_server.go b/pkg/server/server_controller_new_server.go index 55f5d86dbef8..a56ba3522758 100644 --- a/pkg/server/server_controller_new_server.go +++ b/pkg/server/server_controller_new_server.go @@ -245,6 +245,7 @@ func makeSharedProcessTenantServerConfig( baseCfg.Locality = kvServerCfg.BaseConfig.Locality baseCfg.SpanConfigsDisabled = kvServerCfg.BaseConfig.SpanConfigsDisabled baseCfg.EnableDemoLoginEndpoint = kvServerCfg.BaseConfig.EnableDemoLoginEndpoint + baseCfg.DefaultZoneConfig = kvServerCfg.BaseConfig.DefaultZoneConfig // TODO(knz): use a single network interface for all tenant servers. // See: https://github.com/cockroachdb/cockroach/issues/92524 diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go index f2437b81e7ec..177d2eb3e07f 100644 --- a/pkg/server/testserver.go +++ b/pkg/server/testserver.go @@ -1476,6 +1476,7 @@ func (ts *testServer) StartTenant( baseCfg.StartDiagnosticsReporting = params.StartDiagnosticsReporting baseCfg.DisableTLSForHTTP = params.DisableTLSForHTTP baseCfg.EnableDemoLoginEndpoint = params.EnableDemoLoginEndpoint + baseCfg.DefaultZoneConfig = ts.Cfg.DefaultZoneConfig // Waiting for capabilities can time To avoid paying this cost in all // cases, we only set the nodelocal storage capability if the caller has From 2f8c7665052a92f67b1782f72f3e2a42fc2d9ab8 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 5 Sep 2023 11:43:45 +0200 Subject: [PATCH 3/4] partitionccl: fix a test Release note: None --- pkg/ccl/partitionccl/zone_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/ccl/partitionccl/zone_test.go b/pkg/ccl/partitionccl/zone_test.go index 213199c1fdad..dbbecbb680dc 100644 --- a/pkg/ccl/partitionccl/zone_test.go +++ b/pkg/ccl/partitionccl/zone_test.go @@ -386,11 +386,14 @@ func TestZoneConfigAppliesToTemporaryIndex(t *testing.T) { }, } - s, sqlDB, kvDB := serverutils.StartServer(t, params) - defer s.Stopper().Stop(context.Background()) + srv, sqlDB, kvDB := serverutils.StartServer(t, params) + defer srv.Stopper().Stop(context.Background()) + + s := srv.ApplicationLayer() + tdb := sqlutils.MakeSQLRunner(sqlDB) - codec := s.ApplicationLayer().Codec() - sv := &s.ApplicationLayer().ClusterSettings().SV + codec := s.Codec() + sv := &s.ClusterSettings().SV sql.SecondaryTenantZoneConfigsEnabled.Override(context.Background(), sv, true) if _, err := sqlDB.Exec(` From 547a0484dcc47485e72572454bdf81bb181ea65e Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 5 Sep 2023 12:15:06 +0200 Subject: [PATCH 4/4] externalconn/testutils: fix a mistake in `SetSQLDBForUser` Prior to this patch, this method was incorrectly swapping a SQL connection to the secondary tenant for one to the system tenant. This patch fixes it. Note: the problem was "invisible" because this incorrect redirect was the first thing that ever happened in any data file to a secondary tenant. So really all the "multi-tenant" testdata files were really exercising the system tenant. Release note: None --- pkg/cloud/externalconn/testutils/cluster.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cloud/externalconn/testutils/cluster.go b/pkg/cloud/externalconn/testutils/cluster.go index 408cd3b0df06..3cfc3d1f3bf3 100644 --- a/pkg/cloud/externalconn/testutils/cluster.go +++ b/pkg/cloud/externalconn/testutils/cluster.go @@ -58,7 +58,7 @@ func (h *Handle) SetSQLDBForUser(tenantID roachpb.TenantID, user string) func() return resetToRootUser } - userSQLDB := h.tc.Server(0).SQLConnForUser(h.t, user, "") + userSQLDB := tenantState.ApplicationLayerInterface.SQLConnForUser(h.t, user, "") tenantState.curDB = sqlutils.MakeSQLRunner(userSQLDB) tenantState.userToDB[user] = tenantState.curDB