release: audit unit/integration upgrade tests

[srosenberg]

All unit tests which invoke MakeTestingClusterSettingsWithVersions and pass a non-default binaryMinSupportedVersion (i.e., anything other than clusterversion.TestingBinaryMinSupportedVersion) should be audited. The reason is explained in detail in [1].

TL;DR Before [1], the test cluster would be bootstrapped at the current version unless the test passed BootstrapVersionKeyOverride. Thus, migrations were not being tested other than their idempotency. After [1], the test cluster is correctly bootstrapped at the default binaryMinSupportedVersion unless the test overrides it. This roughly translates to,

NOTE: the above only concerns the unit tests. Roachtests use a different bootstrapping mechanism which ensures an upgrade test is always bootstrapped at some previous version.

• Node is bootstrapped and joins the test cluster at the default binaryMinSupportedVersion
• StartServer uses s.BinaryVersionOverride() to SET CLUSTER SETTING version which triggers migrations from the bootstrapped version to the (overridden) BinaryVersion

Below are all unit tests, grouped by package, which should be audited. This set is conservative. The ones which say FAIL pass a non-default binaryMinSupportedVersion, but may still be correct. The ones which don't say FAIL are likely to be correct, but it wouldn't hurt to double-check them.

pkg/ccl/kvccl/kvtenantccl

@cockroachdb/multi-tenant

TestTenantUpgradeFailure
TestTenantUpgradeInterlock

pkg/ccl/serverccl

@cockroachdb/multi-tenant

TestServerStartupGuardrails (FAIL)
TestBumpTenantClusterVersion (FAIL)
TestValidateTargetTenantClusterVersion (FAIL)

pkg/kv/kvclient/kvcoord

TestBiDirectionalRangefeedNotUsedUntilUpgradeFinalilzed (FAIL)

pkg/kv/kvserver

TestMigrateWaitsForApplication (FAIL)
TestLeaseUpgradeVersionGate (FAIL)
TestRangeMigration (FAIL)
TestLoadBasedRebalancingObjective
TestRebalanceObjectiveManager
TestStoreConfig

pkg/kv/kvserver/batcheval

TestDeclareKeysResolveIntent

pkg/server/settingswatcher

TestVersionGuard

pkg/storage

TestPebbleIterator_ExternalCorruption #100592

pkg/upgrade/upgrademanager

@cockroachdb/sql-schema and @cockroachdb/jobs

TestAlreadyRunningJobsAreHandledProperly - #100830
TestConcurrentMigrationAttempts - #100873
TestMigrateUpdatesReplicaVersion - #100873
TestPauseMigration - #100830

pkg/upgrade/upgrades

@cockroachdb/sql-schema

TestDatabaseRoleSettingsUserIDMigration1500Users (FAIL) seems like a mistake that this is in this list
TestDeleteDescriptorsOfDroppedFunctions (FAIL) #100726
TestExternalConnectionsUserIDMigration10Users (FAIL) seems like a mistake that this is in this list
TestFixUserfileRelatedDescriptorCorruptionUpgrade (FAIL) #100726
TestPreconditionBeforeStartingAnUpgrade (FAIL) #100726
TestMigrationWithFailuresMultipleAltersOnSameColumn fixed by #100644
TestSystemJobInfoMigration #100726 #100696
TestSystemPrivilegesIndexMigration seems like a mistake that this is in this list
TestSystemActivityMigration #100726
TestWaitForDelRangeInGCJob (FAIL) #100726
TestWebSessionsUserIDMigrationNoUsers seems like a mistake that this is in this list

pkg/ccl/backupccl/datadriven_test.go

@cockroachdb/disaster-recovery

testdata/backup-restore/restore-schema-only-mixed-version:new-cluster name=s1 beforeVersion=Start22_2 disable-tenant #100696
testdata/backup-restore/in-progress-import-rollback:new-cluster name=s1 beforeVersion=23_1_MVCCTombstones disable-tenant #100696
testdata/backup-restore/restore-mixed-version:new-cluster name=s1 beforeVersion=Start22_2 disable-tenant #100696
testdata/backup-restore/restore-mixed-version:new-cluster name=s2 beforeVersion=Start22_2 share-io-dir=s1 disable-tenant #100696

[1] #99082

Jira issue: CRDB-26487

[srosenberg]

@cockroachdb/multi-tenant, @cockroachdb/sql-schema, @cockroachdb/storage, @cockroachdb/kv-prs, @cockroachdb/server-prs,@cockroachdb/disaster-recovery

[rafiss]

TestDatabaseRoleSettingsUserIDMigration1500Users is tagged, but it does not call MakeTestingClusterSettingsWithVersions. It does set this though:

				Server: &server.TestingKnobs{
					DisableAutomaticVersionUpgrade: make(chan struct{}),
					BootstrapVersionKeyOverride:    clusterversion.V22_2,
					BinaryVersionOverride:          clusterversion.ByKey(clusterversion.V23_1DatabaseRoleSettingsHasRoleIDColumn - 1),
				},

But so does TestSystemPrivilegesUserIDMigration1500Users, and that's not in this list.

So I was curious how the list was generated?

[srosenberg]

TestDatabaseRoleSettingsUserIDMigration1500Users is tagged, but it does not call MakeTestingClusterSettingsWithVersions.

It used to before this change [1]. When I ran this last night, my local release-23.1 was a few days behind.

[1] 16ff79d#diff-581dd19761966c928337b36%5B%E2%80%A6%5D174592c84a4c44a699816111b8c5

[aliher1911]

TestRangeMigration - is ok, it is using made up versions to verify that below raft migration infrastructure
TestLeaseUpgradeVersionGate - is ok as it only needs version to test version gates
TestMigrateWaitsForApplication - is ok as it is testing migration machinery, not actions.
TestBiDirectionalRangefeedNotUsedUntilUpgradeFinalilzed - is ok, we only need version gate regardless of data.

What I noticed when checking tests is that docs on BinaryVersionOverride explain intricacies of forcing versions settings and bootstrap override while it would be more helpful to explain how to achieve certain goal. e.g. start test cluster with data at old version to test migration, how to start cluster to validate stateless version gate without paying price of actual migration etc.
I think audited tests could have BinaryVersionOverride removed without any ill effects, but I'm not 100% sure and I'm confident they currently test what's expected.

[msbutler]

@srosenberg @adityamaru @rafiss I spent a bit of time trawling though this doc explaining the problem and the lengthy comment on testingKnobs.BinaryVersionOverride, and I'm still not totally clear what the best practice is for mixed version unit test writers. Here's how I guess we ought initialize a test cluster:

tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
		ServerArgs: base.TestServerArgs{
			Knobs: base.TestingKnobs{
				Server: &server.TestingKnobs{
					DisableAutomaticVersionUpgrade: make(chan struct{}),
					BinaryVersionOverride:          clusterversion.TestingBinaryMinSupportedVersion,
					BootstrapVersionKeyOverride:    clusterversion.BinaryMinSupportedVersionKey,
				},
			},
		},
	})

In other words, a unit test writer needs to specify both the binaryVersionOverride and the BootstrapVersionKeyOverride. I'm inferring this from @adityamaru 's TODO:

cockroach/pkg/server/testing_knobs.go

Lines 79 to 81 in 0a0985e

	// TODO(adityamaru): We should force tests that set BinaryVersionOverride to
	// also set BootstrapVersionKeyOverride so as to specify what image they would
	// like the cluster bootstrapped at before upgrading to BinaryVersionOverride.

Could somebody clarify and then update the issue header?

[renatolabs]

Trying to find out what is left to be done here.

@cockroachdb/multi-tenant Have you had the chance to take a look at the tests under pkg/ccl/kvccl/kvtenantccl, pkg/ccl/serverccl, and pkg/server/settingswatcher?

@cockroachdb/storage have you had a chance to look at TestPebbleIterator_ExternalCorruption?

[nicktrav]

The Storage test was updated last week. I've updated the summary with a link to the patch.

[knz]

I checked TestTenantUpgradeInterlock.

For the others we're waiting for @healthy-pod and @ajstorm to be back next week.

[renatolabs]

Everything has been addressed, closing the issue. Thanks everyone for your help!