roachtest: normalize versions in multitenant-upgrade roachtest

[healthy-pod]

If the multitenant-upgrade roachtest uses a mix
of release/non-release binaries, it may be using versions
that are technically the same but fail to confirm that because
version in test binaries are incremented by 1M.

This code change fixes the issue by normalizing versions before
comparing them.

Closes #95648

Epic: none
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 

[ajstorm]

I left some comments, but I think we're going to need @postamar's eyes on this too. I know the spirit of what his PR meant to accomplish, but the details are important and subtle.

@postamar could you please take a look?

[ajstorm]

IIUC, this change does subtly alter what we're doing here. Before your change, we'd always initialize the tenant to a fully valid cluster version (as opposed to a cluster version which may be mid-way between two version, in the case of an in-progres upgrade). I'm not certain what's the right thing to do here, but I'd think that we wouldn't want to initialize to intermediate versions as we'd need to initialize the catalogs to that intermediate state as well, which seems difficult.

[ajstorm]

Is this change future-proof? If, for example, we let MinSupportedVerion trail a few releases, would this comment still be correct? I'd imagine that the test would still do what we'd want it to do, though maybe not if we wanted to test all of the upgrade stages in between v0 and v2.

[healthy-pod]

I agree maybe this is not the right change, maybe we should make the changes to how we test instead? I am looking for suggestions but I will leave 2 points on why I made this change to further explain how this is just a testing issue and not an actual correctness issue:

1. passing a version override using testing knobs or by setting version when creating testing cluster settings is no longer effective because the changes in createTenantInternal run after that (ofcourse) and do not check for any overrides. Ideally, minVersion and curVersion in createTenantInternal should have the testing overrides values if an override was passed.
2. checking for version equality between two servers where a server is started with a crdb_test-built binary and the other isn't will return different versions even if they are technically the same. I am thinking of adding another version equality function that works around this by incrementing/decrementing one of the versions by 1e6 and returning true if they become equal.

[healthy-pod]

When multiple previous releases are supported, my previous code change becomes incorrect. I removed my changes to tenant_creation and just made the test ignore the difference between normal versions and versions from test binaries by normalizing them.

Ready for another pass.

[renatolabs]

Drive-by comment about upgrade testing here: If we decide to merge this PR as it currently is, I'd suggest adding a note/TODO/issue to revert it at some point. I think we can agree that asserting on specific cluster versions makes for a stronger test (and having or not having the version offset is part of that); more importantly, we want to eventually live in a world where most upgrade roachtests don't have to deal with version offsetting (#92608).

I'd also make a small correction here that version offsetting does not depend on the crdb_test build flag; offsetting happens if the binary is not considered a release binary:

cockroach/pkg/clusterversion/cockroach_versions.go

Lines 712 to 717 in 3e65660

	// developmentBranch must true on the main development branch but should be set
	// to false on a release branch once the set of versions becomes append-only and
	// associated upgrade implementations are frozen. It can be forced to true via
	// an env var even on a release branch, to allow running a release binary in a
	// dev cluster.
	var developmentBranch = true || envutil.EnvOrDefaultBool("COCKROACH_FORCE_DEV_VERSION", false)

[healthy-pod]

@renatolabs

I'd suggest adding a note/TODO/issue to revert it at some point

What should the trigger of this revert be? Is there something expected after which we won't need to normalize versions?

I'd also make a small correction here that version offsetting does not depend on the crdb_test build flag; offsetting happens if the binary is not considered a release binary

👍

[renatolabs]

What should the trigger of this revert be?

Once #92608 is closed.

[ajstorm]

I like this change much better than the initial one, but it's not clear to me that it's what we want long-term. I'd think that the long-term fix here is to ensure that the create tenant code respects the version overrides contained in the testing knobs. That way we won't need the offset ignoring code. I'm OK to merge this now if we plan to add the testing knobs fix in a separate PR.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @healthy-pod, @knz, @postamar, @smg260, and @srosenberg)

---

pkg/cmd/roachtest/tests/multitenant_upgrade.go line 315 at r2 (raw file):

) {
	normalizeVersion := func(v roachpb.Version) roachpb.Version {
		if (v.Major - clusterversion.DevOffset) >= 0 {

Nit: it might beeasier to understand this code if it was written as:

if v.Major > clusterversion.DevOffset {

[healthy-pod]

We added the tenant logical version overrides in #94973, this is not related to that. Previously (ie before #94774) we used to set the tenant logical version to be exactly the storage logical version. As in, we read the SLV and set the TLV to that. Now this is not true, it's either the SBV if it's equal to the SLV or otherwise the min version the storage binary can use to create a tenant. What happens here is:

current binary = non-release binary with version offset
predecessor binary = release binary without version offset

• storage starts with predecessor binary and its logical version is predecessor binary so SLV doesn't have offset
• storage upgrades to current binary but doesn't finalize so SLV is still without offset
• storage binary now though is current binary which is non-release and when it "calculates" the tenant creation version (instead of what used to happen previously which was just copying SLV) it will be the min supported version of current binary which happens to be predecessor binary but with offset because current binary is not a release binary even if storage didn't finalize yet.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @postamar, @smg260, and @srosenberg)

[healthy-pod]

bors r=ajstorm

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)