clusterversion: bump binaryMinSupportedVersion to 23.1, allowing 22.2 for testing

[rail]

Previously, we used the same minimum version for production upgrades and testing. In 23.2 we will be able to upgrade from 22.2, but this is not yet a stable feature.

This PR bumps the default version used for tests to 23.1, allowing using 22.2 if the COCKROACH_ALLOW_VERSION_SKIPPING environment variable is set to true.

Fixes: RE-477
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[rail]

Looks like this affects test logic in some places. 🤔

[RaduBerinde]

In my opinion, most of the code should look exactly like we allow using 22.2 non-experimentally. There should only be one override in the cockroach binary initialization that bumps it to 23.1 for production.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @celiala, @knz, @rail, and @sumeerbhola)

---

pkg/clusterversion/cockroach_versions.go line 911 at r1 (raw file):

	// an error. This typically trails the current release by one (see top-level
	// comment).
	// Note: this variable is used for tests, and it is not the source of truth.

Is this really only used for tests? It's used in MakeVersionHandle which is production, no?

In any case, if we bump the tests to use 23.1, any 22.2 -> 23.2 paths will not be tested, why would we want to do that?

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @celiala, @knz, @rail, and @sumeerbhola)

---

pkg/clusterversion/cockroach_versions.go line 905 at r1 (raw file):

// separate concepts.
var (
	TestingBinaryMinSupportedVersionKey = V23_1

All the logic and concepts explained here are already pretty hard to understand. Now we're adding a key that gets rewritten based on an env var in init(). This kind of stuff should be done during initialization, not in init().

I think it would help to have two separate concepts:

• the min supported version - the one that the code is supposed to support. This stays 22.2
• the min allowed version - the one that we are confident it is stable. This is 22.2 or 23.1 depending on flags. This concept can be restricted to the cockroach binary initialization and doesn't need to be here.

[rail]

FTR, I'm still experimenting (i.e. learning this part of the code ;) ) and open for all kind suggestions.

[RaduBerinde]

I added @knz and @dt too as they may have some good ideas about how to do this in the cleanest way possible.

[dt]

I know I suggested this change originally -- keeping BinaryMinSupportedVersionKey set to 22.2 until 23.2 was out to go though the motions of having a year old BinaryMinSupportedVersionKey, since that's what we'll be doing in 24.1 when it is 23.1.

That said, I'm sympathetic to Radu's argument that this is making a complicated and subtle part of the system more complicated (albeit just for one release), so I could also see walking away from this idea and just bumping it to the constant v23_1: we already went through majority of the development cycle with it set to 22_2, so we've already go through most of the motions of keeping year-old gates and compatibility around for a year, so I won't be heartbroken if we bumped it now with a simple constant.

[dt]

I'm not sure we want to change this: I think this config is explicitly labeled as 22.2-23.1 so it should be skipped / deleted when we drop 22.2 interop, rather than run against 23.1 instead, right?

[dt]

nit: I personally might find this easier to read as if clusterversion.V22_2 < clusterversion.BinaryMinSupportedVersionKey { t.Skip("test of 22.2 interop not applicable")}

[dt]

if we do want to go with the env-var approach, I might suggest rather than introducing a new "AllowVersionSkipping" var and more variables and conditionals, we just use an anon-function to set BinaryMinSupportedVersionKey, e.g.

var BinaryMinSupportedVersionKey = func() Key { 
  if envutil("COCKROACH_UNSAFE_22_2_COMPAT") { return V22_2} 
  return V23_1
}()

Since this is a one-time thing for this release, we'd delete it as soon as the branch is cut (and just go back to = V23_1, so I'd just minimize the blast radius to how we set that one var.

[rail]

I filed #111760 as an alternative to this solution. Draft PRs will be available soon.

[celiala]

[For visibility] the REA team discussed everything raise here so far last week, and decided to remove version skipping support for 22.2 → 23.2:

i.e. we will set minBinary=V23_1 for release-23.2 (backporting as needed before the beta.1).

Reference:

• https://cockroachlabs.atlassian.net/browse/RE-477?focusedCommentId=192128
• minBinary schedule has been updated: https://cockroachlabs.atlassian.net/wiki/spaces/RE/pages/3197665321/Schedule+for+advancing+BinaryMinSupportedVersionKey+a.k.a.+minBinary