Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: remove references to closed multi-tenancy issues #111321

Merged
merged 1 commit into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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