cluster: remove legacy version upgrade tests for auto upgrade merge

[windchan7]

Legacy tests such as TestClusterVersionUpgrade1_0To2_0,
TestDockerReadWriteBidirectionalReferenceVersion, and
TestDockerReadWriteForwardReferenceVersion no longer applies.

Therefore, we remove and they will be replaced with new roachtests
when the auto upgrade PR #24987 merges.

Release note: None

[cockroach-teamcity]

This change is 

[benesch]

but hold off on merging until we get someone else to buy in that these are no longer useful.

I think the TestDockerReadWrite...ReferenceVersion tests are already obsoleted by the already-existing version roachtest. (Your PR, #24987, enhances the roachtest version coverage but is not strictly necessary to justify removing these tests.) The one argument for keeping them is that they run on every PR while roachtests only run nightly, but they sure require a lot of duplicated effort and haven't caught a bug in quite a long time.

The obvious choices are @bdarnell and @tschottdorf, but they're both OOO. Perhaps @nvanbenschoten or @petermattis feels comfortable LGTM'ing in their absence?

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[tbg]

I'm fine (happy, actually) removing the docker reference tests, these haven't caught anything in a long time and I've wanted to kill them for a while (#18960).

What is the rationale for removing the unit tests? Yes, the same functionality is exercised by roachtest, but there's value in having a basic sanity check that the upgrade mechanism works (and one that can be stress tested). My suggestion here is to remove all the stuff about Raft tombstones and to populate the versions slice automatically (assuming that the binary version is 3.3-6, it would upgrade from 3.3 to 4.0). That way, the test won't rot in the future and can be used to test specific migrations that are currently being introduced.

If the auto-upgrade mechanism somehow makes this test unfeasible, we should massage it slightly so that it becomes feasible. I'd be curious to see if there are any problems. (The test should definitely still run quickly, we don't want longer waiting periods here).

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

pkg/server/version_cluster_test.go, line 146 at r1 (raw file):

	}

	// We will put down fake legacy tombstones (#12154) to jog

I love how you can just delete migration-related tests when the migration isn't "current" any more.

---

Comments from Reviewable

[bdarnell]

I agree with @tschottdorf that removing the acceptance test is fine but it would be nice to keep the unit test.

When this is gone, we can also remove the old binaries from the docker image:

https://github.com/cockroachdb/postgres-test/blob/b1347c055696856624c1a62aeffd0fd3b066695a/Dockerfile#L99-L107

[windchan7]

@tschottdorf Hey Tobi, I understand your idea to make the test not rot in the future. But do you mind clarifying what you mean by populating the versions slice automatically?

[windchan7]

@tschottdorf , want to LGTM this test removal?

[tbg]

LGTM 🔥

[windchan7]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)