clusterversion: change string for upgrade versions

[RaduBerinde]

clusterversion: move ReleaseSeries functionality to roacphb.Version

This change moves the implementation of
clusterversion.Key.ReleaseSeries() to roachpb. We now use a
hardcoded map of sucessor versions.

Epic: none
Release note: None

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.

[cockroach-teamcity]

This change is 

[RaduBerinde]

Open to suggestions if there's something better than upgrade-step. Ideally something that would give more of a hint that we're upgrading from the xx.y prefix, and not to it.

[dt]

I'm not sure but upgrading might have a slightly more accurate implication than upgrade?

if I say "22.2 upgraded step 8" in my head that means the 8th step of the "22.2 upgrade". Of course we both know it is the 22.2 to 23.1 upgrade, but that is lost there. If someone said "22.2 upgrading step 8" I think it could sound like it is either to OR from -- it isn't obviously from in my mind, but seems less obviously (and incorrectly) to? Maybe that's just me though.

But I think we could do better if we wanted to: we can put whatever string we want between these parts - it isn't meaningful to the computer. That means we could put the version we're going to in there too, e.g. 22.2-upgrading-to-23.1-step-08.

You might say "but we don't know we're upgrading to 23.1" but we do actually: we only define step 08 in a 23.1 binary, so we know by that point the name of the version we're headed to. We could make a little map in this package called successors = map[Version]ClusterVersion{ {Major:22, Minor: 2} : V23_1, ...} and then this String() method could do something like

if succ, ok := successors[Verison{Major: v.Major, Minor: v.Minor}]; ok { 
  return fmt.Sprintf("%d.%d-upgrading-to-%s-step-%03d", v.Major, v.Minor, succ, v.Internal)
} 
return fmt.Sprintf("%d.%d-upgrading-step-%03d", v.Major, v.Minor, succ, v.Internal)

That said, this is more complicated, and putting literally anything other than just - here would make the situation better than it is now, so a static string upgrade-step or upgrading-step seems like a strict improvement too.

[RaduBerinde]

Very cool idea. I already added something along those lines that in clusterversion: https://github.com/cockroachdb/cockroach/blob/master/pkg/clusterversion/cockroach_versions.go#L430

We can't depend on clusterversion from roachpb. I think the best way is to generate the code for the map as part of dev gen go

[dt]

generate the code for the map as part of dev gen go

Ehhhh; it's a single-digit entry count map. I'd just hand-write it in roachpb/version.go and then change that code in clusterversion to call in to the map instead of deriving it on the fly. If we're worried about rot we can add a tiny unit test in clusterversion that just compares IsFinal to the map entries. Adding a generation step for this feels like overkill / slow build times /2¢.

[RaduBerinde]

Oh, good point. Yes I agree, we can move the current logic to a test as a cross-check. Nice!

[RaduBerinde]

Done!

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @dt, and @herkolategan)

---

docs/generated/settings/settings.html line 279 at r2 (raw file):

<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-upgrading-step-002</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>

This is because of the dev offset.. I could move the DevOffset constant there and remove it if necessary.. but it will still look weird: 1000023.2-upgrading-to-24.1-step-002.

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @dt, and @herkolategan)

---

docs/generated/settings/settings.html line 279 at r2 (raw file):

Previously, RaduBerinde wrote…

This is because of the dev offset.. I could move the DevOffset constant there and remove it if necessary.. but it will still look weird: 1000023.2-upgrading-to-24.1-step-002.

Never mind, I made it work.

[dt]

nice!

[RaduBerinde]

TFTR!

bors r+

[RaduBerinde]

bors r-

[craig]

Canceled.

[RaduBerinde]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)