cluster: remove and test against obsolete versions

[tbg]

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Very nice! mod a few comments.

We can even remove up to Version19_1 now, right? Did you avoid that thinking that it might interfere with backports?

Reviewed 36 of 36 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

---

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

}

func TestClusterVersionMixedVersionTooOld(t *testing.T) {

Were these tests only relevant to version 1.0, before we had upgrade logic? I'm surprised to see us removing them.

---

pkg/settings/cluster/keyed_versions.go, line 76 at r1 (raw file):

	}

	if n := len(byRelease) - 3; n >= 0 {

Nice! Add a comment about what this is trying to test though.

---

pkg/storage/replica_proposal.go, line 976 at r1 (raw file):

		r.mu.RUnlock()
		if !usingAppliedStateKey {
			// The range applied state was introduced in v2.1. It's possible to

I guess we can close #33653 now. cc. @andreimatei

There is an updated comment and test in that PR that you might want to port over to here.

---

pkg/storage/stores.go, line 294 at r1 (raw file):

	}

	// These values should always exist in 1.1-initialized clusters, but may

Is it worth returning an error if this is true then?

[tbg]

Yes, I wanted to leave some slack to avoid unnecessarily interfering. I should have added a comment. (Done).

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Were these tests only relevant to version 1.0, before we had upgrade logic? I'm surprised to see us removing them.

You're right, one of them (joining an old node into a cluster) was removed erroneously (it was failing because of the automatic cluster upgrade that basically led to ignoring the min supported version). I fixed it up and reintroduced it. The other case (joining node is too new) can't be tested like this any more because the RPC handshake prevents the new node from accepting connections from old nodes (or establishing to them).

---

pkg/storage/replica_proposal.go, line 976 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I guess we can close #33653 now. cc. @andreimatei

There is an updated comment and test in that PR that you might want to port over to here.

Done.

---

pkg/storage/stores.go, line 294 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is it worth returning an error if this is true then?

I meant to rename to ReadVersionFromEngineOrZero, but forgot. Note that the caller handles this gracefully. We could also signal that through an error, though it's a wash which one is better, so I kept the status quo.

[tbg]

TFTR!
bors r=nvanbenschoten

[craig]

Canceled

[tbg]

bors r=nvanbenschoten

[tbg]

bors r=nvanbenschoten

[craig]

Build succeeded

• GitHub CI (Cockroach)