From e53b17af66f158e5ba6412b6913b9b67616f633b Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 11 Sep 2020 12:40:00 +0200 Subject: [PATCH 1/2] *: make multi-tenancy issue numbers non-optional There was an issue number missing in a few places, because the argument list made it optional. This patch fixes it and links too the relevant issues. Release note: None --- pkg/base/node_id.go | 4 ++-- pkg/gossip/gossip.go | 6 +++--- pkg/jobs/deprecated.go | 2 +- pkg/jobs/registry.go | 2 +- pkg/server/serverpb/status.go | 4 ++-- pkg/sql/crdb_internal.go | 7 +++++-- pkg/sql/optionalnodeliveness/node_liveness.go | 6 +++--- pkg/sql/relocate.go | 2 +- pkg/sql/set_zone_config.go | 2 +- pkg/util/errorutil/tenant_deprecated_wrapper.go | 9 +++++++-- 10 files changed, 26 insertions(+), 18 deletions(-) diff --git a/pkg/base/node_id.go b/pkg/base/node_id.go index aac0f362f5db..93a74034890c 100644 --- a/pkg/base/node_id.go +++ b/pkg/base/node_id.go @@ -124,8 +124,8 @@ func (c *SQLIDContainer) OptionalNodeID() (roachpb.NodeID, bool) { // OptionalNodeIDErr is like OptionalNodeID, but returns an error (referring to // the optionally supplied Github issues) if the ID is not present. -func (c *SQLIDContainer) OptionalNodeIDErr(issueNos ...int) (roachpb.NodeID, error) { - v, err := c.w.OptionalErr(issueNos...) +func (c *SQLIDContainer) OptionalNodeIDErr(issue int) (roachpb.NodeID, error) { + v, err := c.w.OptionalErr(issue) if err != nil { return 0, err } diff --git a/pkg/gossip/gossip.go b/pkg/gossip/gossip.go index 8748a95e668c..277ba8486c1c 100644 --- a/pkg/gossip/gossip.go +++ b/pkg/gossip/gossip.go @@ -1646,8 +1646,8 @@ type OptionalGossip struct { // // Use of Gossip from within the SQL layer is **deprecated**. Please do not // introduce new uses of it. -func (og OptionalGossip) OptionalErr(issueNos ...int) (*Gossip, error) { - v, err := og.w.OptionalErr(issueNos...) +func (og OptionalGossip) OptionalErr(issue int) (*Gossip, error) { + v, err := og.w.OptionalErr(issue) if err != nil { return nil, err } @@ -1660,7 +1660,7 @@ func (og OptionalGossip) OptionalErr(issueNos ...int) (*Gossip, error) { // // Use of Gossip from within the SQL layer is **deprecated**. Please do not // introduce new uses of it. -func (og OptionalGossip) Optional(issueNos ...int) (*Gossip, bool) { +func (og OptionalGossip) Optional(issue int) (*Gossip, bool) { v, ok := og.w.Optional() if !ok { return nil, false diff --git a/pkg/jobs/deprecated.go b/pkg/jobs/deprecated.go index 3962f263da86..8bec0fdf191c 100644 --- a/pkg/jobs/deprecated.go +++ b/pkg/jobs/deprecated.go @@ -86,7 +86,7 @@ WHERE status IN ($1, $2, $3, $4, $5) ORDER BY created DESC` // If no liveness is available, adopt all jobs. This is reasonable because this // only affects SQL tenants, which have at most one SQL server running on their // behalf at any given time. - if nl, ok := nlw.Optional(47892); ok { + if nl, ok := nlw.Optional(54251); ok { // We subtract the leniency interval here to artificially // widen the range of times over which the job registry will // consider the node to be alive. We rely on the fact that diff --git a/pkg/jobs/registry.go b/pkg/jobs/registry.go index 6c0b0b0a6f5d..daaf4e79fd61 100644 --- a/pkg/jobs/registry.go +++ b/pkg/jobs/registry.go @@ -775,7 +775,7 @@ func (r *Registry) maybeCancelJobs(ctx context.Context, s sqlliveness.Session) { func (r *Registry) maybeCancelJobsDeprecated( ctx context.Context, nlw optionalnodeliveness.Container, ) { - nl, ok := nlw.Optional(47892) + nl, ok := nlw.Optional(54251) if !ok { // At most one container is running on behalf of a SQL tenant, so it must be // this one, and there's no point canceling anything. diff --git a/pkg/server/serverpb/status.go b/pkg/server/serverpb/status.go index 6bb52d42c5ca..f6183ae6756c 100644 --- a/pkg/server/serverpb/status.go +++ b/pkg/server/serverpb/status.go @@ -53,9 +53,9 @@ type NodesStatusServer interface { // available. If it is not, an error referring to the optionally supplied issues // is returned. func (s *OptionalNodesStatusServer) OptionalNodesStatusServer( - issueNos ...int, + issue int, ) (NodesStatusServer, error) { - v, err := s.w.OptionalErr(issueNos...) + v, err := s.w.OptionalErr(issue) if err != nil { return nil, err } diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 60c07b258d8f..562667f53f81 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -53,6 +53,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/types" + "github.com/cockroachdb/cockroach/pkg/util/errorutil" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/json" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -3397,7 +3398,8 @@ CREATE TABLE crdb_internal.kv_node_status ( if err := p.RequireAdminRole(ctx, "read crdb_internal.kv_node_status"); err != nil { return err } - ss, err := p.extendedEvalCtx.NodesStatusServer.OptionalNodesStatusServer() + ss, err := p.extendedEvalCtx.NodesStatusServer.OptionalNodesStatusServer( + errorutil.FeatureNotAvailableToNonSystemTenantsIssue) if err != nil { return err } @@ -3511,7 +3513,8 @@ CREATE TABLE crdb_internal.kv_store_status ( if err := p.RequireAdminRole(ctx, "read crdb_internal.kv_store_status"); err != nil { return err } - ss, err := p.ExecCfg().NodesStatusServer.OptionalNodesStatusServer() + ss, err := p.ExecCfg().NodesStatusServer.OptionalNodesStatusServer( + errorutil.FeatureNotAvailableToNonSystemTenantsIssue) if err != nil { return err } diff --git a/pkg/sql/optionalnodeliveness/node_liveness.go b/pkg/sql/optionalnodeliveness/node_liveness.go index a1f3eddcc929..1cb5e07039a7 100644 --- a/pkg/sql/optionalnodeliveness/node_liveness.go +++ b/pkg/sql/optionalnodeliveness/node_liveness.go @@ -47,8 +47,8 @@ func MakeContainer(nl Interface) Container { // // Use of NodeLiveness from within the SQL layer is **deprecated**. Please do // not introduce new uses of it. -func (nl *Container) OptionalErr(issueNos ...int) (Interface, error) { - v, err := nl.w.OptionalErr(issueNos...) +func (nl *Container) OptionalErr(issue int) (Interface, error) { + v, err := nl.w.OptionalErr(issue) if err != nil { return nil, err } @@ -62,7 +62,7 @@ var _ = (*Container)(nil).OptionalErr // silence unused lint // // Use of NodeLiveness from within the SQL layer is **deprecated**. Please do // not introduce new uses of it. -func (nl *Container) Optional(issueNos ...int) (Interface, bool) { +func (nl *Container) Optional(issue int) (Interface, bool) { v, ok := nl.w.Optional() if !ok { return nil, false diff --git a/pkg/sql/relocate.go b/pkg/sql/relocate.go index b1b71695bf9b..1a0cb88d2a90 100644 --- a/pkg/sql/relocate.go +++ b/pkg/sql/relocate.go @@ -92,7 +92,7 @@ func (n *relocateNode) Next(params runParams) (bool, error) { // Lookup the store in gossip. var storeDesc roachpb.StoreDescriptor gossipStoreKey := gossip.MakeStoreKey(storeID) - g, err := params.extendedEvalCtx.ExecCfg.Gossip.OptionalErr() + g, err := params.extendedEvalCtx.ExecCfg.Gossip.OptionalErr(54250) if err != nil { return false, err } diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index 9a645db88681..42e91625c927 100644 --- a/pkg/sql/set_zone_config.go +++ b/pkg/sql/set_zone_config.go @@ -573,7 +573,7 @@ func (n *setZoneConfigNode) startExec(params runParams) error { return err } - ss, err := params.extendedEvalCtx.NodesStatusServer.OptionalNodesStatusServer() + ss, err := params.extendedEvalCtx.NodesStatusServer.OptionalNodesStatusServer(multitenancyZoneCfgIssueNo) if err != nil { return err } diff --git a/pkg/util/errorutil/tenant_deprecated_wrapper.go b/pkg/util/errorutil/tenant_deprecated_wrapper.go index 170b6b1e23a5..424d08828bc4 100644 --- a/pkg/util/errorutil/tenant_deprecated_wrapper.go +++ b/pkg/util/errorutil/tenant_deprecated_wrapper.go @@ -73,10 +73,15 @@ func (w TenantSQLDeprecatedWrapper) Optional() (interface{}, bool) { // OptionalErr calls Optional and returns an error (referring to the optionally // supplied Github issues) if the wrapped object is not available. -func (w TenantSQLDeprecatedWrapper) OptionalErr(issueNos ...int) (interface{}, error) { +func (w TenantSQLDeprecatedWrapper) OptionalErr(issue int) (interface{}, error) { v, ok := w.Optional() if !ok { - return nil, UnsupportedWithMultiTenancy(issueNos...) + return nil, UnsupportedWithMultiTenancy(issue) } return v, nil } + +// FeatureNotAvailableToNonSystemTenantsIssue is to be used with the Optinal +// and related error interfaces when a feature is simply not available +// to non-system tenants (i.e. we're not planning to change this). +const FeatureNotAvailableToNonSystemTenantsIssue = 54252 From 3fcafa8213237f20da583abf79fd7fcfdcbdece3 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 11 Sep 2020 12:48:18 +0200 Subject: [PATCH 2/2] sql: make multitenancy errors report to telemetry This change ensures that multitenancy errors get collected in telemetry. By also making the issue number non-optional, this patch also catches a few more cases that were missing open issues. Release note: None --- pkg/sql/opt_exec_factory.go | 8 ++--- pkg/sql/scatter.go | 2 +- pkg/sql/sem/builtins/generator_builtins.go | 3 +- pkg/sql/show_zone_config.go | 2 +- pkg/util/errorutil/tenant.go | 30 +++++++------------ .../errorutil/tenant_deprecated_wrapper.go | 5 ---- 6 files changed, 19 insertions(+), 31 deletions(-) diff --git a/pkg/sql/opt_exec_factory.go b/pkg/sql/opt_exec_factory.go index 9c53ff66a19d..775a7f27806a 100644 --- a/pkg/sql/opt_exec_factory.go +++ b/pkg/sql/opt_exec_factory.go @@ -1692,7 +1692,7 @@ func (ef *execFactory) ConstructAlterTableSplit( index cat.Index, input exec.Node, expiration tree.TypedExpr, ) (exec.Node, error) { if !ef.planner.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54254) } expirationTime, err := parseExpirationTime(ef.planner.EvalContext(), expiration) @@ -1713,7 +1713,7 @@ func (ef *execFactory) ConstructAlterTableUnsplit( index cat.Index, input exec.Node, ) (exec.Node, error) { if !ef.planner.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54254) } return &unsplitNode{ @@ -1726,7 +1726,7 @@ func (ef *execFactory) ConstructAlterTableUnsplit( // ConstructAlterTableUnsplitAll is part of the exec.Factory interface. func (ef *execFactory) ConstructAlterTableUnsplitAll(index cat.Index) (exec.Node, error) { if !ef.planner.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54254) } return &unsplitAllNode{ @@ -1740,7 +1740,7 @@ func (ef *execFactory) ConstructAlterTableRelocate( index cat.Index, input exec.Node, relocateLease bool, ) (exec.Node, error) { if !ef.planner.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54250) } return &relocateNode{ diff --git a/pkg/sql/scatter.go b/pkg/sql/scatter.go index fd7dafd999a0..b6205f31f194 100644 --- a/pkg/sql/scatter.go +++ b/pkg/sql/scatter.go @@ -34,7 +34,7 @@ type scatterNode struct { // Privileges: INSERT on table. func (p *planner) Scatter(ctx context.Context, n *tree.Scatter) (planNode, error) { if !p.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(54255) } tableDesc, index, err := p.getTableAndIndex(ctx, &n.TableOrIndex, privilege.INSERT) diff --git a/pkg/sql/sem/builtins/generator_builtins.go b/pkg/sql/sem/builtins/generator_builtins.go index dacdb0a3d336..47d8a9aa4bc9 100644 --- a/pkg/sql/sem/builtins/generator_builtins.go +++ b/pkg/sql/sem/builtins/generator_builtins.go @@ -1129,7 +1129,8 @@ func makeCheckConsistencyGenerator( ctx *tree.EvalContext, args tree.Datums, ) (tree.ValueGenerator, error) { if !ctx.Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy( + errorutil.FeatureNotAvailableToNonSystemTenantsIssue) } keyFrom := roachpb.Key(*args[1].(*tree.DBytes)) diff --git a/pkg/sql/show_zone_config.go b/pkg/sql/show_zone_config.go index 778ad3086a1d..cc21da8aba4c 100644 --- a/pkg/sql/show_zone_config.go +++ b/pkg/sql/show_zone_config.go @@ -64,7 +64,7 @@ const ( func (p *planner) ShowZoneConfig(ctx context.Context, n *tree.ShowZoneConfig) (planNode, error) { if !p.ExecCfg().Codec.ForSystemTenant() { - return nil, errorutil.UnsupportedWithMultiTenancy() + return nil, errorutil.UnsupportedWithMultiTenancy(multitenancyZoneCfgIssueNo) } return &delayedNode{ diff --git a/pkg/util/errorutil/tenant.go b/pkg/util/errorutil/tenant.go index 27aac2cff6ed..1eb022966edd 100644 --- a/pkg/util/errorutil/tenant.go +++ b/pkg/util/errorutil/tenant.go @@ -10,28 +10,20 @@ package errorutil -import ( - "fmt" - "strings" - - "github.com/cockroachdb/errors" -) +import "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" // UnsupportedWithMultiTenancy returns an error suitable for returning when an // operation could not be carried out due to the SQL server running in // multi-tenancy mode. In that mode, Gossip and other components of the KV layer // are not available. -func UnsupportedWithMultiTenancy(issues ...int) error { - var buf strings.Builder - buf.WriteString("operation is unsupported in multi-tenancy mode") - if len(issues) > 0 { - buf.WriteString("; see:\n") - const prefix = "\nhttps://github.com/cockroachdb/cockroach/issues/" - for _, n := range issues { - fmt.Fprint(&buf, prefix, n) - } - } - // TODO(knz): This should be done in a different way, - // see: https://github.com/cockroachdb/cockroach/issues/48164 - return errors.Newf("%s", buf.String()) +func UnsupportedWithMultiTenancy(issue int) error { + return unimplemented.NewWithIssue(issue, "operation is unsupported in multi-tenancy mode") } + +// FeatureNotAvailableToNonSystemTenantsIssue is to be used with the +// Optional and related error interfaces when a feature is simply not +// available to non-system tenants (i.e. we're not planning to change +// this). +// For all other multitenancy errors where there is a plan to +// improve the situation, a specific issue should be created instead. +const FeatureNotAvailableToNonSystemTenantsIssue = 54252 diff --git a/pkg/util/errorutil/tenant_deprecated_wrapper.go b/pkg/util/errorutil/tenant_deprecated_wrapper.go index 424d08828bc4..5d98755aca74 100644 --- a/pkg/util/errorutil/tenant_deprecated_wrapper.go +++ b/pkg/util/errorutil/tenant_deprecated_wrapper.go @@ -80,8 +80,3 @@ func (w TenantSQLDeprecatedWrapper) OptionalErr(issue int) (interface{}, error) } return v, nil } - -// FeatureNotAvailableToNonSystemTenantsIssue is to be used with the Optinal -// and related error interfaces when a feature is simply not available -// to non-system tenants (i.e. we're not planning to change this). -const FeatureNotAvailableToNonSystemTenantsIssue = 54252