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: server,upgrades: bootstrap at binaryMinVersion and run upgrades to BinaryVersionOverride #99587

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Mar 26, 2023

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

/cc @cockroachdb/release


Previously, tests that set BinaryVersionOverride would bootstrap
their test clusters at this overridden value. As part of this bootstrap
we would initialize the cluster with data as it appears on binaryVersion
i.e. system tables would be created using the schemas defined on the current binaryVersion.
Most tests that set BinaryVersionOverride would then manually call into upgrade.Upgrade
to move the cluster version forward. Since we have already bootstrapped the initial
data at binaryVersion these upgrades would not really run. We would just be testing
their idempotency unless the test did some gymnastics to "undo" the bootstrap and
then test the actual migration. So effectively, the BinaryVersionOverride made
the server think it was running that version but none of the associated upgrade
logic to reach that version was exercised.

In 23.1 we baked in the initial data that is bootstrapped on binaryMinSupportedVersion.
This development allows us to bootstrap the test cluster to binaryMinSupportedVersion
and actually step through the upgrades until BinaryVersionOverride before running the
test.

The new behaviour of setting this override is described below:

This value, when set, influences test cluster/server creation in two
different ways:

 Case 1:
 ------
 If the test has not overridden the
 `cluster.Settings.Version.BinaryMinSupportedVersion`, then the cluster will
 be bootstrapped at `binaryMinSupportedVersion`  (if this server is the one
 bootstrapping the cluster). After all the servers in the test cluster have
 been started, `SET CLUSTER SETTING version = BinaryVersionOverride` will be
 run to step through the upgrades until the specified override.

 Case 2:
 ------
 If the test has overridden the
 `cluster.Settings.Version.BinaryMinSupportedVersion` then it is not safe
 for us to bootstrap at `binaryMinSupportedVersion` as it might be less than
 the overridden minimum supported version. Furthermore, we do not have the
 initial cluster data (system tables etc.) to bootstrap at the overridden
 minimum supported version. In this case we bootstrap at
 `BinaryVersionOverride` and populate the cluster with initial data
 corresponding to the `binaryVersion`. In other words no upgrades are
 *really* run and the server only thinks that it is running at
 `BinaryVersionOverride`. Tests that fall in this category should be audited
 for correctness.
The version that we bootstrap at is also used when advertising this
server's binary version when sending out join requests.

Informs: #97762
Fixes: #99651

Release note: None


Release justification: low risk, test only change that increases the coverage of upgrade tests

…naryVersionOverride

Previously, tests that set BinaryVersionOverride would bootstrap
their test clusters at this overridden value. As part of this bootstrap
we would initialize the cluster with data as it appears on `binaryVersion`
i.e. system tables would be created using the schemas defined on the current `binaryVersion`.
Most tests that set BinaryVersionOverride would then manually call into `upgrade.Upgrade`
to move the cluster version forward. Since we have already bootstrapped the initial
data at `binaryVersion` these upgrades would not *really* run. We would just be testing
their idempotency unless the test did some gymnastics to "undo" the bootstrap and
then test the actual migration. So effectively, the `BinaryVersionOverride` made
the server think it was running that version but none of the associated upgrade
logic to reach that version was exercised.

In 23.1 we baked in the initial data that is bootstrapped on `binaryMinSupportedVersion`.
This development allows us to bootstrap the test cluster to `binaryMinSupportedVersion`
and *actually* step through the upgrades until `BinaryVersionOverride` before running the
test.

The new behaviour of setting this override is described below:

This value, when set, influences test cluster/server creation in two
different ways:

	 Case 1:
	 ------
	 If the test has not overridden the
	 `cluster.Settings.Version.BinaryMinSupportedVersion`, then the cluster will
	 be bootstrapped at `binaryMinSupportedVersion`  (if this server is the one
	 bootstrapping the cluster). After all the servers in the test cluster have
	 been started, `SET CLUSTER SETTING version = BinaryVersionOverride` will be
	 run to step through the upgrades until the specified override.

	 Case 2:
	 ------
	 If the test has overridden the
	 `cluster.Settings.Version.BinaryMinSupportedVersion` then it is not safe
	 for us to bootstrap at `binaryMinSupportedVersion` as it might be less than
	 the overridden minimum supported version. Furthermore, we do not have the
	 initial cluster data (system tables etc.) to bootstrap at the overridden
	 minimum supported version. In this case we bootstrap at
	 `BinaryVersionOverride` and populate the cluster with initial data
	 corresponding to the `binaryVersion`. In other words no upgrades are
	 *really* run and the server only thinks that it is running at
	 `BinaryVersionOverride`. Tests that fall in this category should be audited
	 for correctness.
	The version that we bootstrap at is also used when advertising this
	server's binary version when sending out join requests.

Informs: #97762

Release note: None
@blathers-crl blathers-crl bot requested a review from a team March 26, 2023 00:54
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-99082 branch from b995ddf to feee281 Compare March 26, 2023 00:54
@blathers-crl blathers-crl bot requested review from a team as code owners March 26, 2023 00:54
@blathers-crl blathers-crl bot requested review from herkolategan and renatolabs and removed request for a team March 26, 2023 00:54
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-99082 branch from 5d4586c to 95406c1 Compare March 26, 2023 00:54
@blathers-crl
Copy link
Author

blathers-crl bot commented Mar 26, 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?

@blathers-crl blathers-crl bot closed this Mar 26, 2023
@blathers-crl blathers-crl bot deleted the blathers/backport-release-23.1-99082 branch March 26, 2023 00:54
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru restored the blathers/backport-release-23.1-99082 branch March 26, 2023 01:13
@adityamaru adityamaru reopened this Mar 26, 2023
@adityamaru adityamaru merged commit 82c780a into release-23.1 Mar 27, 2023
@adityamaru adityamaru deleted the blathers/backport-release-23.1-99082 branch March 27, 2023 21:07
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