Skip to content

Commit

Permalink
sql: remove references to closed multi-tenancy issues
Browse files Browse the repository at this point in the history
This commit updates a few places to remove now-closed multi-tenancy
issues. Two of those places are gated behind the system tenant, so the
issue doesn't matter, and they now use existing issue (even though it
doesn't really relate to the features in question). In one place we
choose to return "tenant cluster setting" not enabled error to make it
more clear (I was just bitten by this because we returned opaque
"unimplemented" error). In two other places in comments we update the
references to existing issues tracking the corresponding work.

Release note: None
  • Loading branch information
yuzefovich committed Sep 27, 2023
1 parent 4b2badd commit 02d4fbb
Show file tree
Hide file tree
Showing 11 changed files with 21 additions and 30 deletions.
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# LogicTest: 3node-tenant
# tenant-cluster-setting-override-opt: sql.virtual_cluster.feature_access.zone_configs.enabled=false

statement error pq: unimplemented: operation is unsupported within a virtual cluster
statement error pq: operation is disabled within a virtual cluster\nHINT: Feature was disabled by the system operator.\nDETAIL: Feature flag: sql.virtual_cluster.feature_access.zone_configs.enabled
ALTER TABLE t CONFIGURE ZONE USING num_replicas = 5;

statement error setting sql.virtual_cluster.feature_access.zone_configs.enabled is only settable by the operator
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/interactive_tests/test_demo_partitioning.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ end_test
start_test "Expect an error if geo-partitioning is requested with multitenant mode"
send "$argv demo --no-line-editor --geo-partitioned-replicas --log-dir=logs \r"
# expect a failure
eexpect "operation is unsupported within a virtual cluster"
eexpect "operation is disabled within a virtual cluster"
eexpect $prompt
end_test

Expand Down
4 changes: 2 additions & 2 deletions pkg/cli/interactive_tests/test_disable_replication.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ send "PS1=':''/# '\r"
eexpect ":/# "

start_test "Check that demo disables replication properly"
# Disable multitenant until zone configs are properly enabled in tenants.
# See 67679 for more details.
# Disable multitenant until default zone config is properly propagated to
# virtual clusters. See #110003 for more details.
send "$argv demo --multitenant=false -e 'show zone configuration for range default'\r"
eexpect "num_replicas = 1"
eexpect ":/# "
Expand Down
6 changes: 2 additions & 4 deletions pkg/server/serverpb/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,8 @@ type TenantStatusServer interface {
// OptionalNodesStatusServer returns the wrapped NodesStatusServer, if it is
// available. If it is not, an error referring to the optionally supplied issues
// is returned.
func (s *OptionalNodesStatusServer) OptionalNodesStatusServer(
issue int,
) (NodesStatusServer, error) {
v, err := s.w.OptionalErr(issue)
func (s *OptionalNodesStatusServer) OptionalNodesStatusServer() (NodesStatusServer, error) {
v, err := s.w.OptionalErr(errorutil.FeatureNotAvailableToNonSystemTenantsIssue)
if err != nil {
return nil, err
}
Expand Down
7 changes: 2 additions & 5 deletions pkg/sql/crdb_internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/vtable"
"github.com/cockroachdb/cockroach/pkg/util/admission/admissionpb"
"github.com/cockroachdb/cockroach/pkg/util/duration"
"github.com/cockroachdb/cockroach/pkg/util/errorutil"
"github.com/cockroachdb/cockroach/pkg/util/json"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/mon"
Expand Down Expand Up @@ -5426,8 +5425,7 @@ CREATE TABLE crdb_internal.kv_node_status (
if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil {
return err
}
ss, err := p.extendedEvalCtx.NodesStatusServer.OptionalNodesStatusServer(
errorutil.FeatureNotAvailableToNonSystemTenantsIssue)
ss, err := p.extendedEvalCtx.NodesStatusServer.OptionalNodesStatusServer()
if err != nil {
return err
}
Expand Down Expand Up @@ -5540,8 +5538,7 @@ CREATE TABLE crdb_internal.kv_store_status (
if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil {
return err
}
ss, err := p.ExecCfg().NodesStatusServer.OptionalNodesStatusServer(
errorutil.FeatureNotAvailableToNonSystemTenantsIssue)
ss, err := p.ExecCfg().NodesStatusServer.OptionalNodesStatusServer()
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/distsql_plan_bulk.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (dsp *DistSQLPlanner) setupAllNodesPlanningSystem(
planCtx := dsp.NewPlanningCtxWithOracle(ctx, evalCtx, nil /* planner */, nil, /* txn */
DistributionTypeAlways, oracle, localityFilter)

ss, err := execCfg.NodesStatusServer.OptionalNodesStatusServer(47900)
ss, err := execCfg.NodesStatusServer.OptionalNodesStatusServer()
if err != nil {
return planCtx, []base.SQLInstanceID{dsp.gatewaySQLInstanceID}, nil //nolint:returnerrcheck
}
Expand Down
5 changes: 4 additions & 1 deletion pkg/sql/exec_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -3793,5 +3793,8 @@ func (cfg *ExecutorConfig) RequireSystemTenantOrClusterSetting(
if cfg.Codec.ForSystemTenant() || setting.Get(&cfg.Settings.SV) {
return nil
}
return errors.Newf("tenant cluster setting %s disabled", setting.Name())
return errors.WithDetailf(errors.WithHint(
errors.New("operation is disabled within a virtual cluster"),
"Feature was disabled by the system operator."),
"Feature flag: %s", setting.Name())
}
2 changes: 1 addition & 1 deletion pkg/sql/logictest/logictestbase/logictestbase.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ var LogicTestConfigs = []TestClusterConfig{
// Have to disable the default test tenant here as there are test run in
// this mode which try to modify zone configurations and we're more
// restrictive in the way we allow zone configs to be modified by
// secondary tenants. See #75569 for more info.
// secondary tenants. See #100787 for more info.
//
// TODO(#76378): We should review this choice. Zone configs have
// been supported for secondary tenants since v22.2.
Expand Down
8 changes: 4 additions & 4 deletions pkg/sql/multitenant_admin_function_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ func TestMultiTenantAdminFunction(t *testing.T) {
result: [][]string{{ignore, "/1", maxTimestamp}},
},
secondaryWithoutClusterSetting: tenantExpected{
errorMessage: "tenant cluster setting sql.virtual_cluster.feature_access.manual_range_split.enabled disabled",
errorMessage: "operation is disabled within a virtual cluster",
},
queryClusterSetting: sql.SecondaryTenantSplitAtEnabled,
setupCapability: bcap(tenantcapabilities.CanAdminSplit, false),
Expand All @@ -484,7 +484,7 @@ func TestMultiTenantAdminFunction(t *testing.T) {
result: [][]string{{"\xf0\x8a\x89", "/1", maxTimestamp}},
},
secondaryWithoutClusterSetting: tenantExpected{
errorMessage: "tenant cluster setting sql.virtual_cluster.feature_access.manual_range_split.enabled disabled",
errorMessage: "operation is disabled within a virtual cluster",
},
queryClusterSetting: sql.SecondaryTenantSplitAtEnabled,
setupCapability: bcap(tenantcapabilities.CanAdminSplit, false),
Expand Down Expand Up @@ -571,7 +571,7 @@ func TestMultiTenantAdminFunction(t *testing.T) {
result: [][]string{{ignore, ignore}},
},
secondaryWithoutClusterSetting: tenantExpected{
errorMessage: "tenant cluster setting sql.virtual_cluster.feature_access.manual_range_scatter.enabled disabled",
errorMessage: "operation is disabled within a virtual cluster",
},
secondaryWithoutCapability: tenantExpected{
errorMessage: `does not have capability "can_admin_scatter"`,
Expand All @@ -588,7 +588,7 @@ func TestMultiTenantAdminFunction(t *testing.T) {
result: [][]string{{"\xf0\x8a", "/Table/104/2"}},
},
secondaryWithoutClusterSetting: tenantExpected{
errorMessage: "tenant cluster setting sql.virtual_cluster.feature_access.manual_range_scatter.enabled disabled",
errorMessage: "operation is disabled within a virtual cluster",
},
queryClusterSetting: sql.SecondaryTenantScatterEnabled,
setupCapability: bcap(tenantcapabilities.CanAdminScatter, false),
Expand Down
11 changes: 2 additions & 9 deletions pkg/sql/set_zone_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/errorutil"
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
"github.com/cockroachdb/cockroach/pkg/util/log/logpb"
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
Expand Down Expand Up @@ -262,10 +261,7 @@ func (p *planner) SetZoneConfig(ctx context.Context, n *tree.SetZoneConfig) (pla
}

if err := execCfg.RequireSystemTenantOrClusterSetting(SecondaryTenantZoneConfigsEnabled); err != nil {
// Return an unimplemented error here instead of referencing the cluster
// setting here as zone configurations for secondary tenants are intended to
// be hidden.
return nil, errorutil.UnsupportedUnderClusterVirtualization(MultitenancyZoneCfgIssueNo)
return nil, err
}

if err := checkPrivilegeForSetZoneConfig(ctx, p, n.ZoneSpecifier); err != nil {
Expand Down Expand Up @@ -1013,7 +1009,7 @@ func validateZoneAttrsAndLocalities(
return nil
}
if execCfg.Codec.ForSystemTenant() {
ss, err := execCfg.NodesStatusServer.OptionalNodesStatusServer(MultitenancyZoneCfgIssueNo)
ss, err := execCfg.NodesStatusServer.OptionalNodesStatusServer()
if err != nil {
return err
}
Expand Down Expand Up @@ -1134,9 +1130,6 @@ func validateZoneLocalitiesForSecondaryTenants(
return nil
}

// MultitenancyZoneCfgIssueNo points to the multitenancy zone config issue number.
const MultitenancyZoneCfgIssueNo = 49854

type zoneConfigUpdate struct {
id descpb.ID
zoneConfig catalog.ZoneConfig
Expand Down
2 changes: 1 addition & 1 deletion pkg/workload/workloadsql/workloadsql.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ func Split(ctx context.Context, db *gosql.DB, table workload.Table, concurrency

// Test that we can actually perform a scatter.
if _, err := db.Exec("ALTER TABLE system.jobs SCATTER"); err != nil {
if strings.Contains(err.Error(), "tenant cluster setting sql.virtual_cluster.feature_access.manual_range_scatter.enabled disabled") {
if strings.Contains(err.Error(), "operation is disabled within a virtual cluster") {
log.Infof(ctx, `skipping workload splits; can't scatter on tenants'`)
//nolint:returnerrcheck
return nil
Expand Down

0 comments on commit 02d4fbb

Please sign in to comment.