Merge pull request #140570 from cockroachdb/blathers/backport-release…

…-25.1-140540 release-25.1: upgrade_manager: fix ListBetweenOverride for some tests
cockroachdb · Feb 6, 2025 · 4633169 · 4633169
2 parents 6b3b4fb + 2cf6fa8
commit 4633169
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 13 deletions.
diff --git a/pkg/upgrade/upgradebase/testing_knobs.go b/pkg/upgrade/upgradebase/testing_knobs.go
@@ -27,9 +27,9 @@ const (
 // are useful for testing.
 type TestingKnobs struct {
 
-	// ListBetweenOverride injects an override for `clusterversion.ListBetween()
-	// in order to run upgrades corresponding to versions which do not
-	// actually exist.
+	// ListBetweenOverride injects an override for clusterversion.ListBetween() in
+	// order to run upgrades corresponding to versions which do not actually
+	// exist. This function has to return versions in the range (from, to].
 	ListBetweenOverride func(from, to roachpb.Version) []roachpb.Version
 
 	// RegistryOverride is used to inject upgrades for specific cluster versions.

diff --git a/pkg/upgrade/upgrademanager/manager.go b/pkg/upgrade/upgrademanager/manager.go
@@ -828,7 +828,14 @@ func (m *Manager) getRunningMigrationJob(
 
 func (m *Manager) listBetween(from roachpb.Version, to roachpb.Version) []roachpb.Version {
 	if m.knobs.ListBetweenOverride != nil {
-		return m.knobs.ListBetweenOverride(from, to)
+		result := m.knobs.ListBetweenOverride(from, to)
+		// Sanity check result to catch invalid overrides.
+		for _, v := range result {
+			if v.LessEq(from) || to.Less(v) {
+				panic(fmt.Sprintf("ListBetweenOverride(%s, %s) returned invalid version %s", from, to, v))
+			}
+		}
+		return result
 	}
 	return clusterversion.ListBetween(from, to)
 }

diff --git a/pkg/upgrade/upgrademanager/manager_external_test.go b/pkg/upgrade/upgrademanager/manager_external_test.go
@@ -100,7 +100,7 @@ func TestAlreadyRunningJobsAreHandledProperly(t *testing.T) {
 				},
 				UpgradeManager: &upgradebase.TestingKnobs{
 					ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version {
-						return []roachpb.Version{from, to}
+						return []roachpb.Version{to}
 					},
 					RegistryOverride: func(v roachpb.Version) (upgradebase.Upgrade, bool) {
 						if v != endCV.Version() {
@@ -399,7 +399,7 @@ func TestMigrateUpdatesReplicaVersion(t *testing.T) {
 				},
 				UpgradeManager: &upgradebase.TestingKnobs{
 					ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version {
-						return []roachpb.Version{from, to}
+						return []roachpb.Version{to}
 					},
 					RegistryOverride: func(cv roachpb.Version) (upgradebase.Upgrade, bool) {
 						if cv != endCV {
@@ -504,7 +504,13 @@ func TestConcurrentMigrationAttempts(t *testing.T) {
 				},
 				UpgradeManager: &upgradebase.TestingKnobs{
 					ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version {
-						return versions
+						var res []roachpb.Version
+						for _, v := range versions {
+							if from.Less(v) && v.LessEq(to) {
+								res = append(res, v)
+							}
+						}
+						return res
 					},
 					RegistryOverride: func(cv roachpb.Version) (upgradebase.Upgrade, bool) {
 						return upgrade.NewSystemUpgrade("test", cv, func(
@@ -556,7 +562,7 @@ func TestConcurrentMigrationAttempts(t *testing.T) {
 	for k, c := range migrationRunCounts {
 		require.Equalf(t, 1, c, "version: %v", k)
 	}
-	require.Len(t, migrationRunCounts, len(versions))
+	require.Len(t, migrationRunCounts, len(versions)-1)
 }
 
 // TestPauseMigration ensures that upgrades can indeed be paused and that
@@ -592,7 +598,13 @@ func TestPauseMigration(t *testing.T) {
 				},
 				UpgradeManager: &upgradebase.TestingKnobs{
 					ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version {
-						return []roachpb.Version{from, to}
+						// We expect calls to ListBetween to be made for (0.0, startCV] and (startCV, endCV].
+						if to != startCV.Version() {
+							if from != startCV.Version() || to != endCV.Version() {
+								panic(fmt.Sprintf("unexpected versions %v, %v\n", from, to))
+							}
+						}
+						return []roachpb.Version{to}
 					},
 					RegistryOverride: func(cv roachpb.Version) (upgradebase.Upgrade, bool) {
 						if cv != endCV.Version() {

diff --git a/pkg/upgrade/upgrades/schema_changes_external_test.go b/pkg/upgrade/upgrades/schema_changes_external_test.go
@@ -351,9 +351,7 @@ func testMigrationWithFailures(
 						},
 						UpgradeManager: &upgradebase.TestingKnobs{
 							ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version {
-								return []roachpb.Version{
-									endCV,
-								}
+								return []roachpb.Version{to}
 							},
 							RegistryOverride: func(cv roachpb.Version) (upgradebase.Upgrade, bool) {
 								if cv.Equal(endCV) {
@@ -364,7 +362,7 @@ func testMigrationWithFailures(
 										upgrade.RestoreActionNotRequired("test"),
 									), true
 								}
-								panic("unexpected version")
+								return nil, false
 							}},
 					},
 				},