server: feature-gate the span config manager

We'll repurpose the COCKROACH_EXPERIMENTAL_SPAN_CONFIGS envvar
introduced in the previous commit to prevent initializing the span
config manager for the host tenant (now truly switching off all span
config infrastructure unless explicitly opted into).

For both host and secondary tenants, we also want to disable the
creation of the span config reconciliation job by default. We introduce
`spanconfig.experimental_reconciliation_job.enabled` to that end.

---

We re-work some of the controls around when we start the reconciliation
job. It was previously possible for a tenant pod to start with a
conservative view of it's cluster version setting, decide not to create
the reconciliation job, find out about the up-to-date cluster version
(introducing the span config job), but only create it when the next
`check_interval` timer fired. This was undesirable.

We also want to "bounce" the job immediately when the `check_interval`
setting is changed. The earlier code didn't quite provide that property.

Release justification: non-production code changes
Release note: None

pkg/base/test_server_args.go

@@ -133,7 +133,8 @@ type TestServerArgs struct {
 	// If set, a TraceDir is initialized at the provided path.
 	TraceDir string
 
-	// If set, the span configs infrastructure will be enabled.
+	// If set, the span configs infrastructure will be enabled. This is
+	// equivalent to setting COCKROACH_EXPERIMENTAL_SPAN_CONFIGS.
 	EnableSpanConfigs bool
 }
 

pkg/cli/testdata/doctor/test_examine_cluster

@@ -3,5 +3,5 @@ debug doctor examine cluster
 debug doctor examine cluster
 Examining 41 descriptors and 42 namespace entries...
   ParentID  50, ParentSchemaID 29: relation "foo" (53): expected matching namespace entry, found none
-Examining 4 jobs...
+Examining 3 jobs...
 ERROR: validation failed

pkg/clusterversion/clusterversion.go

@@ -131,6 +131,12 @@ type Handle interface {
 	// node restarts when initializing the cluster version, as seen by this
 	// node.
 	SetActiveVersion(context.Context, ClusterVersion) error
+
+	// SetOnChange installs a callback that's invoked when the active cluster
+	// version changes. The callback should avoid doing long-running or blocking
+	// work; it's called on the same goroutine handling all cluster setting
+	// updates.
+	SetOnChange(fn func(context.Context))
 }
 
 // handleImpl is a concrete implementation of Handle. It mostly relegates to the
@@ -205,6 +211,11 @@ func (v *handleImpl) SetActiveVersion(ctx context.Context, cv ClusterVersion) er
 	return nil
 }
 
+// SetOnChange implements the Handle interface.
+func (v *handleImpl) SetOnChange(fn func(ctx context.Context)) {
+	version.SetOnChange(v.sv, fn)
+}
+
 // IsActive implements the Handle interface.
 func (v *handleImpl) IsActive(ctx context.Context, key Key) bool {
 	return version.isActive(ctx, v.sv, key)

pkg/server/config.go

@@ -149,6 +149,11 @@ type BaseConfig struct {
 	// instantiate stores.
 	StorageEngine enginepb.EngineType
 
+	// Enables the use of the (experimental) span configs infrastructure.
+	//
+	// Environment Variable: COCKROACH_EXPERIMENTAL_SPAN_CONFIGS
+	SpanConfigsEnabled bool
+
 	// TestingKnobs is used for internal test controls only.
 	TestingKnobs base.TestingKnobs
 }
@@ -222,11 +227,6 @@ type KVConfig struct {
 	// The following values can only be set via environment variables and are
 	// for testing only. They are not meant to be set by the end user.
 
-	// Enables the use of the (experimental) span configs infrastructure.
-	//
-	// Environment Variable: COCKROACH_EXPERIMENTAL_SPAN_CONFIGS
-	SpanConfigsEnabled bool
-
 	// Enables linearizable behavior of operations on this node by making sure
 	// that no commit timestamp is reported back to the client until all other
 	// node clocks have necessarily passed it.

pkg/server/node.go

@@ -1447,10 +1447,6 @@ func (emptyMetricStruct) MetricStruct() {}
 func (n *Node) GetSpanConfigs(
 	ctx context.Context, req *roachpb.GetSpanConfigsRequest,
 ) (*roachpb.GetSpanConfigsResponse, error) {
-	if !n.storeCfg.SpanConfigsEnabled {
-		return nil, errors.New("use of span configs disabled")
-	}
-
 	entries, err := n.spanConfigAccessor.GetSpanConfigEntriesFor(ctx, req.Spans)
 	if err != nil {
 		return nil, err
@@ -1463,9 +1459,6 @@ func (n *Node) GetSpanConfigs(
 func (n *Node) UpdateSpanConfigs(
 	ctx context.Context, req *roachpb.UpdateSpanConfigsRequest,
 ) (*roachpb.UpdateSpanConfigsResponse, error) {
-	if !n.storeCfg.SpanConfigsEnabled {
-		return nil, errors.New("use of span configs disabled")
-	}
 	// TODO(irfansharif): We want to protect ourselves from tenants creating
 	// outlandishly large string buffers here and OOM-ing the host cluster. Is
 	// the maximum protobuf message size enough of a safeguard?

pkg/server/server.go

@@ -636,6 +636,8 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 			db, internalExecutor, cfg.Settings,
 			systemschema.SpanConfigurationsTableName.FQString(),
 		)
+	} else {
+		spanConfigAccessor = spanconfigkvaccessor.DisabledAccessor{}
 	}
 
 	if storeTestingKnobs := cfg.TestingKnobs.Store; storeTestingKnobs != nil {

pkg/server/server_sql.go

@@ -829,20 +829,22 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 		execCfg.MigrationTestingKnobs = knobs
 	}
 
-	// Instantiate a span config manager; it exposes a hook to idempotently
-	// create the span config reconciliation job and captures all relevant job
-	// dependencies.
-	spanConfigKnobs, _ := cfg.TestingKnobs.SpanConfig.(*spanconfig.TestingKnobs)
-	spanConfigMgr := spanconfigmanager.New(
-		cfg.db,
-		jobRegistry,
-		cfg.circularInternalExecutor,
-		cfg.stopper,
-		cfg.Settings,
-		cfg.spanConfigAccessor,
-		spanConfigKnobs,
-	)
-	execCfg.SpanConfigReconciliationJobDeps = spanConfigMgr
+	var spanConfigMgr *spanconfigmanager.Manager
+	if !codec.ForSystemTenant() || cfg.SpanConfigsEnabled {
+		// Instantiate a span config manager. If we're the host tenant we'll
+		// only do it if COCKROACH_EXPERIMENTAL_SPAN_CONFIGS is set.
+		spanConfigKnobs, _ := cfg.TestingKnobs.SpanConfig.(*spanconfig.TestingKnobs)
+		spanConfigMgr = spanconfigmanager.New(
+			cfg.db,
+			jobRegistry,
+			cfg.circularInternalExecutor,
+			cfg.stopper,
+			cfg.Settings,
+			cfg.spanConfigAccessor,
+			spanConfigKnobs,
+		)
+		execCfg.SpanConfigReconciliationJobDeps = spanConfigMgr
+	}
 
 	temporaryObjectCleaner := sql.NewTemporaryObjectCleaner(
 		cfg.Settings,
@@ -1043,8 +1045,10 @@ func (s *SQLServer) preStart(
 		return err
 	}
 
-	if err := s.spanconfigMgr.Start(ctx); err != nil {
-		return err
+	if s.spanconfigMgr != nil {
+		if err := s.spanconfigMgr.Start(ctx); err != nil {
+			return err
+		}
 	}
 
 	var bootstrapVersion roachpb.Version

pkg/settings/version.go

@@ -176,15 +176,3 @@ func TestingRegisterVersionSetting(key, desc string, impl VersionSettingImpl) *V
 	register(key, desc, &setting)
 	return &setting
 }
-
-// SetOnChange is part of the Setting interface, and is discouraged for use in
-// VersionSetting (we're implementing it here to not fall back on the embedded
-// `common` type definition).
-//
-// NB: VersionSetting is unique in more ways than one, and we might want to move
-// it out of the settings package before long (see TODO on the type itself). In
-// our current usage we don't rely on attaching pre-change triggers, so let's
-// not add it needlessly.
-func (v *VersionSetting) SetOnChange(sv *Values, fn func(ctx context.Context)) {
-	panic("unimplemented")
-}

pkg/spanconfig/spanconfigkvaccessor/BUILD.bazel

@@ -2,7 +2,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "spanconfigkvaccessor",
-    srcs = ["kvaccessor.go"],
+    srcs = [
+        "disabled.go",
+        "kvaccessor.go",
+    ],
     importpath = "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigkvaccessor",
     visibility = ["//visibility:public"],
     deps = [

pkg/spanconfig/spanconfigkvaccessor/disabled.go

@@ -0,0 +1,39 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package spanconfigkvaccessor
+
+import (
+	"context"
+
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/spanconfig"
+	"github.com/cockroachdb/errors"
+)
+
+// DisabledAccessor provides a implementation of the KVAccessor interface that
+// simply errors out with a disabled error.
+type DisabledAccessor struct{}
+
+// GetSpanConfigEntriesFor is part of the KVAccessor interface.
+func (n DisabledAccessor) GetSpanConfigEntriesFor(
+	context.Context, []roachpb.Span,
+) ([]roachpb.SpanConfigEntry, error) {
+	return nil, errors.New("span configs disabled")
+}
+
+// UpdateSpanConfigEntries is part of the KVAccessor interface.
+func (n DisabledAccessor) UpdateSpanConfigEntries(
+	context.Context, []roachpb.Span, []roachpb.SpanConfigEntry,
+) error {
+	return errors.New("span configs disabled")
+}
+
+var _ spanconfig.KVAccessor = &DisabledAccessor{}

pkg/spanconfig/spanconfigkvaccessor/kvaccessor.go

@@ -52,19 +52,22 @@ func New(
 	}
 }
 
-// enabledSetting is a hidden cluster setting that gates usage of the
-// KVAccessor. It has no effect unless COCKROACH_EXPERIMENTAL_SPAN_CONFIGS is
-// also set.
+// enabledSetting gates usage of the KVAccessor. It has no effect unless
+// COCKROACH_EXPERIMENTAL_SPAN_CONFIGS is also set.
 var enabledSetting = settings.RegisterBoolSetting(
 	"spanconfig.experimental_kvaccessor.enabled",
 	"enable the use of the kv accessor", false).WithSystemOnly()
 
+// errDisabled is returned if the setting gating usage of the KVAccessor is
+// disabled.
+var errDisabled = errors.New("span config kv accessor disabled")
+
 // GetSpanConfigEntriesFor is part of the KVAccessor interface.
 func (k *KVAccessor) GetSpanConfigEntriesFor(
 	ctx context.Context, spans []roachpb.Span,
 ) (resp []roachpb.SpanConfigEntry, retErr error) {
 	if !enabledSetting.Get(&k.settings.SV) {
-		return nil, errors.New("use of span configs disabled")
+		return nil, errDisabled
 	}
 
 	if len(spans) == 0 {
@@ -116,7 +119,7 @@ func (k *KVAccessor) UpdateSpanConfigEntries(
 	ctx context.Context, toDelete []roachpb.Span, toUpsert []roachpb.SpanConfigEntry,
 ) error {
 	if !enabledSetting.Get(&k.settings.SV) {
-		return errors.New("use of span configs disabled")
+		return errDisabled
 	}
 
 	if err := validateUpdateArgs(toDelete, toUpsert); err != nil {

pkg/spanconfig/spanconfigmanager/BUILD.bazel

@@ -17,8 +17,6 @@ go_library(
         "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/spanconfig",
-        "//pkg/sql/sem/tree",
-        "//pkg/sql/sessiondata",
         "//pkg/sql/sqlutil",
         "//pkg/util/log",
         "//pkg/util/stop",
@@ -35,14 +33,18 @@ go_test(
     deps = [
         ":spanconfigmanager",
         "//pkg/base",
+        "//pkg/clusterversion",
         "//pkg/jobs",
         "//pkg/jobs/jobspb",
         "//pkg/security",
         "//pkg/security/securitytest",
         "//pkg/server",
+        "//pkg/settings/cluster",
         "//pkg/spanconfig",
         "//pkg/sql",
+        "//pkg/testutils",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",
         "//pkg/util/protoutil",