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 binaryMinSupportedVersion to 23.1 #111760

Closed
rail opened this issue Oct 4, 2023 · 8 comments
Closed

clusterversion: bump binaryMinSupportedVersion to 23.1 #111760

rail opened this issue Oct 4, 2023 · 8 comments
Assignees
Labels
A-release branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-release Release Engineering & Automation Team

Comments

@rail
Copy link
Member

rail commented Oct 4, 2023

We need to bump binaryMinSupportedVersion to 23.1 before we release the first 23.2 beta (branch cut is currently scheduled for Oct 10).

TODO:

  1. [release-blocker] PR 1 - bump minBinary, adding skips to all the tests that fail.
  2. Create tracking issue(s) for tests skipped in (1). Then assign to teams so they delete whenever.
  3. Rename / prefix 23.1 gates with a TODODelete_ prefix or something, so when teams see if in the code they know they can clean up related code.
  4. Create tracking issue(s) to delete 23.1 gates. Then assign to teams so they delete whenever.

Also tracked in https://cockroachlabs.atlassian.net/browse/REL-506

Jira issue: CRDB-32062

@rail rail added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-release T-release Release Engineering & Automation Team labels Oct 4, 2023
@rail rail self-assigned this Oct 4, 2023
rail added a commit to rail/cockroach that referenced this issue Oct 6, 2023
This PR bumps `BinaryMinSupportedVersion` to 23.1 before cutting the 23.2
release branch, as part of the stability period process.

Issue: cockroachdb#111760
Epic: REL-433
Release note: None
@RaduBerinde
Copy link
Member

I think we should rename things like local-mixed-22_2-23_1_test to `local-mixed-with-previous-version' so we don't have to rename it every time. Can be done separately so it's just a mechanical change.

@dt
Copy link
Member

dt commented Oct 10, 2023

rename things like local-mixed-22_2-23_1_test to `local-mixed-with-previous-version'

This is actually the opposite of a change we elected to make a couple versions ago. Having the old version name explicitly in the test makes it easier to know that you can delete that config when we are no longer testing behaviors specific to that version, without needing to know what exactly those behaviors are.

Having it implicitly move up to test a new different version is much harder, in practice, since the person making that change is typically a release engineer, who is then facing a test that starts failing since the behaviors it thought it was testing - 22.2 behaviors -- changed since it is now testing against a different version. The feature-area engineers should be the ones writing tests that encode the expected behaviors of their feature area in a specific -- and explicitly named! -- version and then the release engineering folks just need to know that if they're changing what versions we want to test, certain we'll-labeled feature tests are or are not applicable.

@RaduBerinde
Copy link
Member

Oh, that makes sense - I was reacting to the (draft?) changes in #111768. Looked like that PR renamed local-mixed-22.2-23.1 and all uses of it to local-mixed-23.1-23.2..

I think we're missing a comment with some guidance above where "local-mixed-22.2-23.1" is defined about how to move forward when 22.2 is no longer supported.

Also, it feels like the name is not exactly correct, it should be local-mixed-22.2-current or local-mixed-with-22.2

@RaduBerinde
Copy link
Member

One question though.. why is there no local-mixed-23.1-current config? Do we really have no post-23.1 migrations to test?

My hunch is that we're using local-mixed-22.2-23.1 config to test both migrations in 23.1 and migrations in master (i.e. 23.2) and disambiguating this will be a pain.

@RaduBerinde RaduBerinde self-assigned this Oct 10, 2023
@RaduBerinde
Copy link
Member

RaduBerinde commented Oct 10, 2023

Here is my (updated) plan of action:

  • bump min supported to 23.1
  • remove logictest config local-mixed-22.2-23.1 and add local-mixed-23.1; skip the latter config on all failures caused by features added in 23.2
  • remove pre 23.1 version gates
  • fix up/remove any tests that involve 22.2

The changes above are backported to release-23.2.

Then, only on release-23.2:

  • mint the 23.2 version

Then, only on master:

  • add 23.2 version, switch to 24.1 version
  • add logictest config local-mixed-23.2
  • add a way for the binary to choose between setting 23.1 or 23.2 min supported version

RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 12, 2023
This commit changes the minimum supported version to 23.1. It is
intended to be backported to release-23.2.

Tests that had to be modified mostly fall in one of two categories:
 - tests that verify a release-specific behavior involving 22.2 or
   in-development versions of 23.1; these tests were deleted.
 - tests that verify general version handling logic but used specific
   versions; I tried to make these use the min supported version or a
   new `VCurrent_Start` constant.

The logic tests required a bunch of work because we were missing a
`local-mixed-23.1` config, which should have been added early in the
23.2 cycle. Without this config, all test directives related to 23.2
features were conditioned on `local-mixed-22-23.1` (i.e. these
features were lumped in with 23.1 features). I added the new config
and let the test failures guide me; most of the cases that needed to
be conditioned on `local-mixed-23.1` where around the bigger 23.2
features (isolation levels, procedures). In subsequent release cycles,
we will create the new config as soon as possible.

This commit does not remove the obsolete in-development 23.1 version
keys and related code; that will be done separately.

Informs: cockroachdb#111760
Epic: REL-506
Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 12, 2023
This commit changes the minimum supported version to 23.1. It is
intended to be backported to release-23.2.

Tests that had to be modified mostly fall in one of two categories:
 - tests that verify a release-specific behavior involving 22.2 or
   in-development versions of 23.1; these tests were deleted.
 - tests that verify general version handling logic but used specific
   versions; I tried to make these use the min supported version or a
   new `VCurrent_Start` constant.

The logic tests required a bunch of work because we were missing a
`local-mixed-23.1` config, which should have been added early in the
23.2 cycle. Without this config, all test directives related to 23.2
features were conditioned on `local-mixed-22-23.1` (i.e. these
features were lumped in with 23.1 features). I added the new config
and let the test failures guide me; most of the cases that needed to
be conditioned on `local-mixed-23.1` where around the bigger 23.2
features (isolation levels, procedures). In subsequent release cycles,
we will create the new config as soon as possible.

This commit does not remove the obsolete in-development 23.1 version
keys and related code; that will be done separately.

Informs: cockroachdb#111760
Epic: REL-506
Release note: None
@rafiss rafiss added release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 labels Oct 12, 2023
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 12, 2023
This commit changes the minimum supported version to 23.1. It is
intended to be backported to release-23.2.

Tests that had to be modified mostly fall in one of two categories:
 - tests that verify a release-specific behavior involving 22.2 or
   in-development versions of 23.1; these tests were deleted.
 - tests that verify general version handling logic but used specific
   versions; I tried to make these use the min supported version or a
   new `VCurrent_Start` constant.

The logic tests required a bunch of work because we were missing a
`local-mixed-23.1` config, which should have been added early in the
23.2 cycle. Without this config, all test directives related to 23.2
features were conditioned on `local-mixed-22-23.1` (i.e. these
features were lumped in with 23.1 features). I added the new config
and let the test failures guide me; most of the cases that needed to
be conditioned on `local-mixed-23.1` where around the bigger 23.2
features (isolation levels, procedures). In subsequent release cycles,
we will create the new config as soon as possible.

This commit does not remove the obsolete in-development 23.1 version
keys and related code; that will be done separately.

Informs: cockroachdb#111760
Epic: REL-506
Release note: None
craig bot pushed a commit that referenced this issue Oct 12, 2023
112122: clusterversion: bump min supported version to 23.1 r=RaduBerinde a=RaduBerinde

This commit changes the minimum supported version to 23.1. It is
intended to be backported to release-23.2.

Tests that had to be modified mostly fall in one of two categories:
 - tests that verify a release-specific behavior involving 22.2 or
   in-development versions of 23.1; these tests were deleted.
 - tests that verify general version handling logic but used specific
   versions; I tried to make these use the min supported version or a
   new `VCurrent_Start` constant.

The logic tests required a bunch of work because we were missing a
`local-mixed-23.1` config, which should have been added early in the
23.2 cycle. Without this config, all test directives related to 23.2
features were conditioned on `local-mixed-22.2-23.1` (i.e. lumped in
with 23.1 features). I added the new config and let the test failures
guide me; most of the cases that needed to be conditioned on
`local-mixed-23.1` where around the bigger 23.2 features (isolation
levels, procedures). In subsequent release cycles, we will create the
new config as soon as possible.

This commit does not remove the obsolete in-development 23.1 version
keys and related code; that will be done separately.

Informs: #111760
Epic: REL-506
Release note: None

Co-authored-by: Radu Berinde <[email protected]>
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 13, 2023
This commit changes the minimum supported version to 23.1. It is
intended to be backported to release-23.2.

Tests that had to be modified mostly fall in one of two categories:
 - tests that verify a release-specific behavior involving 22.2 or
   in-development versions of 23.1; these tests were deleted.
 - tests that verify general version handling logic but used specific
   versions; I tried to make these use the min supported version or a
   new `VCurrent_Start` constant.

The logic tests required a bunch of work because we were missing a
`local-mixed-23.1` config, which should have been added early in the
23.2 cycle. Without this config, all test directives related to 23.2
features were conditioned on `local-mixed-22-23.1` (i.e. these
features were lumped in with 23.1 features). I added the new config
and let the test failures guide me; most of the cases that needed to
be conditioned on `local-mixed-23.1` where around the bigger 23.2
features (isolation levels, procedures). In subsequent release cycles,
we will create the new config as soon as possible.

This commit does not remove the obsolete in-development 23.1 version
keys and related code; that will be done separately.

Informs: cockroachdb#111760
Epic: REL-506
Release note: None
@celiala
Copy link
Collaborator

celiala commented Oct 16, 2023

Of the 4 TODOs listed, only item 1 is the release-blocker (items 2-4 don't block the release and can be done anytime).

Removing the release-blocker label, since item 1 is now complete and backported.

@celiala celiala removed the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Oct 16, 2023
@RaduBerinde
Copy link
Member

2 is done too, I had to skip a couple and I filed issues. Working on 3 and 4 right now.

RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 16, 2023
This commit renames all pre-23.1 version keys (except those associated permanent
upgrades) to start with `TODO_Delete_`. These keys will be removed by
relevant teams.

Informs: cockroachdb#111760
Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Oct 16, 2023
This commit renames all pre-23.1 version keys (except those associated permanent
upgrades) to start with `TODO_Delete_`. These keys will be removed by
relevant teams.

Informs: cockroachdb#111760
Release note: None
craig bot pushed a commit that referenced this issue Oct 17, 2023
112449: clusterversion: rename pre-23.1 version gates r=RaduBerinde a=RaduBerinde

This commit renames all pre-23.1 version keys (except those associated permanent upgrades) to start with `TODO_Delete_`. These keys will be removed by relevant teams.

Informs: #111760
Release note: None

Co-authored-by: Radu Berinde <[email protected]>
@RaduBerinde
Copy link
Member

#112501 is merging soon. Filed #112501 for tracking the removal of the old gates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-release branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-release Release Engineering & Automation Team
Projects
No open projects
Status: Untriaged
Development

No branches or pull requests

5 participants