clusterversion: change string for upgrade versions

This change concerns cluster versions during upgrade. When starting an
upgrade from e.g. 23.3 to 24.1, we start with the cluster version 23.2
and then we go through a sequence of internal versions associated with
various upgrade steps. These versions are presented as `23.2-x`, e.g.
`23.2-8`.

This formatting doesn't make it clear what this version represents. It
can also be confusing - `23.2-8` looks very close to `23.2.8` which
might be an actual CockroachDB version.

This change renames versions during upgrade:
`23.2-upgrading-to-24.1-step-008`. The internal part is always
formatted to three digits (this is intended to reduce the chance of
confusing the internal part to a patch release).

Informs: #112629
Release note (general change): Versions during upgrades are renamed,
for example `23.2-8` is now `23.2-upgrading-to-24.1-step-008`.

docs/generated/settings/settings-for-tenants.txt

@@ -325,4 +325,4 @@ trace.snapshot.rate	duration	0s	if non-zero, interval at which background trace
 trace.span_registry.enabled	boolean	true	if set, ongoing traces can be seen at https://<ui>/#/debug/tracez	application
 trace.zipkin.collector	string		the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.	application
 ui.display_timezone	enumeration	etc/utc	the timezone used to format timestamps in the ui [etc/utc = 0, america/new_york = 1]	application
-version	version	1000023.2-4	set the active cluster version in the format '<major>.<minor>'	application
+version	version	1000023.2-upgrading-to-1000024.1-step-004	set the active cluster version in the format '<major>.<minor>'	application

docs/generated/settings/settings.html

@@ -276,6 +276,6 @@
 <tr><td><div id="setting-trace-span-registry-enabled" class="anchored"><code>trace.span_registry.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if set, ongoing traces can be seen at https://&lt;ui&gt;/#/debug/tracez</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-trace-zipkin-collector" class="anchored"><code>trace.zipkin.collector</code></div></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as &lt;host&gt;:&lt;port&gt;. If no port is specified, 9411 will be used.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-ui-display-timezone" class="anchored"><code>ui.display_timezone</code></div></td><td>enumeration</td><td><code>etc/utc</code></td><td>the timezone used to format timestamps in the ui [etc/utc = 0, america/new_york = 1]</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000023.2-4</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000023.2-upgrading-to-1000024.1-step-004</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 </tbody>
 </table>

pkg/ccl/backupccl/restore_planning_test.go

@@ -132,10 +132,10 @@ func TestBackupManifestVersionCompatibility(t *testing.T) {
 		},
 		{
 			name:                    "alpha-restore",
-			backupVersion:           roachpb.Version{Major: 100022, Minor: 2, Internal: 14},
+			backupVersion:           roachpb.Version{Major: 1000022, Minor: 2, Internal: 14},
 			clusterVersion:          roachpb.Version{Major: 23, Minor: 1},
 			minimumSupportedVersion: roachpb.Version{Major: 22, Minor: 2},
-			expectedError:           "backup from version 100022.2-14 is newer than current version 23.1",
+			expectedError:           "backup from version 1000022.2-upgrading-to-1000023.1-step-014 is newer than current version 23.1",
 		},
 		{
 			name:                    "old-backup",

pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant

@@ -421,18 +421,16 @@ node_id  component  field   value
 0        UI         URI     /
 
 statement error unsupported within a virtual cluster
-SELECT node_id, network, regexp_replace(address, '\d+$', '<port>') as address, attrs, locality, regexp_replace(server_version, '^\d+\.\d+(-\d+)?$', '<server_version>') as server_version FROM crdb_internal.gossip_nodes WHERE node_id = 1
+SELECT node_id FROM crdb_internal.gossip_nodes WHERE node_id = 1
 
 statement error unsupported within a virtual cluster
-SELECT node_id, epoch, regexp_replace(expiration, '^\d+\.\d+,\d+$', '<timestamp>') as expiration, draining, decommissioning, membership FROM crdb_internal.gossip_liveness WHERE node_id = 1
+SELECT node_id FROM crdb_internal.gossip_liveness WHERE node_id = 1
 
 statement error unsupported within a virtual cluster
-SELECT node_id, network, regexp_replace(address, '\d+$', '<port>') as address, attrs, locality, regexp_replace(server_version, '^\d+\.\d+(-\d+)?$', '<server_version>') as server_version, regexp_replace(go_version, '^go.+$', '<go_version>') as go_version
-FROM crdb_internal.kv_node_status WHERE node_id = 1
+SELECT node_id FROM crdb_internal.kv_node_status WHERE node_id = 1
 
 statement error unsupported within a virtual cluster
-SELECT node_id, store_id, attrs, used
-FROM crdb_internal.kv_store_status WHERE node_id = 1
+SELECT node_id FROM crdb_internal.kv_store_status WHERE node_id = 1
 
 query TT
 SELECT * FROM crdb_internal.regions ORDER BY 1

pkg/clusterversion/BUILD.bazel

@@ -16,7 +16,6 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/clusterversion",
     visibility = ["//visibility:public"],
     deps = [
-        "//pkg/build",
         "//pkg/roachpb",
         "//pkg/settings",
         "//pkg/testutils/skip",

pkg/clusterversion/cockroach_versions.go

@@ -430,6 +430,9 @@ func (k Key) IsFinal() bool {
 //     (e.g. 24.1).
 //
 // The key must be in the range [MinSupported, Latest].
+//
+// Note that the release series won't have the DevOffset applied, even if the
+// version has it.
 func (k Key) ReleaseSeries() roachpb.ReleaseSeries {
 	// Note: TestReleaseSeries ensures that this works for all valid Keys.
 	s, _ := removeDevOffset(k.Version()).ReleaseSeries()

pkg/clusterversion/cockroach_versions_test.go

@@ -145,12 +145,11 @@ func TestClusterVersionPrettyPrint(t *testing.T) {
 		cv  ClusterVersion
 		exp string
 	}{
-		{cv(19, 2, 1, 5), "19.2-5"},
-		{cv(20, 1, 0, 4), "20.1-4"},
-		{cv(20, 2, 0, 7), "20.2-7(fence)"},
-		{cv(20, 2, 0, 4), "20.2-4"},
-		{cv(20, 2, 1, 5), "20.2-5(fence)"},
-		{cv(20, 2, 1, 4), "20.2-4"},
+		{cv(20, 1, 0, 4), "20.1-upgrading-to-20.2-step-004"},
+		{cv(20, 2, 0, 7), "20.2-upgrading-to-21.1-step-007(fence)"},
+		{cv(20, 2, 0, 4), "20.2-upgrading-to-21.1-step-004"},
+		{cv(22, 2, 1, 5), "22.2-upgrading-to-23.1-step-005(fence)"},
+		{cv(22, 2, 1, 4), "22.2-upgrading-to-23.1-step-004"},
 	}
 	for _, test := range tests {
 		if actual := test.cv.PrettyPrint(); actual != test.exp {
@@ -169,9 +168,11 @@ func TestReleaseSeries(t *testing.T) {
 		}
 	}
 
+	// Verify the latest version.
+	require.Equal(t, fmt.Sprintf("v%s", Latest.ReleaseSeries()), build.BinaryVersionPrefix())
+
 	// Verify the ReleaseSeries results down to MinSupported.
 	expected := Latest.ReleaseSeries()
-	require.Equal(t, fmt.Sprintf("v%s", expected), build.BinaryVersionPrefix())
 	for k := Latest; k >= MinSupported; k-- {
 		if k.IsFinal() {
 			v := removeDevOffset(k.Version())

pkg/clusterversion/dev_offset.go

@@ -76,7 +76,7 @@ var devOffsetKeyStart = func() Key {
 
 // DevOffset is the offset applied to major versions into the future if this is
 // a dev branch.
-const DevOffset = 1_000_000
+const DevOffset = roachpb.VersionMajorDevOffset
 
 // maybeApplyDevOffset applies DevOffset to the major version, if appropriate.
 func maybeApplyDevOffset(key Key, v roachpb.Version) roachpb.Version {

pkg/cmd/roachtest/roachtestutil/mixedversion/helper_test.go

@@ -33,7 +33,7 @@ func TestClusterVersionAtLeast(t *testing.T) {
 			name:           "invalid minVersion",
 			currentVersion: "23.1",
 			minVersion:     "v23.1",
-			expectedErr:    `invalid version v23.1: strconv.ParseInt: parsing "v23": invalid syntax`,
+			expectedErr:    `invalid version v23.1`,
 		},
 		{
 			name:           "cluster version is behind",

pkg/roachpb/version.go

@@ -12,8 +12,8 @@ package roachpb
 
 import (
 	"fmt"
+	"regexp"
 	"strconv"
-	"strings"
 
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/redact"
@@ -54,16 +54,36 @@ func (v Version) AtLeast(otherV Version) bool {
 	return !v.Less(otherV)
 }
 
-// String implements the fmt.Stringer interface.
+// String implements the fmt.Stringer interface. The result is of the form
+// "23.2" for final versions and "23.2-upgrading-to-24.1-step-004" for
+// transitional internal versions during upgrade.
 func (v Version) String() string { return redact.StringWithoutMarkers(v) }
 
+// VersionMajorDevOffset is an offset we apply to major version numbers during
+// development; see clusterversion.DevOffset for more information.
+const VersionMajorDevOffset = 1_000_000
+
 // SafeFormat implements the redact.SafeFormatter interface.
 func (v Version) SafeFormat(p redact.SafePrinter, _ rune) {
 	if v.IsFinal() {
 		p.Printf("%d.%d", v.Major, v.Minor)
 		return
 	}
-	p.Printf("%d.%d-%d", v.Major, v.Minor, v.Internal)
+	// If the version is offset, remove the offset and add it back to the result. We want
+	// 1000023.1-upgrading-to-1000023.2-step-002, not 1000023.1-upgrading-to-23.2-step-002.
+	noOffsetVersion := v
+	if v.Major > VersionMajorDevOffset {
+		noOffsetVersion.Major -= VersionMajorDevOffset
+	}
+	if s, ok := noOffsetVersion.ReleaseSeries(); ok {
+		if v.Major > VersionMajorDevOffset {
+			s.Major += VersionMajorDevOffset
+		}
+		p.Printf("%d.%d-upgrading-to-%d.%d-step-%03d", v.Major, v.Minor, s.Major, s.Minor, v.Internal)
+	} else {
+		// This shouldn't happen in practice.
+		p.Printf("%d.%d-upgrading-step-%03d", v.Major, v.Minor, v.Internal)
+	}
 }
 
 // IsFinal returns true if this is a final version (as opposed to a transitional
@@ -87,39 +107,45 @@ func (v Version) PrettyPrint() string {
 	return fmt.Sprintf("%v(fence)", v)
 }
 
-// ParseVersion parses a Version from a string of the form
-// "<major>.<minor>-<internal>" where the "-<internal>" is optional. We don't
-// use the Patch component, so it is always zero.
-func ParseVersion(s string) (Version, error) {
-	var c Version
-	dotParts := strings.Split(s, ".")
+var (
+	verPattern = regexp.MustCompile(
+		`^(?P<major>[0-9]+)\.(?P<minor>[0-9]+)(|(-|-upgrading(|-to-[0-9]+.[0-9]+)-step-)(?P<internal>[0-9]+))$`,
+	)
+	verPatternMajorIdx    = verPattern.SubexpIndex("major")
+	verPatternMinorIdx    = verPattern.SubexpIndex("minor")
+	verPatternInternalIdx = verPattern.SubexpIndex("internal")
+)
 
-	if len(dotParts) != 2 {
+// ParseVersion parses a Version from a string of one of the forms:
+//   - "<major>.<minor>"
+//   - "<major>.<minor>-upgrading-to-<nextmajor>.<nextminor>-step-<internal>"
+//   - "<major>.<minor>-<internal>" (older version of the above)
+//
+// We don't use the Patch component, so it is always zero.
+func ParseVersion(s string) (Version, error) {
+	matches := verPattern.FindStringSubmatch(s)
+	if matches == nil {
 		return Version{}, errors.Errorf("invalid version %s", s)
 	}
 
-	parts := append(dotParts[:1], strings.Split(dotParts[1], "-")...)
-	if len(parts) == 2 {
-		parts = append(parts, "0")
+	var err error
+	toInt := func(s string) int32 {
+		if err != nil || s == "" {
+			return 0
+		}
+		var n int64
+		n, err = strconv.ParseInt(s, 10, 32)
+		return int32(n)
 	}
-
-	if len(parts) != 3 {
-		return c, errors.Errorf("invalid version %s", s)
+	v := Version{
+		Major:    toInt(matches[verPatternMajorIdx]),
+		Minor:    toInt(matches[verPatternMinorIdx]),
+		Internal: toInt(matches[verPatternInternalIdx]),
 	}
-
-	ints := make([]int64, len(parts))
-	for i := range parts {
-		var err error
-		if ints[i], err = strconv.ParseInt(parts[i], 10, 32); err != nil {
-			return c, errors.Wrapf(err, "invalid version %s", s)
-		}
+	if err != nil {
+		return Version{}, errors.Wrapf(err, "invalid version %s", s)
 	}
-
-	c.Major = int32(ints[0])
-	c.Minor = int32(ints[1])
-	c.Internal = int32(ints[2])
-
-	return c, nil
+	return v, nil
 }
 
 // MustParseVersion calls ParseVersion and panics on error.
@@ -175,11 +201,14 @@ var successorSeries = map[ReleaseSeries]ReleaseSeries{
 // For non-final versions (which indicate an update to the next series), this
 // requires knowledge of the next series; unknown non-final versions will return
 // ok=false.
-func (v Version) ReleaseSeries() (_ ReleaseSeries, ok bool) {
+//
+// Note that if the version has the clusterversion.DevOffset applied, the
+// resulting release series will have it too.
+func (v Version) ReleaseSeries() (s ReleaseSeries, ok bool) {
 	base := ReleaseSeries{v.Major, v.Minor}
 	if v.IsFinal() {
 		return base, true
 	}
-	res, ok := base.Successor()
-	return res, ok
+	s, ok = base.Successor()
+	return s, ok
 }

pkg/roachpb/version_test.go

@@ -18,6 +18,30 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestParseVersion(t *testing.T) {
+	testData := []struct {
+		s         string
+		v         Version
+		roundtrip bool
+	}{
+		{s: "23.1", v: Version{Major: 23, Minor: 1}, roundtrip: true},
+		{s: "23.1-upgrading-to-23.2-step-004", v: Version{Major: 23, Minor: 1, Internal: 4}, roundtrip: true},
+		{s: "1000023.1-upgrading-to-1000023.2-step-004", v: Version{Major: 1000023, Minor: 1, Internal: 4}, roundtrip: true},
+		{s: "23.1-4", v: Version{Major: 23, Minor: 1, Internal: 4}},
+		{s: "23.1-upgrading-step-004", v: Version{Major: 23, Minor: 1, Internal: 4}},
+	}
+	for _, tc := range testData {
+		t.Run("", func(t *testing.T) {
+			v, err := ParseVersion(tc.s)
+			require.NoError(t, err)
+			require.Equal(t, tc.v, v)
+			if tc.roundtrip {
+				require.Equal(t, tc.s, v.String())
+			}
+		})
+	}
+}
+
 func TestVersionCmp(t *testing.T) {
 	v := func(major, minor, patch, internal int32) Version {
 		return Version{

pkg/sql/logictest/testdata/logic_test/crdb_internal

@@ -667,7 +667,7 @@ node_id  component  field   value
 1        UI         URI     /
 
 query ITTTTT colnames
-SELECT node_id, network, regexp_replace(address, '\d+$', '<port>') as address, attrs, locality, regexp_replace(server_version, '^\d+\.\d+(-\d+)?$', '<server_version>') as server_version FROM crdb_internal.gossip_nodes WHERE node_id = 1
+SELECT node_id, network, regexp_replace(address, '\d+$', '<port>') as address, attrs, locality, regexp_replace(server_version, '^\d+\.\d+(-upgrading-to-\d+\.\d+-step-\d+)?$', '<server_version>') as server_version FROM crdb_internal.gossip_nodes WHERE node_id = 1
 ----
 node_id  network  address           attrs  locality            server_version
 1        tcp      127.0.0.1:<port>  []     region=test,dc=dc1  <server_version>
@@ -679,7 +679,7 @@ node_id  epoch        expiration    draining  decommissioning     membership
 1        <epoch>      <timestamp>   false     false               active
 
 query ITTTTTT colnames
-SELECT node_id, network, regexp_replace(address, '\d+$', '<port>') as address, attrs, locality, regexp_replace(server_version, '^\d+\.\d+(-\d+)?$', '<server_version>') as server_version, regexp_replace(go_version, '^go.+$', '<go_version>') as go_version
+SELECT node_id, network, regexp_replace(address, '\d+$', '<port>') as address, attrs, locality, regexp_replace(server_version, '^\d+\.\d+(-upgrading-to-\d+\.\d+-step-\d+)?$', '<server_version>') as server_version, regexp_replace(go_version, '^go.+$', '<go_version>') as go_version
 FROM crdb_internal.kv_node_status WHERE node_id = 1
 ----
 node_id  network  address           attrs  locality            server_version    go_version

pkg/sql/show_cluster_setting_test.go

@@ -38,26 +38,26 @@ func TestCheckClusterSettingValuesAreEquivalent(t *testing.T) {
 		exp   string
 	}{
 		{ // 0
-			local: encode(t, "22.2-10"),
-			kv:    encode(t, "22.2-10"),
+			local: encode(t, "22.2-upgrading-to-23.1-step-010"),
+			kv:    encode(t, "22.2-upgrading-to-23.1-step-010"),
 		},
 		{ // 1
-			local: encode(t, "22.2-12"),
-			kv:    encode(t, "22.2-11"),
-			exp:   "value differs between local setting (22.2-12) and KV (22.2-11)",
+			local: encode(t, "22.2-upgrading-to-23.1-step-012"),
+			kv:    encode(t, "22.2-upgrading-to-23.1-step-011"),
+			exp:   "value differs between local setting (22.2-upgrading-to-23.1-step-012) and KV (22.2-upgrading-to-23.1-step-011)",
 		},
 		{ // 2
-			local: encode(t, "22.2-11"),
-			kv:    encode(t, "22.2-10"),
+			local: encode(t, "22.2-upgrading-to-23.1-step-011"),
+			kv:    encode(t, "22.2-upgrading-to-23.1-step-010"),
 		},
 		{ // 3
-			local: encode(t, "22.2-11"),
+			local: encode(t, "22.2-upgrading-to-23.1-step-011"),
 			kv:    []byte("abc"),
-			exp:   "value differs between local setting (22.2-11) and KV ([97 98 99])",
+			exp:   "value differs between local setting (22.2-upgrading-to-23.1-step-011) and KV ([97 98 99])",
 		},
 		{ // 4
-			kv:  encode(t, "22.2-11"),
-			exp: "value differs between local setting ([]) and KV (22.2-11)",
+			kv:  encode(t, "22.2-upgrading-to-23.1-step-011"),
+			exp: "value differs between local setting ([]) and KV (22.2-upgrading-to-23.1-step-011)",
 		},
 	} {
 		t.Run("", func(t *testing.T) {