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

release-23.1: upgrademanager: fix upgrade manager tests that relied on wrong invariants #100867

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Apr 6, 2023

Backport 1/1 commits from #100830 on behalf of @adityamaru.

/cc @cockroachdb/release


The two tests in question were low level tests that upgrade clusters from mock version 41 to 42 and override the upgrade flow to test the internals of the upgrade manager. These mock cluster versions were not tied to our real cluster versions and so were not offset with the DevOffset when a branch is marked as a development branch. For this reason when a branch was a developmentBranch they would sort under all the "real" cluster versions while when a branch was marked as a release branch they would sort above all the "real" cluster versions. This undeterministic behaviour did not play well with certain job migrations that were added this release.

This change rewrites the test to use real cluster versions so that their values are in sync with whether the branch is a developmentBranch or not. This change also removes some overrides to make the test more intuitive. Cocnretely, the test servers now bootstrap at the minimum supported binary version and run all the upgrades until the startCV before executing the body of the test.

Fixes: #100685
Informs: #100552

Release note: None
Release justification: test only fix to resolve a release blocker preventing us from cutting the beta


Release justification:

…ants

The two tests in question were low level tests that upgrade clusters
from mock version 41 to 42 and override the upgrade flow to
test the internals of the upgrade manager. These mock cluster versions
were not tied to our real cluster versions and so were not offset
with the `DevOffset` when a branch is marked as a development branch.
For this reason when a branch was a developmentBranch they would sort
under all the "real" cluster versions while when a branch was marked
as a release branch they would sort above all the "real" cluster versions.
This undeterministic behaviour did not play well with certain job migrations
that were added this release.

This change rewrites the test to use real cluster versions so that their
values are in sync with whether the branch is a developmentBranch or not.
This change also removes some overrides to make the test more intuitive.
Cocnretely, the test servers now bootstrap at the minimum supported binary version
and run all the upgrades until the `startCV` before executing the body of the
test.

Fixes: #100685
Informs: #100552

Release note: None
@blathers-crl blathers-crl bot requested a review from a team April 6, 2023 20:33
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-100830 branch 2 times, most recently from c36290c to bede330 Compare April 6, 2023 20:33
@blathers-crl
Copy link
Author

blathers-crl bot commented Apr 6, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz merged commit 2bb5d17 into release-23.1 Apr 6, 2023
@knz knz deleted the blathers/backport-release-23.1-100830 branch April 6, 2023 22:15
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