Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clusterversion: bump MinSupported to 24.2 #134340

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Nov 5, 2024

Once we advance the version to to 25.1, we will bump again to 24.3

Epic: REL-1292
Release note: None

Copy link

blathers-crl bot commented Nov 5, 2024

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
Copy link
Member

This change is Reviewable

Copy link

blathers-crl bot commented Nov 5, 2024

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

@RaduBerinde RaduBerinde force-pushed the bump-min-version-to-24-3 branch 9 times, most recently from ca8d442 to 1067353 Compare November 6, 2024 19:15
@RaduBerinde RaduBerinde marked this pull request as ready for review November 6, 2024 19:28
@RaduBerinde RaduBerinde requested review from a team as code owners November 6, 2024 19:28
@RaduBerinde RaduBerinde requested review from nameisbhaskar, DarrylWong, jbowens and mgartner and removed request for a team November 6, 2024 19:28
celiala
celiala previously requested changes Nov 6, 2024
Copy link
Collaborator

@celiala celiala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 62 of 62 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @jbowens, @mgartner, @nameisbhaskar, @RaduBerinde, and @rail)


pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/skip_version_upgrades line 31 at r1 (raw file):

----
Seed:               12345
Upgrades:           v23.1.4 → v23.2.0 → v24.1.1 → <current>

it seems like we should keep v24.1, since v24.1 is the regular release?

so perhaps instead [once .0 comes out]:

v23.1.4 → v23.2.0 → v24.1.1 → v24.3.0 →


pkg/sql/logictest/testdata/logic_test/upgrade_skip_version line 2 at r1 (raw file):

# TODO(radu): reenable this test in 25.2 when we support upgrade from two
# previous releases.

let's x-reference this tracking issue, so we don't forget:

#134455


pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion_test.go line 247 at r1 (raw file):

			numUpgrades:       3,
			enableSkipVersion: true,
			expectedReleases:  []string{"23.1.17", "23.2.4", "24.1.1"},

should we leave a 24.1 release here?

@celiala celiala dismissed their stale review November 6, 2024 20:29

oops. didn't mean to block

Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @celiala, @DarrylWong, @jbowens, @mgartner, @nameisbhaskar, and @rail)


pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/skip_version_upgrades line 31 at r1 (raw file):

Previously, celiala wrote…

it seems like we should keep v24.1, since v24.1 is the regular release?

so perhaps instead [once .0 comes out]:

v23.1.4 → v23.2.0 → v24.1.1 → v24.3.0 →

As far as I understand, this is the randomized sequence of versions that the test decides on with that seed. The old version was deciding to do a 24.2->current upgrade which is no longer allowed. It now chooses to skip 24.1 because 24.2 supported skipping experimentally.

Note that the test only sometimes allows skipping versions (with a random probability) so both ways get tested.


pkg/sql/logictest/testdata/logic_test/upgrade_skip_version line 2 at r1 (raw file):

Previously, celiala wrote…

let's x-reference this tracking issue, so we don't forget:

#134455

Good idea, done!


pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion_test.go line 247 at r1 (raw file):

Previously, celiala wrote…

should we leave a 24.1 release here?

Like before, this is testing the logic of the test itself which always skips the latest possible skippable release in the chain (when version skipping is enabled).

Once we advance the version to to 25.1, we will bump again to 24.3

Epic: REL-1292
Release note: None
Copy link
Collaborator

@celiala celiala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! :lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @jbowens, @mgartner, @nameisbhaskar, @RaduBerinde, and @rail)


pkg/cmd/roachtest/roachtestutil/mixedversion/testdata/planner/skip_version_upgrades line 31 at r1 (raw file):

Previously, RaduBerinde wrote…

As far as I understand, this is the randomized sequence of versions that the test decides on with that seed. The old version was deciding to do a 24.2->current upgrade which is no longer allowed. It now chooses to skip 24.1 because 24.2 supported skipping experimentally.

Note that the test only sometimes allows skipping versions (with a random probability) so both ways get tested.

ah, thanks for the context. makes sense 👍


pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion_test.go line 247 at r1 (raw file):

Previously, RaduBerinde wrote…

Like before, this is testing the logic of the test itself which always skips the latest possible skippable release in the chain (when version skipping is enabled).

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants