Merge cockroachdb#116791

116791: tabledesc_test: deflake TestIndexDoesNotStorePrimaryKeyColumnMixedVersion r=rafiss a=Xiang-Gu Previously, this test would flake under `--stress` and the root cause is because the upgrade precondition check that it relies on actually checks for corrupt descriptor at AOST '-10s', and it's possible that the corrupt descriptor was injected within the last 10s to the system, and thus the check produced a "false negative" (meaning it saw there isn't no corruption at AOST '-10s' and mistakenly assumed there isn't corruption now) and thus mistakenly allowed the cluster upgrade to proceed. Fixes cockroachdb#116622 Release note: None Co-authored-by: Xiang Gu <[email protected]>
itsbilal · Dec 20, 2023 · bab4160 · bab4160
2 parents d8fefa0 + de78ad1
commit bab4160
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 4 deletions.
diff --git a/pkg/sql/catalog/tabledesc/BUILD.bazel b/pkg/sql/catalog/tabledesc/BUILD.bazel
@@ -118,8 +118,8 @@ go_test(
         "//pkg/sql/types",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
-        "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
+        "//pkg/upgrade/upgrades",
         "//pkg/util/hlc",
         "//pkg/util/leaktest",
         "//pkg/util/log",

diff --git a/pkg/sql/catalog/tabledesc/validate_version_gating_test.go b/pkg/sql/catalog/tabledesc/validate_version_gating_test.go
@@ -25,8 +25,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
-	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/upgrade/upgrades"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -54,7 +54,6 @@ import (
 func TestIndexDoesNotStorePrimaryKeyColumnMixedVersion(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-	skip.UnderStressRace(t)
 
 	// Start a test cluster whose cluster version is MinSupported and auto-upgrade is
 	// disabled.
@@ -115,6 +114,8 @@ func TestIndexDoesNotStorePrimaryKeyColumnMixedVersion(t *testing.T) {
 
 	// Assert cluster version upgrade is blocked.
 	require.Equal(t, [][]string{{"1000023.1"}}, tdb.QueryStr(t, "SHOW CLUSTER SETTING version;"))
+	// disable AOST for upgrade precondition check to ensure it sees the injected, corrupt descriptor.
+	defer upgrades.TestingSetFirstUpgradePreconditionAOST(false)()
 	_, err := sqlDB.Exec(`SET CLUSTER SETTING version = $1`, clusterversion.Latest.String())
 	require.ErrorContains(t, err, `verifying precondition for version 1000023.1-upgrading-to-1000023.2-step-002: "".crdb_internal.invalid_objects is not empty`)
 }

diff --git a/pkg/upgrade/upgrades/first_upgrade.go b/pkg/upgrade/upgrades/first_upgrade.go
@@ -97,6 +97,16 @@ func upgradeDescriptors(
 	})
 }
 
+var firstUpgradePreconditionUsesAOST = true
+
+// TestingSetFirstUpgradePreconditionAOST allows tests to disable withAOST trick in upgrade
+// precondition check, which otherwise can produce false negative.
+func TestingSetFirstUpgradePreconditionAOST(enabled bool) func() {
+	old := firstUpgradePreconditionUsesAOST
+	firstUpgradePreconditionUsesAOST = enabled
+	return func() { firstUpgradePreconditionUsesAOST = old }
+}
+
 // FirstUpgradeFromReleasePrecondition is the precondition check for upgrading
 // from any supported major release.
 //
@@ -118,7 +128,7 @@ func FirstUpgradeFromReleasePrecondition(
 	// a diagnostic query. If no corruptions were found back then, we assume that
 	// there are no corruptions now. Otherwise, we retry and do everything
 	// without an AOST clause henceforth.
-	withAOST := true
+	withAOST := firstUpgradePreconditionUsesAOST
 	diagnose := func(tbl string) (hasRows bool, err error) {
 		q := fmt.Sprintf("SELECT count(*) FROM \"\".crdb_internal.%s", tbl)
 		if withAOST {