diff --git a/pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go b/pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go index 55eb13d865f8..2062e8dcdce9 100644 --- a/pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go +++ b/pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go @@ -252,15 +252,14 @@ func TestTenantUpgradeFailure(t *testing.T) { TestingKnobs: base.TestingKnobs{ JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(), UpgradeManager: &upgrade.TestingKnobs{ - ListBetweenOverride: func(from, to clusterversion.ClusterVersion) []clusterversion.ClusterVersion { - return []clusterversion.ClusterVersion{{Version: v1}, {Version: v2}} + ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version { + return []roachpb.Version{v1, v2} }, - RegistryOverride: func(cv clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { - switch cv.Version { + RegistryOverride: func(v roachpb.Version) (upgrade.Upgrade, bool) { + switch v { case v1: - return upgrade.NewTenantUpgrade("testing", clusterversion.ClusterVersion{ - Version: v1, - }, + return upgrade.NewTenantUpgrade("testing", + v1, upgrades.NoPrecondition, func( ctx context.Context, version clusterversion.ClusterVersion, deps upgrade.TenantDeps, @@ -268,9 +267,8 @@ func TestTenantUpgradeFailure(t *testing.T) { return nil }), true case v2: - return upgrade.NewTenantUpgrade("testing next", clusterversion.ClusterVersion{ - Version: v2, - }, + return upgrade.NewTenantUpgrade("testing next", + v2, upgrades.NoPrecondition, func( ctx context.Context, version clusterversion.ClusterVersion, deps upgrade.TenantDeps, diff --git a/pkg/clusterversion/clusterversion.go b/pkg/clusterversion/clusterversion.go index 929d63480ebe..5598fe9f7486 100644 --- a/pkg/clusterversion/clusterversion.go +++ b/pkg/clusterversion/clusterversion.go @@ -265,19 +265,6 @@ func (cv ClusterVersion) SafeFormat(p redact.SafePrinter, _ rune) { p.Print(cv.Version) } -// PrettyPrint returns the value in a format that makes it apparent whether or -// not it is a fence version. -func (cv ClusterVersion) PrettyPrint() string { - // If we're a version greater than v20.2 and have an odd internal version, - // we're a fence version. See fenceVersionFor in pkg/upgrade to understand - // what these are. - fenceVersion := !cv.Version.LessEq(roachpb.Version{Major: 20, Minor: 2}) && (cv.Internal%2) == 1 - if !fenceVersion { - return cv.String() - } - return redact.Sprintf("%s%s", cv.String(), "(fence)").StripMarkers() -} - // ClusterVersionImpl implements the settings.ClusterVersionImpl interface. func (cv ClusterVersion) ClusterVersionImpl() {} diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index 68d7ac407177..76e7f1fb6b5f 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -638,16 +638,16 @@ func ByKey(key Key) roachpb.Version { // ListBetween returns the list of cluster versions in the range // (from, to]. -func ListBetween(from, to ClusterVersion) []ClusterVersion { +func ListBetween(from, to roachpb.Version) []roachpb.Version { return listBetweenInternal(from, to, versionsSingleton) } -func listBetweenInternal(from, to ClusterVersion, vs keyedVersions) []ClusterVersion { - var cvs []ClusterVersion +func listBetweenInternal(from, to roachpb.Version, vs keyedVersions) []roachpb.Version { + var cvs []roachpb.Version for _, keyedV := range vs { // Read: "from < keyedV <= to". - if from.Less(keyedV.Version) && keyedV.Version.LessEq(to.Version) { - cvs = append(cvs, ClusterVersion{Version: keyedV.Version}) + if from.Less(keyedV.Version) && keyedV.Version.LessEq(to) { + cvs = append(cvs, keyedV.Version) } } return cvs diff --git a/pkg/clusterversion/cockroach_versions_test.go b/pkg/clusterversion/cockroach_versions_test.go index 5f73d59f0452..2c98637208f2 100644 --- a/pkg/clusterversion/cockroach_versions_test.go +++ b/pkg/clusterversion/cockroach_versions_test.go @@ -119,27 +119,27 @@ func TestGetVersionsBetween(t *testing.T) { Version: roachpb.Version{Major: int32(i)}, }) } - cv := func(major int32) ClusterVersion { - return ClusterVersion{Version: roachpb.Version{Major: major}} + v := func(major int32) roachpb.Version { + return roachpb.Version{Major: major} } - list := func(first, last int32) []ClusterVersion { - var cvs []ClusterVersion + list := func(first, last int32) []roachpb.Version { + var cvs []roachpb.Version for i := first; i <= last; i++ { - cvs = append(cvs, cv(i)) + cvs = append(cvs, v(i)) } return cvs } var tests = []struct { - from, to ClusterVersion - exp []ClusterVersion + from, to roachpb.Version + exp []roachpb.Version }{ - {cv(5), cv(8), list(6, 8)}, - {cv(1), cv(1), []ClusterVersion{}}, - {cv(7), cv(7), []ClusterVersion{}}, - {cv(1), cv(5), list(3, 5)}, - {cv(6), cv(12), list(7, 9)}, - {cv(4), cv(5), list(5, 5)}, + {v(5), v(8), list(6, 8)}, + {v(1), v(1), []roachpb.Version{}}, + {v(7), v(7), []roachpb.Version{}}, + {v(1), v(5), list(3, 5)}, + {v(6), v(12), list(7, 9)}, + {v(4), v(5), list(5, 5)}, } for _, test := range tests { diff --git a/pkg/roachpb/version.go b/pkg/roachpb/version.go index 8ecf093ce109..9bd037a039d4 100644 --- a/pkg/roachpb/version.go +++ b/pkg/roachpb/version.go @@ -11,6 +11,7 @@ package roachpb import ( + "fmt" "strconv" "strings" @@ -60,6 +61,19 @@ func (v Version) SafeFormat(p redact.SafePrinter, _ rune) { p.Printf("%d.%d-%d", v.Major, v.Minor, v.Internal) } +// PrettyPrint returns the value in a format that makes it apparent whether or +// not it is a fence version. +func (v Version) PrettyPrint() string { + // If we're a version greater than v20.2 and have an odd internal version, + // we're a fence version. See fenceVersionFor in pkg/upgrade to understand + // what these are. + fenceVersion := !v.LessEq(Version{Major: 20, Minor: 2}) && (v.Internal%2) == 1 + if !fenceVersion { + return v.String() + } + return fmt.Sprintf("%v(fence)", v) +} + // ParseVersion parses a Version from a string of the form // ".-" where the "-" is optional. We don't // use the Patch component, so it is always zero. diff --git a/pkg/server/version_cluster_test.go b/pkg/server/version_cluster_test.go index 5fe22cb3b9ac..e53d10e21dcc 100644 --- a/pkg/server/version_cluster_test.go +++ b/pkg/server/version_cluster_test.go @@ -419,16 +419,15 @@ func TestClusterVersionMixedVersionTooOld(t *testing.T) { // Inject an upgrade which would run to upgrade the cluster. // We'll validate that we never create a job for this upgrade. UpgradeManager: &upgrade.TestingKnobs{ - ListBetweenOverride: func(from, to clusterversion.ClusterVersion) []clusterversion.ClusterVersion { - return []clusterversion.ClusterVersion{to} + ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version { + return []roachpb.Version{to} }, - RegistryOverride: func(cv clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { - if !cv.Version.Equal(v1) { + RegistryOverride: func(cv roachpb.Version) (upgrade.Upgrade, bool) { + if !cv.Equal(v1) { return nil, false } - return upgrade.NewTenantUpgrade("testing", clusterversion.ClusterVersion{ - Version: v1, - }, + return upgrade.NewTenantUpgrade("testing", + v1, upgrades.NoPrecondition, func( ctx context.Context, version clusterversion.ClusterVersion, deps upgrade.TenantDeps, diff --git a/pkg/sql/logictest/logic.go b/pkg/sql/logictest/logic.go index 2aa89200b1ae..41b484bdda12 100644 --- a/pkg/sql/logictest/logic.go +++ b/pkg/sql/logictest/logic.go @@ -1342,18 +1342,16 @@ func (t *logicTest) newCluster( // If we're injecting fake versions, hook up logic to simulate the end // version existing. - from := clusterversion.ClusterVersion{Version: cfg.BootstrapVersion} - to := clusterversion.ClusterVersion{Version: cfg.BinaryVersion} - if len(clusterversion.ListBetween(from, to)) == 0 { + if len(clusterversion.ListBetween(cfg.BootstrapVersion, cfg.BinaryVersion)) == 0 { mm, ok := nodeParams.Knobs.UpgradeManager.(*upgrade.TestingKnobs) if !ok { mm = &upgrade.TestingKnobs{} nodeParams.Knobs.UpgradeManager = mm } mm.ListBetweenOverride = func( - from, to clusterversion.ClusterVersion, - ) []clusterversion.ClusterVersion { - return []clusterversion.ClusterVersion{to} + from, to roachpb.Version, + ) []roachpb.Version { + return []roachpb.Version{to} } } } diff --git a/pkg/upgrade/system_upgrade.go b/pkg/upgrade/system_upgrade.go index ef9c69364f12..3f41ba2fff49 100644 --- a/pkg/upgrade/system_upgrade.go +++ b/pkg/upgrade/system_upgrade.go @@ -124,22 +124,18 @@ type SystemUpgrade struct { type SystemUpgradeFunc func(context.Context, clusterversion.ClusterVersion, SystemDeps) error // NewSystemUpgrade constructs a SystemUpgrade. -func NewSystemUpgrade( - description string, cv clusterversion.ClusterVersion, fn SystemUpgradeFunc, -) *SystemUpgrade { +func NewSystemUpgrade(description string, v roachpb.Version, fn SystemUpgradeFunc) *SystemUpgrade { return &SystemUpgrade{ upgrade: upgrade{ description: description, - cv: cv, + v: v, }, fn: fn, } } // Run kickstarts the actual upgrade process for system-level upgrades. -func (m *SystemUpgrade) Run( - ctx context.Context, cv clusterversion.ClusterVersion, d SystemDeps, -) error { - ctx = logtags.AddTag(ctx, fmt.Sprintf("upgrade=%s", cv), nil) - return m.fn(ctx, cv, d) +func (m *SystemUpgrade) Run(ctx context.Context, v roachpb.Version, d SystemDeps) error { + ctx = logtags.AddTag(ctx, fmt.Sprintf("upgrade=%s", v), nil) + return m.fn(ctx, clusterversion.ClusterVersion{Version: v}, d) } diff --git a/pkg/upgrade/tenant_upgrade.go b/pkg/upgrade/tenant_upgrade.go index d10cad2cf8cd..72816b5a4c0f 100644 --- a/pkg/upgrade/tenant_upgrade.go +++ b/pkg/upgrade/tenant_upgrade.go @@ -83,15 +83,12 @@ var _ Upgrade = (*TenantUpgrade)(nil) // NewTenantUpgrade constructs a TenantUpgrade. func NewTenantUpgrade( - description string, - cv clusterversion.ClusterVersion, - precondition PreconditionFunc, - fn TenantUpgradeFunc, + description string, v roachpb.Version, precondition PreconditionFunc, fn TenantUpgradeFunc, ) *TenantUpgrade { m := &TenantUpgrade{ upgrade: upgrade{ description: description, - cv: cv, + v: v, }, fn: fn, precondition: precondition, @@ -100,11 +97,9 @@ func NewTenantUpgrade( } // Run kick-starts the actual upgrade process for tenant-level upgrades. -func (m *TenantUpgrade) Run( - ctx context.Context, cv clusterversion.ClusterVersion, d TenantDeps, -) error { - ctx = logtags.AddTag(ctx, fmt.Sprintf("upgrade=%s", cv), nil) - return m.fn(ctx, cv, d) +func (m *TenantUpgrade) Run(ctx context.Context, v roachpb.Version, d TenantDeps) error { + ctx = logtags.AddTag(ctx, fmt.Sprintf("upgrade=%s", v), nil) + return m.fn(ctx, clusterversion.ClusterVersion{Version: v}, d) } // Precondition runs the precondition check if there is one and reports diff --git a/pkg/upgrade/testing_knobs.go b/pkg/upgrade/testing_knobs.go index e203a1033068..36c6f178eabf 100644 --- a/pkg/upgrade/testing_knobs.go +++ b/pkg/upgrade/testing_knobs.go @@ -12,7 +12,7 @@ package upgrade import ( "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/roachpb" ) // TestingKnobs are knobs to inject behavior into the upgrade manager which @@ -22,10 +22,10 @@ type TestingKnobs struct { // ListBetweenOverride injects an override for `clusterversion.ListBetween() // in order to run upgrades corresponding to versions which do not // actually exist. - ListBetweenOverride func(from, to clusterversion.ClusterVersion) []clusterversion.ClusterVersion + ListBetweenOverride func(from, to roachpb.Version) []roachpb.Version // RegistryOverride is used to inject upgrades for specific cluster versions. - RegistryOverride func(cv clusterversion.ClusterVersion) (Upgrade, bool) + RegistryOverride func(v roachpb.Version) (Upgrade, bool) } // ModuleTestingKnobs makes TestingKnobs a base.ModuleTestingKnobs. diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 52f9fb198a9c..4b57286271b7 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -24,7 +24,7 @@ package upgrade import ( "fmt" - "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/roachpb" ) // Upgrade defines a program to be executed once every node in the cluster is @@ -61,7 +61,7 @@ import ( // [3]: truncatedStateMigration // [4]: pkg/kv/kvserver/batch_eval/cmd_migrate.go type Upgrade interface { - ClusterVersion() clusterversion.ClusterVersion + Version() roachpb.Version Name() string internal() // restrict implementations to this package } @@ -72,7 +72,7 @@ type JobDeps interface { // GetUpgrade returns the upgrade associated with the cluster version // if one exists. - GetUpgrade(key clusterversion.ClusterVersion) (Upgrade, bool) + GetUpgrade(key roachpb.Version) (Upgrade, bool) // SystemDeps returns a handle to upgrade dependencies on a system tenant. SystemDeps() SystemDeps @@ -80,17 +80,17 @@ type JobDeps interface { type upgrade struct { description string - cv clusterversion.ClusterVersion + v roachpb.Version } // ClusterVersion makes SystemUpgrade an Upgrade. -func (m *upgrade) ClusterVersion() clusterversion.ClusterVersion { - return m.cv +func (m *upgrade) Version() roachpb.Version { + return m.v } // Name returns a human-readable name for this upgrade. func (m *upgrade) Name() string { - return fmt.Sprintf("Upgrade to %s: %q", m.cv.String(), m.description) + return fmt.Sprintf("Upgrade to %s: %q", m.v.String(), m.description) } func (m *upgrade) internal() {} diff --git a/pkg/upgrade/upgradejob/BUILD.bazel b/pkg/upgrade/upgradejob/BUILD.bazel index 8396f9335730..489a9062251a 100644 --- a/pkg/upgrade/upgradejob/BUILD.bazel +++ b/pkg/upgrade/upgradejob/BUILD.bazel @@ -11,6 +11,7 @@ go_library( "//pkg/jobs", "//pkg/jobs/jobspb", "//pkg/kv", + "//pkg/roachpb", "//pkg/security/username", "//pkg/settings/cluster", "//pkg/sql", diff --git a/pkg/upgrade/upgradejob/upgrade_job.go b/pkg/upgrade/upgradejob/upgrade_job.go index b56880fff4da..c03b11316aa0 100644 --- a/pkg/upgrade/upgradejob/upgrade_job.go +++ b/pkg/upgrade/upgradejob/upgrade_job.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" @@ -43,13 +44,11 @@ func init() { } // NewRecord constructs a new jobs.Record for this upgrade. -func NewRecord( - version clusterversion.ClusterVersion, user username.SQLUsername, name string, -) jobs.Record { +func NewRecord(version roachpb.Version, user username.SQLUsername, name string) jobs.Record { return jobs.Record{ Description: name, Details: jobspb.MigrationDetails{ - ClusterVersion: &version, + ClusterVersion: &clusterversion.ClusterVersion{Version: version}, }, Username: user, Progress: jobspb.MigrationProgress{}, @@ -66,15 +65,15 @@ var _ jobs.Resumer = (*resumer)(nil) func (r resumer) Resume(ctx context.Context, execCtxI interface{}) error { execCtx := execCtxI.(sql.JobExecContext) pl := r.j.Payload() - cv := *pl.GetMigration().ClusterVersion + v := pl.GetMigration().ClusterVersion.Version ie := execCtx.ExecCfg().InternalExecutor - alreadyCompleted, err := CheckIfMigrationCompleted(ctx, nil /* txn */, ie, cv) + alreadyCompleted, err := CheckIfMigrationCompleted(ctx, nil /* txn */, ie, v) if alreadyCompleted || err != nil { - return errors.Wrapf(err, "checking migration completion for %v", cv) + return errors.Wrapf(err, "checking migration completion for %v", v) } mc := execCtx.MigrationJobDeps() - m, ok := mc.GetUpgrade(cv) + m, ok := mc.GetUpgrade(v) if !ok { // TODO(ajwerner): Consider treating this as an assertion failure. Jobs // should only be created for a cluster version if there is an associated @@ -86,7 +85,7 @@ func (r resumer) Resume(ctx context.Context, execCtxI interface{}) error { } switch m := m.(type) { case *upgrade.SystemUpgrade: - err = m.Run(ctx, cv, mc.SystemDeps()) + err = m.Run(ctx, v, mc.SystemDeps()) case *upgrade.TenantUpgrade: tenantDeps := upgrade.TenantDeps{ DB: execCtx.ExecCfg().DB, @@ -122,18 +121,18 @@ func (r resumer) Resume(ctx context.Context, execCtxI interface{}) error { return sr, cleanup, nil } - err = m.Run(ctx, cv, tenantDeps) + err = m.Run(ctx, v, tenantDeps) default: return errors.AssertionFailedf("unknown migration type %T", m) } if err != nil { - return errors.Wrapf(err, "running migration for %v", cv) + return errors.Wrapf(err, "running migration for %v", v) } // Mark the upgrade as having been completed so that subsequent iterations // no-op and new jobs are not created. - if err := markMigrationCompleted(ctx, ie, cv); err != nil { - return errors.Wrapf(err, "marking migration complete for %v", cv) + if err := markMigrationCompleted(ctx, ie, v); err != nil { + return errors.Wrapf(err, "marking migration complete for %v", v) } return nil } @@ -143,7 +142,7 @@ func (r resumer) Resume(ctx context.Context, execCtxI interface{}) error { // The txn may be nil, in which case the check will be run in its own // transaction. func CheckIfMigrationCompleted( - ctx context.Context, txn *kv.Txn, ie sqlutil.InternalExecutor, cv clusterversion.ClusterVersion, + ctx context.Context, txn *kv.Txn, ie sqlutil.InternalExecutor, v roachpb.Version, ) (alreadyCompleted bool, _ error) { row, err := ie.QueryRow( ctx, @@ -159,10 +158,10 @@ SELECT EXISTS( AND internal = $4 ); `, - cv.Major, - cv.Minor, - cv.Patch, - cv.Internal) + v.Major, + v.Minor, + v.Patch, + v.Internal) if err != nil { return false, err } @@ -170,7 +169,7 @@ SELECT EXISTS( } func markMigrationCompleted( - ctx context.Context, ie sqlutil.InternalExecutor, cv clusterversion.ClusterVersion, + ctx context.Context, ie sqlutil.InternalExecutor, v roachpb.Version, ) error { _, err := ie.ExecEx( ctx, @@ -188,10 +187,10 @@ INSERT completed_at ) VALUES ($1, $2, $3, $4, $5)`, - cv.Major, - cv.Minor, - cv.Patch, - cv.Internal, + v.Major, + v.Minor, + v.Patch, + v.Internal, timeutil.Now()) return err } diff --git a/pkg/upgrade/upgrademanager/BUILD.bazel b/pkg/upgrade/upgrademanager/BUILD.bazel index 84cf807a644d..543717c21d46 100644 --- a/pkg/upgrade/upgrademanager/BUILD.bazel +++ b/pkg/upgrade/upgrademanager/BUILD.bazel @@ -12,6 +12,7 @@ go_library( "//pkg/jobs/jobspb", "//pkg/keys", "//pkg/kv", + "//pkg/roachpb", "//pkg/security/username", "//pkg/server/serverpb", "//pkg/server/settingswatcher", diff --git a/pkg/upgrade/upgrademanager/manager.go b/pkg/upgrade/upgrademanager/manager.go index 37c630ffb954..cffe667d41eb 100644 --- a/pkg/upgrade/upgrademanager/manager.go +++ b/pkg/upgrade/upgrademanager/manager.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/settingswatcher" @@ -54,7 +55,7 @@ type Manager struct { } // GetUpgrade returns the upgrade associated with this key. -func (m *Manager) GetUpgrade(key clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { +func (m *Manager) GetUpgrade(key roachpb.Version) (upgrade.Upgrade, bool) { if m.knobs.RegistryOverride != nil { if m, ok := m.knobs.RegistryOverride(key); ok { return m, ok @@ -175,7 +176,7 @@ func (m *Manager) Migrate( return err } - clusterVersions := m.listBetween(from, to) + clusterVersions := m.listBetween(from.Version, to.Version) log.Infof(ctx, "migrating cluster from %s to %s (stepping through %s)", from, to, clusterVersions) if len(clusterVersions) == 0 { return nil @@ -186,7 +187,7 @@ func (m *Manager) Migrate( // that might be doomed to fail. { finalVersion := clusterVersions[len(clusterVersions)-1] - if err := validateTargetClusterVersion(ctx, m.deps.Cluster, finalVersion); err != nil { + if err := validateTargetClusterVersion(ctx, m.deps.Cluster, clusterversion.ClusterVersion{Version: finalVersion}); err != nil { return err } } @@ -197,6 +198,7 @@ func (m *Manager) Migrate( for _, clusterVersion := range clusterVersions { log.Infof(ctx, "stepping through %s", clusterVersion) + cv := clusterversion.ClusterVersion{Version: clusterVersion} // First, run the actual upgrade if any. if err := m.runMigration(ctx, user, clusterVersion); err != nil { return err @@ -281,7 +283,7 @@ func (m *Manager) Migrate( // can join the cluster will run a release that support the fence // version, and by design also supports the actual version (which is // the direct successor of the fence). - fenceVersion := upgrade.FenceVersionFor(ctx, clusterVersion) + fenceVersion := upgrade.FenceVersionFor(ctx, cv) if err := bumpClusterVersion(ctx, m.deps.Cluster, fenceVersion); err != nil { return err } @@ -289,18 +291,18 @@ func (m *Manager) Migrate( // Now sanity check that we'll actually be able to perform the real // cluster version bump, cluster-wide. - if err := validateTargetClusterVersion(ctx, m.deps.Cluster, clusterVersion); err != nil { + if err := validateTargetClusterVersion(ctx, m.deps.Cluster, cv); err != nil { return err } // Finally, bump the real version cluster-wide. - err := bumpClusterVersion(ctx, m.deps.Cluster, clusterVersion) + err := bumpClusterVersion(ctx, m.deps.Cluster, cv) if err != nil { return err } // Bump up the cluster version for tenants, which // will bump over individual version bumps. - err = updateSystemVersionSetting(ctx, clusterVersion) + err = updateSystemVersionSetting(ctx, cv) if err != nil { return err } @@ -351,7 +353,7 @@ func forEveryNodeUntilClusterStable( } func (m *Manager) runMigration( - ctx context.Context, user username.SQLUsername, version clusterversion.ClusterVersion, + ctx context.Context, user username.SQLUsername, version roachpb.Version, ) error { mig, exists := m.GetUpgrade(version) if !exists { @@ -369,10 +371,7 @@ func (m *Manager) runMigration( } func (m *Manager) getOrCreateMigrationJob( - ctx context.Context, - user username.SQLUsername, - version clusterversion.ClusterVersion, - name string, + ctx context.Context, user username.SQLUsername, version roachpb.Version, name string, ) (alreadyCompleted bool, jobID jobspb.JobID, _ error) { newJobID := m.jr.MakeJobID() if err := m.deps.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) { @@ -401,8 +400,11 @@ func (m *Manager) getOrCreateMigrationJob( } func (m *Manager) getRunningMigrationJob( - ctx context.Context, txn *kv.Txn, version clusterversion.ClusterVersion, + ctx context.Context, txn *kv.Txn, version roachpb.Version, ) (found bool, jobID jobspb.JobID, _ error) { + // Wrap the version into a ClusterVersion so that the JSON looks like what the + // Payload proto has inside. + cv := clusterversion.ClusterVersion{Version: version} const query = ` SELECT id, status FROM ( @@ -417,7 +419,7 @@ SELECT id, status WHERE status IN ` + jobs.NonTerminalStatusTupleString + ` ) WHERE pl->'migration'->'clusterVersion' = $1::JSON;` - jsonMsg, err := protoreflect.MessageToJSON(&version, protoreflect.FmtFlags{EmitDefaults: false}) + jsonMsg, err := protoreflect.MessageToJSON(&cv, protoreflect.FmtFlags{EmitDefaults: false}) if err != nil { return false, 0, errors.Wrap(err, "failed to marshal version to JSON") } @@ -451,9 +453,7 @@ SELECT id, status } } -func (m *Manager) listBetween( - from clusterversion.ClusterVersion, to clusterversion.ClusterVersion, -) []clusterversion.ClusterVersion { +func (m *Manager) listBetween(from roachpb.Version, to roachpb.Version) []roachpb.Version { if m.knobs.ListBetweenOverride != nil { return m.knobs.ListBetweenOverride(from, to) } @@ -462,9 +462,7 @@ func (m *Manager) listBetween( // checkPreconditions runs the precondition check for each tenant upgrade // associated with the provided versions. -func (m *Manager) checkPreconditions( - ctx context.Context, versions []clusterversion.ClusterVersion, -) error { +func (m *Manager) checkPreconditions(ctx context.Context, versions []roachpb.Version) error { for _, v := range versions { mig, ok := m.GetUpgrade(v) if !ok { @@ -474,7 +472,7 @@ func (m *Manager) checkPreconditions( if !ok { continue } - if err := tm.Precondition(ctx, v, upgrade.TenantDeps{ + if err := tm.Precondition(ctx, clusterversion.ClusterVersion{Version: v}, upgrade.TenantDeps{ DB: m.deps.DB, Codec: m.codec, Settings: m.settings, diff --git a/pkg/upgrade/upgrademanager/manager_external_test.go b/pkg/upgrade/upgrademanager/manager_external_test.go index 345ecd0e7c29..db988bde6693 100644 --- a/pkg/upgrade/upgrademanager/manager_external_test.go +++ b/pkg/upgrade/upgrademanager/manager_external_test.go @@ -57,8 +57,8 @@ func TestAlreadyRunningJobsAreHandledProperly(t *testing.T) { defer log.Scope(t).Close(t) // We're going to be migrating from startCV to endCV. - startCV := clusterversion.ClusterVersion{Version: roachpb.Version{Major: 41}} - endCV := clusterversion.ClusterVersion{Version: roachpb.Version{Major: 42}} + startCV := roachpb.Version{Major: 41} + endCV := roachpb.Version{Major: 42} ch := make(chan chan error) @@ -66,10 +66,10 @@ func TestAlreadyRunningJobsAreHandledProperly(t *testing.T) { tc := testcluster.StartTestCluster(t, 2, base.TestClusterArgs{ ReplicationMode: base.ReplicationManual, ServerArgs: base.TestServerArgs{ - Settings: cluster.MakeTestingClusterSettingsWithVersions(endCV.Version, startCV.Version, false), + Settings: cluster.MakeTestingClusterSettingsWithVersions(endCV, startCV, false), Knobs: base.TestingKnobs{ Server: &server.TestingKnobs{ - BinaryVersionOverride: startCV.Version, + BinaryVersionOverride: startCV, DisableAutomaticVersionUpgrade: make(chan struct{}), }, DistSQL: &execinfra.TestingKnobs{ @@ -77,14 +77,14 @@ func TestAlreadyRunningJobsAreHandledProperly(t *testing.T) { ProcessorNoTracingSpan: true, }, UpgradeManager: &upgrade.TestingKnobs{ - ListBetweenOverride: func(from, to clusterversion.ClusterVersion) []clusterversion.ClusterVersion { - return []clusterversion.ClusterVersion{to} + ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version { + return []roachpb.Version{to} }, - RegistryOverride: func(cv clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { - if cv != endCV { + RegistryOverride: func(v roachpb.Version) (upgrade.Upgrade, bool) { + if v != endCV { return nil, false } - return upgrade.NewTenantUpgrade("test", cv, upgrades.NoPrecondition, func( + return upgrade.NewTenantUpgrade("test", v, upgrades.NoPrecondition, func( ctx context.Context, version clusterversion.ClusterVersion, deps upgrade.TenantDeps, ) error { canResume := make(chan error) @@ -209,32 +209,32 @@ func TestMigrateUpdatesReplicaVersion(t *testing.T) { defer log.Scope(t).Close(t) // We're going to be migrating from startCV to endCV. - startCV := clusterversion.ClusterVersion{Version: roachpb.Version{Major: 41}} - endCV := clusterversion.ClusterVersion{Version: roachpb.Version{Major: 42}} + startCV := roachpb.Version{Major: 41} + endCV := roachpb.Version{Major: 42} var desc roachpb.RangeDescriptor ctx := context.Background() tc := testcluster.StartTestCluster(t, 2, base.TestClusterArgs{ ReplicationMode: base.ReplicationManual, ServerArgs: base.TestServerArgs{ - Settings: cluster.MakeTestingClusterSettingsWithVersions(endCV.Version, startCV.Version, false), + Settings: cluster.MakeTestingClusterSettingsWithVersions(endCV, startCV, false), Knobs: base.TestingKnobs{ Server: &server.TestingKnobs{ - BinaryVersionOverride: startCV.Version, + BinaryVersionOverride: startCV, DisableAutomaticVersionUpgrade: make(chan struct{}), }, UpgradeManager: &upgrade.TestingKnobs{ - ListBetweenOverride: func(from, to clusterversion.ClusterVersion) []clusterversion.ClusterVersion { - return []clusterversion.ClusterVersion{from, to} + ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version { + return []roachpb.Version{from, to} }, - RegistryOverride: func(cv clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { + RegistryOverride: func(cv roachpb.Version) (upgrade.Upgrade, bool) { if cv != endCV { return nil, false } return upgrade.NewSystemUpgrade("test", cv, func( ctx context.Context, version clusterversion.ClusterVersion, d upgrade.SystemDeps, ) error { - return d.DB.Migrate(ctx, desc.StartKey, desc.EndKey, cv.Version) + return d.DB.Migrate(ctx, desc.StartKey, desc.EndKey, cv) }), true }, }, @@ -243,7 +243,7 @@ func TestMigrateUpdatesReplicaVersion(t *testing.T) { }) defer tc.Stopper().Stop(ctx) // RegisterKVMigration the below raft upgrade. - unregisterKVMigration := batcheval.TestingRegisterMigrationInterceptor(endCV.Version, func() {}) + unregisterKVMigration := batcheval.TestingRegisterMigrationInterceptor(endCV, func() {}) defer unregisterKVMigration() // We'll take a specific range, still running at startCV, generate an @@ -268,8 +268,8 @@ func TestMigrateUpdatesReplicaVersion(t *testing.T) { repl, err := store.GetReplica(rangeID) require.NoError(t, err) - if got := repl.Version(); got != startCV.Version { - t.Fatalf("got replica version %s, expected %s", got, startCV.Version) + if got := repl.Version(); got != startCV { + t.Fatalf("got replica version %s, expected %s", got, startCV) } // Wait until all nodes have are considered live. @@ -292,8 +292,8 @@ func TestMigrateUpdatesReplicaVersion(t *testing.T) { _, err = tc.Conns[0].ExecContext(ctx, `SET CLUSTER SETTING version = $1`, endCV.String()) require.NoError(t, err) - if got := repl.Version(); got != endCV.Version { - t.Fatalf("got replica version %s, expected %s", got, endCV.Version) + if got := repl.Version(); got != endCV { + t.Fatalf("got replica version %s, expected %s", got, endCV) } } @@ -317,14 +317,10 @@ func TestConcurrentMigrationAttempts(t *testing.T) { // RegisterKVMigration the upgrades to update the map with run counts. // There should definitely not be any concurrency of execution, so the race // detector should not fire. - var versions []clusterversion.ClusterVersion + var versions []roachpb.Version for major := int32(startMajor); major <= endMajor; major++ { - versions = append(versions, clusterversion.ClusterVersion{ - Version: roachpb.Version{ - Major: major, - }, - }) + versions = append(versions, roachpb.Version{Major: major}) } ctx := context.Background() var active int32 // used to detect races @@ -332,20 +328,20 @@ func TestConcurrentMigrationAttempts(t *testing.T) { ReplicationMode: base.ReplicationManual, ServerArgs: base.TestServerArgs{ Settings: cluster.MakeTestingClusterSettingsWithVersions( - versions[len(versions)-1].Version, - versions[0].Version, + versions[len(versions)-1], + versions[0], false, ), Knobs: base.TestingKnobs{ Server: &server.TestingKnobs{ - BinaryVersionOverride: versions[0].Version, + BinaryVersionOverride: versions[0], DisableAutomaticVersionUpgrade: make(chan struct{}), }, UpgradeManager: &upgrade.TestingKnobs{ - ListBetweenOverride: func(from, to clusterversion.ClusterVersion) []clusterversion.ClusterVersion { + ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version { return versions }, - RegistryOverride: func(cv clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { + RegistryOverride: func(cv roachpb.Version) (upgrade.Upgrade, bool) { return upgrade.NewSystemUpgrade("test", cv, func( ctx context.Context, version clusterversion.ClusterVersion, d upgrade.SystemDeps, ) error { @@ -404,8 +400,8 @@ func TestPauseMigration(t *testing.T) { defer log.Scope(t).Close(t) // We're going to be migrating from startCV to endCV. - startCV := clusterversion.ClusterVersion{Version: roachpb.Version{Major: 41}} - endCV := clusterversion.ClusterVersion{Version: roachpb.Version{Major: 42}} + startCV := roachpb.Version{Major: 41} + endCV := roachpb.Version{Major: 42} type migrationEvent struct { unblock chan<- error @@ -416,18 +412,18 @@ func TestPauseMigration(t *testing.T) { tc := testcluster.StartTestCluster(t, 2, base.TestClusterArgs{ ReplicationMode: base.ReplicationManual, ServerArgs: base.TestServerArgs{ - Settings: cluster.MakeTestingClusterSettingsWithVersions(endCV.Version, startCV.Version, false), + Settings: cluster.MakeTestingClusterSettingsWithVersions(endCV, startCV, false), Knobs: base.TestingKnobs{ JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(), Server: &server.TestingKnobs{ - BinaryVersionOverride: startCV.Version, + BinaryVersionOverride: startCV, DisableAutomaticVersionUpgrade: make(chan struct{}), }, UpgradeManager: &upgrade.TestingKnobs{ - ListBetweenOverride: func(from, to clusterversion.ClusterVersion) []clusterversion.ClusterVersion { - return []clusterversion.ClusterVersion{to} + ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version { + return []roachpb.Version{to} }, - RegistryOverride: func(cv clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { + RegistryOverride: func(cv roachpb.Version) (upgrade.Upgrade, bool) { if cv != endCV { return nil, false } @@ -516,14 +512,14 @@ func TestPrecondition(t *testing.T) { // Start by running v0. We want the precondition of v1 to prevent // us from reaching v1 (or v2). We want the precondition to not be // run when migrating from v1 to v2. - next := func(version clusterversion.ClusterVersion) clusterversion.ClusterVersion { + next := func(version roachpb.Version) roachpb.Version { version.Internal += 2 return version } - v0 := clusterversion.ClusterVersion{Version: clusterversion.ByKey(clusterversion.V22_1)} + v0 := clusterversion.ByKey(clusterversion.V22_1) v1 := next(v0) v2 := next(v1) - versions := []clusterversion.ClusterVersion{v0, v1, v2} + versions := []roachpb.Version{v0, v1, v2} var migrationRun, preconditionRun int64 var preconditionErr, migrationErr atomic.Value preconditionErr.Store(true) @@ -542,17 +538,17 @@ func TestPrecondition(t *testing.T) { knobs := base.TestingKnobs{ Server: &server.TestingKnobs{ DisableAutomaticVersionUpgrade: make(chan struct{}), - BinaryVersionOverride: v0.Version, + BinaryVersionOverride: v0, }, // Inject an upgrade which would run to upgrade the cluster. // We'll validate that we never create a job for this upgrade. UpgradeManager: &upgrade.TestingKnobs{ - ListBetweenOverride: func(from, to clusterversion.ClusterVersion) []clusterversion.ClusterVersion { - start := sort.Search(len(versions), func(i int) bool { return from.Less(versions[i].Version) }) - end := sort.Search(len(versions), func(i int) bool { return to.Less(versions[i].Version) }) + ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version { + start := sort.Search(len(versions), func(i int) bool { return from.Less(versions[i]) }) + end := sort.Search(len(versions), func(i int) bool { return to.Less(versions[i]) }) return versions[start:end] }, - RegistryOverride: func(cv clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { + RegistryOverride: func(cv roachpb.Version) (upgrade.Upgrade, bool) { switch cv { case v1: return upgrade.NewTenantUpgrade("v1", cv, @@ -579,9 +575,9 @@ func TestPrecondition(t *testing.T) { return base.TestServerArgs{ Knobs: knobs, Settings: cluster.MakeTestingClusterSettingsWithVersions( - v2.Version, // binaryVersion - v0.Version, // binaryMinSupportedVersion - false, // initializeVersion + v2, // binaryVersion + v0, // binaryMinSupportedVersion + false, // initializeVersion ), } } @@ -592,9 +588,9 @@ func TestPrecondition(t *testing.T) { 2: args(), }, }) - checkActiveVersion := func(t *testing.T, exp clusterversion.ClusterVersion) { + checkActiveVersion := func(t *testing.T, exp roachpb.Version) { for i := 0; i < tc.NumServers(); i++ { - got := tc.Server(i).ClusterSettings().Version.ActiveVersion(ctx) + got := tc.Server(i).ClusterSettings().Version.ActiveVersion(ctx).Version require.Equalf(t, exp, got, "server %d", i) } } diff --git a/pkg/upgrade/upgrades/BUILD.bazel b/pkg/upgrade/upgrades/BUILD.bazel index c6ca851fd97d..fdb7d125fe4f 100644 --- a/pkg/upgrade/upgrades/BUILD.bazel +++ b/pkg/upgrade/upgrades/BUILD.bazel @@ -35,6 +35,7 @@ go_library( "//pkg/jobs/jobspb", "//pkg/keys", "//pkg/kv", + "//pkg/roachpb", "//pkg/security/username", "//pkg/settings/cluster", "//pkg/sql", diff --git a/pkg/upgrade/upgrades/schema_changes_external_test.go b/pkg/upgrade/upgrades/schema_changes_external_test.go index d9e75546ceac..fa92f271cc6d 100644 --- a/pkg/upgrade/upgrades/schema_changes_external_test.go +++ b/pkg/upgrade/upgrades/schema_changes_external_test.go @@ -242,8 +242,8 @@ func testMigrationWithFailures( skip.UnderRace(t, "very slow") // We're going to be migrating from startCV to endCV. - startCV := clusterversion.ClusterVersion{Version: roachpb.Version{Major: 2041}} - endCV := clusterversion.ClusterVersion{Version: roachpb.Version{Major: 2042}} + startCV := roachpb.Version{Major: 2041} + endCV := roachpb.Version{Major: 2042} // The tests follows the following procedure. // @@ -321,10 +321,10 @@ func testMigrationWithFailures( // Number of schema-change jobs that are skipped. settings := cluster.MakeTestingClusterSettingsWithVersions( - endCV.Version, startCV.Version, false, /* initializeVersion */ + endCV, startCV, false, /* initializeVersion */ ) require.NoError(t, clusterversion.Initialize( - ctx, startCV.Version, &settings.SV, + ctx, startCV, &settings.SV, )) jobsKnobs := jobs.NewTestingKnobsWithShortIntervals() jobsKnobs.BeforeUpdate = beforeUpdate @@ -335,7 +335,7 @@ func testMigrationWithFailures( Knobs: base.TestingKnobs{ Server: &server.TestingKnobs{ DisableAutomaticVersionUpgrade: make(chan struct{}), - BinaryVersionOverride: startCV.Version, + BinaryVersionOverride: startCV, }, JobsTestingKnobs: jobsKnobs, SQLExecutor: &sql.ExecutorTestingKnobs{ @@ -349,12 +349,12 @@ func testMigrationWithFailures( }, }, UpgradeManager: &upgrade.TestingKnobs{ - ListBetweenOverride: func(from, to clusterversion.ClusterVersion) []clusterversion.ClusterVersion { - return []clusterversion.ClusterVersion{ + ListBetweenOverride: func(from, to roachpb.Version) []roachpb.Version { + return []roachpb.Version{ endCV, } }, - RegistryOverride: func(cv clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { + RegistryOverride: func(cv roachpb.Version) (upgrade.Upgrade, bool) { if cv.Equal(endCV) { return upgrade.NewTenantUpgrade("testing", endCV, @@ -402,7 +402,7 @@ func testMigrationWithFailures( // Channel to wait for the migration job to complete. finishChan := make(chan struct{}) go upgrades.UpgradeToVersion( - t, sqlDB, endCV.Version, finishChan, true, /* expectError */ + t, sqlDB, endCV, finishChan, true, /* expectError */ ) var migJobID jobspb.JobID @@ -465,7 +465,7 @@ func testMigrationWithFailures( // Restart the migration job. t.Log("retrying migration, expecting to succeed") - go upgrades.UpgradeToVersion(t, sqlDB, endCV.Version, finishChan, false /* expectError */) + go upgrades.UpgradeToVersion(t, sqlDB, endCV, finishChan, false /* expectError */) // Wait until the new migration job observes an existing mutation job. if test.waitForMigrationRestart { diff --git a/pkg/upgrade/upgrades/upgrades.go b/pkg/upgrade/upgrades/upgrades.go index 8cba6da328e8..3a9fde899100 100644 --- a/pkg/upgrade/upgrades/upgrades.go +++ b/pkg/upgrade/upgrades/upgrades.go @@ -18,13 +18,14 @@ import ( "context" "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/upgrade" "github.com/cockroachdb/errors" ) // GetUpgrade returns the upgrade corresponding to this version if // one exists. -func GetUpgrade(key clusterversion.ClusterVersion) (upgrade.Upgrade, bool) { +func GetUpgrade(key roachpb.Version) (upgrade.Upgrade, bool) { m, ok := registry[key] return m, ok } @@ -42,7 +43,7 @@ func NoTenantUpgradeFunc(context.Context, clusterversion.ClusterVersion, upgrade // registry defines the global mapping between a cluster version and the // associated upgrade. The upgrade is only executed after a cluster-wide // bump of the corresponding version gate. -var registry = make(map[clusterversion.ClusterVersion]upgrade.Upgrade) +var registry = make(map[roachpb.Version]upgrade.Upgrade) var upgrades = []upgrade.Upgrade{ upgrade.NewTenantUpgrade( @@ -180,15 +181,13 @@ var upgrades = []upgrade.Upgrade{ func init() { for _, m := range upgrades { - if _, exists := registry[m.ClusterVersion()]; exists { - panic(errors.AssertionFailedf("duplicate upgrade registration for %v", m.ClusterVersion())) + if _, exists := registry[m.Version()]; exists { + panic(errors.AssertionFailedf("duplicate upgrade registration for %v", m.Version())) } - registry[m.ClusterVersion()] = m + registry[m.Version()] = m } } -func toCV(key clusterversion.Key) clusterversion.ClusterVersion { - return clusterversion.ClusterVersion{ - Version: clusterversion.ByKey(key), - } +func toCV(key clusterversion.Key) roachpb.Version { + return clusterversion.ByKey(key) }