Merge #148967 #149265 #149353

148967: roachtest: add fingerprint validation to classic restore roachtests r=jeffswenson a=kev-cao

With the switch to the new fixtures in #146555, we temporarily removed fingerprint validation. This commit teaches the restore roachtests to fingerprint after restore, validating the results against the fingerprints stored in the fixtures (introduced in #147662.

Epic: CRDB-50823

Fixes: #148962

Release note: None

149265: roachtest: follower-reads/mixed-version handle schema_locked r=rafiss a=fqazi

Previously, we unconditionally attempted to toggle schema_locked in the follower read mixed versions tests, which would fail since versions before 25.3 do not support the new create_table_withschema_locked setting. This patch updates the test to only toggle this setting if the active version for the cluster is 25.3 or greater.

Fixes: #149150

Release note: None

149353: roachtest: mark cluster as expected to die in validate_system_schema_after_version_upgrade r=rafiss a=rafiss

Before wiping the cluster, we need to indicate to the test that the
cluster is going to go down. This is needed after
eae1e78.

fixes #148981
fixes #148998

Release note: None

Co-authored-by: Kevin Cao <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

Author: 4 people

pkg/cmd/roachtest/tests/backup_fixtures.go

@@ -7,6 +7,7 @@ package tests
 
 import (
 	"context"
+	gosql "database/sql"
 	"fmt"
 	"net/url"
 	"path"
@@ -25,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/blobfixture"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
+	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
@@ -412,20 +414,52 @@ func (bd *backupDriver) queryJobStates(
 func (bd *backupDriver) fingerprintFixture(ctx context.Context) map[string]string {
 	conn := bd.c.Conn(ctx, bd.t.L(), 1)
 	defer conn.Close()
-	sql := sqlutils.MakeSQLRunner(conn)
-	aost := bd.getLatestAOST(ctx, sql)
-	tables := getDatabaseTables(ctx, bd.t, sql, bd.sp.fixture.DatabaseName())
+	return fingerprintDatabase(
+		ctx, bd.t, conn,
+		bd.sp.fixture.DatabaseName(), bd.getLatestAOST(sqlutils.MakeSQLRunner(conn)),
+	)
+}
 
-	m := bd.c.NewDeprecatedMonitor(ctx)
+// getLatestAOST returns the end time as seen in SHOW BACKUP of the latest
+// backup in the fixture.
+func (bd *backupDriver) getLatestAOST(sql *sqlutils.SQLRunner) string {
+	uri := bd.registry.URI(bd.fixture.DataPath)
+	query := fmt.Sprintf(
+		`SELECT end_time FROM
+		[SHOW BACKUP FROM LATEST IN '%s']
+		ORDER BY end_time DESC
+		LIMIT 1`,
+		uri.String(),
+	)
+	var endTime string
+	sql.QueryRow(bd.t, query).Scan(&endTime)
+	return endTime
+}
+
+// fingerprintDatabase fingerprints all of the tables in the provided database
+// and returns a map of fully qualified table names to their fingerprints.
+// If AOST is not provided, the current time is used as the AOST.
+func fingerprintDatabase(
+	ctx context.Context, t test.Test, conn *gosql.DB, dbName string, aost string,
+) map[string]string {
+	sql := sqlutils.MakeSQLRunner(conn)
+	tables := getDatabaseTables(ctx, t, sql, dbName)
+	if len(tables) == 0 {
+		t.L().Printf("no tables found in database %s", dbName)
+		return nil
+	}
+	t.L().Printf("fingerprinting %d tables in database %s", len(tables), dbName)
 
-	bd.t.L().Printf("fingerprinting %d tables in %s", len(tables), bd.sp.fixture.DatabaseName())
 	fingerprints := make(map[string]string)
 	var mu syncutil.Mutex
 	start := timeutil.Now()
+	group := t.NewErrorGroup()
 	for _, table := range tables {
-		m.Go(func(ctx context.Context) error {
+		group.Go(func(ctx context.Context, log *logger.Logger) error {
 			fpContents := newFingerprintContents(conn, table)
-			if err := fpContents.Load(ctx, bd.t.L(), aost, nil /* tableContents */); err != nil {
+			if err := fpContents.Load(
+				ctx, log, aost, nil, /* tableContents */
+			); err != nil {
 				return err
 			}
 			mu.Lock()
@@ -434,31 +468,14 @@ func (bd *backupDriver) fingerprintFixture(ctx context.Context) map[string]strin
 			return nil
 		})
 	}
-	m.Wait()
-	bd.t.L().Printf(
+	require.NoError(t, group.WaitE(), "error fingerprinting tables in database %s", dbName)
+	t.L().Printf(
 		"fingerprinted %d tables in %s in %s",
-		len(tables), bd.sp.fixture.DatabaseName(), timeutil.Since(start),
+		len(tables), dbName, timeutil.Since(start),
 	)
-
 	return fingerprints
 }
 
-// getLatestAOST returns the end time as seen in SHOW BACKUP of the latest
-// backup in the fixture.
-func (bd *backupDriver) getLatestAOST(ctx context.Context, sql *sqlutils.SQLRunner) string {
-	uri := bd.registry.URI(bd.fixture.DataPath)
-	query := fmt.Sprintf(
-		`SELECT end_time FROM
-		[SHOW BACKUP FROM LATEST IN '%s']
-		ORDER BY end_time DESC
-		LIMIT 1`,
-		uri.String(),
-	)
-	var endTime string
-	sql.QueryRow(bd.t, query).Scan(&endTime)
-	return endTime
-}
-
 // getDatabaseTables returns the fully qualified name of every table in the
 // fixture.
 // Note: This assumes there aren't any funky characters in the identifiers, so

pkg/cmd/roachtest/tests/follower_reads.go

@@ -17,6 +17,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
@@ -25,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/task"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
@@ -97,7 +99,7 @@ func registerFollowerReads(r registry.Registry) {
 				}()
 
 				rng, _ := randutil.NewPseudoRand()
-				data := initFollowerReadsDB(ctx, t, t.L(), c, connFunc, connFunc, rng, topology)
+				data := initFollowerReadsDB(ctx, t, t.L(), c, connFunc, connFunc, rng, topology, clusterversion.Latest.Version())
 				runFollowerReadsTest(ctx, t, t.L(), c, connFunc, connFunc, rng, topology, rc, data)
 			},
 		})
@@ -488,6 +490,7 @@ func initFollowerReadsDB(
 	connectFunc, systemConnectFunc func(int) *gosql.DB,
 	rng *rand.Rand,
 	topology topologySpec,
+	clusterVersion roachpb.Version,
 ) (data map[int]int64) {
 	systemDB := systemConnectFunc(1)
 	db := connectFunc(1)
@@ -529,10 +532,12 @@ func initFollowerReadsDB(
 
 	// Disable schema_locked within this since it will modify locality on
 	// tables.
-	_, err = db.ExecContext(ctx, "SET create_table_with_schema_locked=false")
-	require.NoError(t, err)
-	_, err = db.ExecContext(ctx, "ALTER ROLE ALL SET create_table_with_schema_locked=false")
-	require.NoError(t, err)
+	if clusterVersion.AtLeast(clusterversion.V25_3.Version()) {
+		_, err = db.ExecContext(ctx, "SET create_table_with_schema_locked=false")
+		require.NoError(t, err)
+		_, err = db.ExecContext(ctx, "ALTER ROLE ALL SET create_table_with_schema_locked=false")
+		require.NoError(t, err)
+	}
 
 	// Create a multi-region database and table.
 	_, err = db.ExecContext(ctx, `CREATE DATABASE mr_db`)
@@ -1075,7 +1080,9 @@ func runFollowerReadsMixedVersionTest(
 			}
 		}
 
-		data = initFollowerReadsDB(ctx, t, l, c, h.Connect, h.System.Connect, r, topology)
+		version, err := h.ClusterVersion(r)
+		require.NoError(t, err)
+		data = initFollowerReadsDB(ctx, t, l, c, h.Connect, h.System.Connect, r, topology, version)
 		return nil
 	}
 

pkg/cmd/roachtest/tests/mixed_version_backup.go

@@ -622,8 +622,9 @@ func newFingerprintContents(db *gosql.DB, table string) *fingerprintContents {
 	return &fingerprintContents{db: db, table: table}
 }
 
-// Load computes the fingerprints for the underlying table and stores
-// the contents in the `fingeprints` field.
+// Load computes the fingerprints for the underlying table and stores the
+// contents in the `fingeprints` field. If timestamp is not set, computes
+// the fingerprint for the current time.
 func (fc *fingerprintContents) Load(
 	ctx context.Context, l *logger.Logger, timestamp string, _ tableContents,
 ) error {

pkg/cmd/roachtest/tests/online_restore.go

@@ -292,7 +292,7 @@ func registerOnlineRestorePerf(r registry.Registry) {
 						// Takes 10 minutes on OR tests for some reason.
 						SkipPostValidations: registry.PostValidationReplicaDivergence,
 						Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
-							rd := makeRestoreDriver(t, c, sp.restoreSpecs)
+							rd := makeRestoreDriver(ctx, t, c, sp.restoreSpecs)
 							rd.prepareCluster(ctx)
 
 							restoreStats := runRestore(
@@ -383,7 +383,7 @@ func registerOnlineRestoreCorrectness(r registry.Registry) {
 					t, sp, defaultSeed, defaultFakeTime, "-online.trace",
 				)
 
-				rd := makeRestoreDriver(t, c, sp.restoreSpecs)
+				rd := makeRestoreDriver(ctx, t, c, sp.restoreSpecs)
 				rd.prepareCluster(ctx)
 
 				runRestore(ctx, t, c, regRestoreSpecs, rd, false /* runOnline */, true /* runWorkload */)