Merge #54256

54256: sql: make multi-tenancy errors report to telemetry r=asubiotto a=knz Fixes #48164. Issues referenced, I also added the X-anchored-telemetry label to them on github (we keep those issues open on github until their reference in the code is removed): #54254 #54255 #54250 #54251 #54252 #49854 #48274 #47150 #47971 #47970 #47900 #47925 Co-authored-by: Raphael 'kena' Poss <[email protected]>
cockroachdb · Sep 14, 2020 · 40f03d9 · 40f03d9
2 parents dc55448 + 3fcafa8
commit 40f03d9
Show file tree

Hide file tree

Showing 15 changed files with 40 additions and 44 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/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/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
@@ -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/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
@@ -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/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/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
@@ -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
@@ -73,10 +73,10 @@ 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
 }