Merge #158016 #158343 #158357

158016: mixedversion: add regression test for #157852 r=herkolategan a=DarrylWong

Release note: none
Epic: none

158343: mma: remove accidental duplicate comment r=wenyihu6 a=sumeerbhola

Epic: CRDB-55052

Release note: None

158357: sql/ttl/ttljob: skip flaky TestRowLevelTTLJobMultipleNodes test r=yuzefovich a=dt

Updates #158317.

Release note: none.
Epic: none.

Co-authored-by: DarrylWong <darryl@cockroachlabs.com>
Co-authored-by: sumeerbhola <sumeer@cockroachlabs.com>
Co-authored-by: David Taylor <davidt@davidt.io>

Author: 4 people

pkg/cmd/roachtest/roachtestutil/mixedversion/BUILD.bazel

@@ -57,6 +57,8 @@ go_test(
     embed = [":mixedversion"],
     embedsrcs = ["testdata/test_releases.yaml"],
     deps = [
+        "//pkg/build",
+        "//pkg/clusterversion",
         "//pkg/cmd/roachtest/option",
         "//pkg/cmd/roachtest/registry",
         "//pkg/cmd/roachtest/roachtestutil",

pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion_test.go

@@ -11,6 +11,8 @@ import (
 	"math/rand"
 	"testing"
 
+	"github.com/cockroachdb/cockroach/pkg/build"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/clusterupgrade"
@@ -477,3 +479,37 @@ func TestSupportsSkipUpgradeTo(t *testing.T) {
 		expect(v, false)
 	}
 }
+
+// Regression test for #157852.
+// This tests that our skip upgrade logic does not break when we are in the middle
+// of the version bump process. We bump the current version and the minSupportedVersion
+// separately, so we want to make sure that:
+//  1. supportsSkipUpgradeTo does not let us perform a skip upgrade from a non supportedVersion.
+//     This edge case happens if we bump the minSupportedVersion first and there is only 1 supported
+//     previous release in the current series despite being a .2 or .4 release.
+//  2. supportsSkipUpgradeTo does not let us perform a skip upgrade over a .2 or .4 release.
+//     This edge case happens if we bump the current version first and there could be 3 supported
+//     previous releases in the current series.
+func TestSupportsSkipCurrentVersion(t *testing.T) {
+	mvt := newTest()
+
+	// Case 1: If the minimumSupportedVersion on the current release says there is only 1 supported
+	// previous release, then it doesn't matter if the previous release is an innovation or not,
+	// we can't skip over it.
+	numSupportedVersions := len(clusterversion.SupportedPreviousReleases())
+
+	// Case 2: If the current release is an innovation release, it means the previous release
+	// is _not_ an innovation, i.e. we can't skip over it.
+	currentRelease := clusterversion.Latest.ReleaseSeries()
+	isInnovationRelease := currentRelease.Minor == 1 || currentRelease.Minor == 3
+
+	// We can only perform a skip upgrade to the current version if it's not an innovation and
+	// there are multiple supported previous versions.
+	expected := !isInnovationRelease && numSupportedVersions > 1
+
+	// N.B. The predecessor is only used to determine if we should skip over the
+	// mixedversion test's minimum supported version, and not relevant for this test.
+	pred := clusterupgrade.MustParseVersion("24.2.1")
+	actual := mvt.supportsSkipUpgradeTo(pred, &clusterupgrade.Version{Version: version.MustParse(build.BinaryVersion())})
+	require.Equal(t, expected, actual)
+}

pkg/kv/kvserver/allocator/mmaprototype/cluster_state_rebalance_stores.go

@@ -523,13 +523,6 @@ func (re *rebalanceEnv) rebalanceLeasesFromLocalStoreID(
 	log.KvDistribution.VEventf(ctx, 2, "local store s%d is CPU overloaded (%v >= %v), attempting lease transfers first",
 		store.StoreID, store.dimSummary[CPURate], overloadSlow)
 	// This store is local, and cpu overloaded. Shed leases first.
-	//
-	// NB: due to REQUIREMENT(change-computation), the top-k ranges for ss
-	// reflect the latest adjusted state, including pending changes. Thus,
-	// this store must be a replica and the leaseholder, which is asserted
-	// below. The code below additionally ignores the range if it has pending
-	// changes, which while not necessary for the is-leaseholder assertion,
-	// makes the case where we assert even narrower.
 	topKRanges := ss.adjusted.topKRanges[localStoreID]
 	var leaseTransferCount int
 	n := topKRanges.len()

pkg/sql/ttl/ttljob/ttljob_test.go

@@ -593,6 +593,7 @@ INSERT INTO t (id, crdb_internal_expiration) VALUES (1, now() - '1 month'), (2,
 func TestRowLevelTTLJobMultipleNodes(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
+	skip.WithIssue(t, 158317)
 
 	skip.UnderRace(t, "the test times out under race")