kvserver, storage: Move SeparatedIntentsEnabled to a testing knob

Since we want to remove the ability to disable the writing
of separated intents once a version upgrade is finalized,
this change moves the SeparatedIntentsEnabled setting to
a testing knob. The bulk of changes here are just propagating
a flag from the new storage.TestingKnobs down to the engine.

Release note: None.

pkg/base/config.go

@@ -527,6 +527,9 @@ type StorageConfig struct {
 	// UseFileRegistry is true if the file registry is needed (eg: encryption-at-rest).
 	// This may force the store version to versionFileRegistry if currently lower.
 	UseFileRegistry bool
+	// DisableSeparatedIntents is true if separated intents should not be written
+	// by intent writers. Only true for tests.
+	DisableSeparatedIntents bool
 	// EncryptionOptions is a serialized protobuf set by Go CCL code and passed
 	// through to C CCL code to set up encryption-at-rest.  Must be set if and
 	// only if encryption is enabled, otherwise left empty.

pkg/kv/kvserver/testing_knobs.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/tenantrate"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/txnwait"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 )
@@ -35,6 +36,7 @@ type StoreTestingKnobs struct {
 	TxnWaitKnobs            txnwait.TestingKnobs
 	ConsistencyTestingKnobs ConsistencyTestingKnobs
 	TenantRateKnobs         tenantrate.TestingKnobs
+	StorageKnobs            storage.TestingKnobs
 
 	// TestingRequestFilter is called before evaluating each request on a
 	// replica. The filter is run before the request acquires latches, so

pkg/migration/migrations/separated_intents_external_test.go

@@ -12,7 +12,8 @@ package migrations_test
 
 import (
 	"context"
-	"strconv"
+	"fmt"
+	"path"
 	"testing"
 	"time"
 
@@ -38,29 +39,31 @@ func TestSeparatedIntentsMigration(t *testing.T) {
 	defer cancel()
 	settings := cluster.MakeTestingClusterSettingsWithVersions(
 		clusterversion.ByKey(clusterversion.SeparatedIntentsMigration),
-		clusterversion.ByKey(clusterversion.SeparatedIntentsMigration - 1),
+		clusterversion.ByKey(clusterversion.SeparatedIntentsMigration-1),
 		false, /* initializeVersion */
 	)
-	storage.SeparatedIntentsEnabled.Override(ctx, &settings.SV, false)
-	stickyEngineRegistry := server.NewStickyInMemEnginesRegistry()
-	defer stickyEngineRegistry.CloseAllStickyInMemEngines()
 	const numServers int = 5
 	stickyServerArgs := make(map[int]base.TestServerArgs)
+	storeKnobs := &kvserver.StoreTestingKnobs{
+		StorageKnobs: storage.TestingKnobs{DisableSeparatedIntents: true},
+	}
+	tempDir, cleanup := testutils.TempDir(t)
+	defer cleanup()
 	for i := 0; i < numServers; i++ {
 		stickyServerArgs[i] = base.TestServerArgs{
 			Settings: settings,
 			StoreSpecs: []base.StoreSpec{
 				{
-					InMemory:               true,
-					StickyInMemoryEngineID: strconv.FormatInt(int64(i), 10),
+					InMemory: false,
+					Path:     path.Join(tempDir, fmt.Sprintf("engine-%d", i)),
 				},
 			},
 			Knobs: base.TestingKnobs{
 				Server: &server.TestingKnobs{
 					DisableAutomaticVersionUpgrade: 1,
-					BinaryVersionOverride:          clusterversion.ByKey(clusterversion.SeparatedIntentsMigration-1),
-					StickyEngineRegistry:           stickyEngineRegistry,
+					BinaryVersionOverride:          clusterversion.ByKey(clusterversion.SeparatedIntentsMigration - 1),
 				},
+				Store: storeKnobs,
 			},
 		}
 	}
@@ -134,7 +137,8 @@ func TestSeparatedIntentsMigration(t *testing.T) {
 	}
 	require.Greater(t, interleavedIntentCount, 0)
 
-	storage.SeparatedIntentsEnabled.Override(ctx, &settings.SV, true)
+	// Start writing separated intents.
+	storeKnobs.StorageKnobs.DisableSeparatedIntents = false
 	require.NoError(t, tc.Restart())
 	time.Sleep(10 * time.Second)
 	require.NoError(t, tc.WaitForFullReplication())

pkg/server/config.go

@@ -476,6 +476,8 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 
 	skipSizeCheck := cfg.TestingKnobs.Store != nil &&
 		cfg.TestingKnobs.Store.(*kvserver.StoreTestingKnobs).SkipMinSizeCheck
+	disableSeparatedIntents := cfg.TestingKnobs.Store != nil &&
+		cfg.TestingKnobs.Store.(*kvserver.StoreTestingKnobs).StorageKnobs.DisableSeparatedIntents
 	for i, spec := range cfg.Stores.Specs {
 		log.Eventf(ctx, "initializing %+v", spec)
 		var sizeInBytes = spec.Size.InBytes
@@ -530,12 +532,13 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 				i, humanizeutil.IBytes(sizeInBytes), openFileLimitPerStore))
 
 			storageConfig := base.StorageConfig{
-				Attrs:             spec.Attributes,
-				Dir:               spec.Path,
-				MaxSize:           sizeInBytes,
-				Settings:          cfg.Settings,
-				UseFileRegistry:   spec.UseFileRegistry,
-				EncryptionOptions: spec.EncryptionOptions,
+				Attrs:                   spec.Attributes,
+				Dir:                     spec.Path,
+				MaxSize:                 sizeInBytes,
+				Settings:                cfg.Settings,
+				UseFileRegistry:         spec.UseFileRegistry,
+				DisableSeparatedIntents: disableSeparatedIntents,
+				EncryptionOptions:       spec.EncryptionOptions,
 			}
 			pebbleConfig := storage.PebbleConfig{
 				StorageConfig: storageConfig,

pkg/server/settingswatcher/BUILD.bazel

@@ -50,7 +50,6 @@ go_test(
         "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/sql",
-        "//pkg/storage",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/sqlutils",

pkg/server/settingswatcher/settings_watcher_external_test.go

@@ -21,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
-	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
@@ -93,9 +92,6 @@ func TestSettingWatcher(t *testing.T) {
 		s0.ExecutorConfig().(sql.ExecutorConfig).RangeFeedFactory,
 		tc.Stopper())
 	require.NoError(t, sw.Start(ctx))
-	// TestCluster randomizes the value of SeparatedIntentsEnabled, so set it to
-	// the same as in fakeSettings for the subsequent equality check.
-	storage.SeparatedIntentsEnabled.Override(ctx, &s0.ClusterSettings().SV, storage.SeparatedIntentsEnabled.Get(&fakeSettings.SV))
 	require.NoError(t, checkSettingsValuesMatch(s0.ClusterSettings(), fakeSettings))
 	for k, v := range toSet {
 		tdb.Exec(t, "SET CLUSTER SETTING "+k+" = $1", v[1])

pkg/server/testserver.go

@@ -15,7 +15,6 @@ import (
 	"context"
 	"encoding/base64"
 	"fmt"
-	"math/rand"
 	"net"
 	"net/http"
 	"net/http/cookiejar"
@@ -129,10 +128,6 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
 	st := params.Settings
 	if params.Settings == nil {
 		st = cluster.MakeClusterSettings()
-		enabledSeparated := rand.Intn(2) == 0
-		log.Infof(context.Background(),
-			"test Config is randomly setting enabledSeparated: %t", enabledSeparated)
-		storage.SeparatedIntentsEnabled.Override(context.Background(), &st.SV, enabledSeparated)
 	}
 	st.ExternalIODir = params.ExternalIODir
 	cfg := makeTestConfig(st)

pkg/storage/BUILD.bazel

@@ -35,6 +35,7 @@ go_library(
         "stacks.go",
         "temp_dir.go",
         "temp_engine.go",
+        "testing_knobs.go",
     ],
     # List out all the c-dependencies pkg/storage depends on.
     cdeps = [

pkg/storage/batch_test.go

@@ -1106,6 +1106,7 @@ func TestDecodeKey(t *testing.T) {
 		1<<20,   /* cacheSize */
 		512<<20, /* storeSize */
 		nil,     /* settings */
+		nil,     /* knobs */
 	)
 	defer e.Close()
 

pkg/storage/bench_pebble_test.go

@@ -42,7 +42,7 @@ func setupMVCCPebble(b testing.TB, dir string) Engine {
 			StorageConfig: base.StorageConfig{
 				Dir: dir,
 				Settings: makeSettingsForSeparatedIntents(
-					false /* oldClusterVersion */, true /* enabled */),
+					false /* oldClusterVersion */),
 			},
 			Opts: opts,
 		})
@@ -54,10 +54,12 @@ func setupMVCCPebble(b testing.TB, dir string) Engine {
 
 func setupMVCCInMemPebble(b testing.TB, loc string) Engine {
 	return setupMVCCInMemPebbleWithSettings(b, makeSettingsForSeparatedIntents(
-		false /* oldClusterVersion */, true /* enabled */))
+		false /* oldClusterVersion */), false /* disabledSeparatedIntents */)
 }
 
-func setupMVCCInMemPebbleWithSettings(b testing.TB, settings *cluster.Settings) Engine {
+func setupMVCCInMemPebbleWithSettings(
+	b testing.TB, settings *cluster.Settings, disableSeparatedIntents bool,
+) Engine {
 	opts := DefaultPebbleOptions()
 	opts.FS = vfs.NewMem()
 	opts.Cache = pebble.NewCache(testCacheSize)
@@ -68,7 +70,8 @@ func setupMVCCInMemPebbleWithSettings(b testing.TB, settings *cluster.Settings)
 		PebbleConfig{
 			Opts: opts,
 			StorageConfig: base.StorageConfig{
-				Settings: settings,
+				Settings:                settings,
+				DisableSeparatedIntents: disableSeparatedIntents,
 			},
 		})
 	if err != nil {

pkg/storage/bench_test.go

@@ -214,7 +214,7 @@ func BenchmarkIntentScan(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 80, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
 							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+								b, makeSettingsForSeparatedIntents(false), sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false /* resolveAll */)
 							lower := makeKey(nil, 0)
@@ -278,7 +278,7 @@ func BenchmarkScanAllIntentsResolved(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
 							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+								b, makeSettingsForSeparatedIntents(false), sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, true /* resolveAll */)
 							lower := makeKey(nil, 0)
@@ -349,7 +349,7 @@ func BenchmarkScanOneAllIntentsResolved(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
 							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+								b, makeSettingsForSeparatedIntents(false), sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, true /* resolveAll */)
 							lower := makeKey(nil, 0)
@@ -401,7 +401,7 @@ func BenchmarkIntentResolution(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 80, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
 							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+								b, makeSettingsForSeparatedIntents(false), sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							lockUpdate := setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false /* resolveAll */)
 							keys := make([]roachpb.Key, numIntentKeys)
@@ -445,7 +445,7 @@ func BenchmarkIntentRangeResolution(b *testing.B) {
 					for _, percentFlushed := range []int{0, 50, 80, 90, 100} {
 						b.Run(fmt.Sprintf("percent-flushed=%d", percentFlushed), func(b *testing.B) {
 							eng := setupMVCCInMemPebbleWithSettings(
-								b, makeSettingsForSeparatedIntents(false, sep))
+								b, makeSettingsForSeparatedIntents(false), sep)
 							numFlushedVersions := (percentFlushed * numVersions) / 100
 							lockUpdate := setupKeysWithIntent(b, eng, numVersions, numFlushedVersions, false /* resolveAll */)
 							keys := make([]roachpb.Key, numIntentKeys+1)

pkg/storage/engine_test.go

@@ -1542,6 +1542,7 @@ func TestSupportsPrev(t *testing.T) {
 			1<<20,   /* cacheSize */
 			512<<20, /* storeSize */
 			nil,     /* settings */
+			nil,     /* knobs */
 		)
 		defer eng.Close()
 		runTest(t, eng, engineTest{
@@ -1665,13 +1666,15 @@ func TestScanSeparatedIntents(t *testing.T) {
 
 	for name, enableSeparatedIntents := range map[string]bool{"interleaved": false, "separated": true} {
 		t.Run(name, func(t *testing.T) {
-			settings := makeSettingsForSeparatedIntents(false, enableSeparatedIntents)
+			settings := makeSettingsForSeparatedIntents(false)
+			knobs := &TestingKnobs{DisableSeparatedIntents: !enableSeparatedIntents}
 			eng := newPebbleInMem(
 				ctx,
 				roachpb.Attributes{},
 				1<<20,   /* cacheSize */
 				512<<20, /* storeSize */
 				settings,
+				knobs,
 			)
 			defer eng.Close()
 

pkg/storage/in_mem.go

@@ -30,7 +30,7 @@ func NewInMem(
 	cacheSize, storeSize int64,
 	settings *cluster.Settings,
 ) Engine {
-	return newPebbleInMem(ctx, attrs, cacheSize, storeSize, settings)
+	return newPebbleInMem(ctx, attrs, cacheSize, storeSize, settings, nil /* knobs */)
 }
 
 // The ForTesting functions randomize the settings for separated intents. This
@@ -48,7 +48,12 @@ func NewInMem(
 // must call the engine's Close method when the engine is no longer needed.
 func NewInMemForTesting(ctx context.Context, attrs roachpb.Attributes, storeSize int64) Engine {
 	settings := MakeRandomSettingsForSeparatedIntents()
-	return newPebbleInMem(ctx, attrs, 0 /* cacheSize */, storeSize, settings)
+	disableSeparatedIntents := rand.Intn(2) == 0
+	knobs := &TestingKnobs{DisableSeparatedIntents: disableSeparatedIntents}
+	log.Infof(context.Background(),
+		"engine creation is randomly setting disableSeparatedIntents: %t",
+		disableSeparatedIntents)
+	return newPebbleInMem(ctx, attrs, 0 /* cacheSize */, storeSize, settings, knobs /* knobs */)
 }
 
 // NewDefaultInMemForTesting allocates and returns a new, opened in-memory engine with
@@ -59,23 +64,21 @@ func NewDefaultInMemForTesting() Engine {
 }
 
 // MakeRandomSettingsForSeparatedIntents makes settings for which it randomly
-// picks whether the cluster understands separated intents, and if yes,
-// whether to write separated intents. Once made, these setting do not change.
+// picks whether the cluster understands and writes separated intents.
+// Once made, these setting do not change.
 func MakeRandomSettingsForSeparatedIntents() *cluster.Settings {
 	oldClusterVersion := rand.Intn(2) == 0
-	enabledSeparated := rand.Intn(2) == 0
 	log.Infof(context.Background(),
-		"engine creation is randomly setting oldClusterVersion: %t, enabledSeparated: %t",
-		oldClusterVersion, enabledSeparated)
-	return makeSettingsForSeparatedIntents(oldClusterVersion, enabledSeparated)
+		"engine creation is randomly setting oldClusterVersion: %t",
+		oldClusterVersion)
+	return makeSettingsForSeparatedIntents(oldClusterVersion)
 }
 
-func makeSettingsForSeparatedIntents(oldClusterVersion bool, enabled bool) *cluster.Settings {
+func makeSettingsForSeparatedIntents(oldClusterVersion bool) *cluster.Settings {
 	version := clusterversion.ByKey(clusterversion.SeparatedIntents)
 	if oldClusterVersion {
 		version = clusterversion.ByKey(clusterversion.V20_2)
 	}
 	settings := cluster.MakeTestingClusterSettingsWithVersions(version, version, true)
-	SeparatedIntentsEnabled.Override(context.TODO(), &settings.SV, enabled)
 	return settings
 }