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

testutils: automate maintenance of predecessor release information #105231

Merged
merged 3 commits into from
Jul 11, 2023

Conversation

renatolabs
Copy link
Contributor

@renatolabs renatolabs commented Jun 20, 2023

Up to this point, we have manually maintained a predecessor_version.json
file that maps each release series to the latest published
release of the predecessor series. This information is then used
by tests in mixed-version and upgrade contexts.

With the end goal of testing upgrades that do not use the latest
published version, this commit enriches the release information file
to also include which releases have been withdrawn. With that
information, the test harness can choose any patch release of the
predecessor release series excluding withdrawn releases, which could
lead to undesired flakes and noise.

In summary, the following changes are made:

  • replacement of predecessor_version.json file for
    cockroach_releases.yaml. The new file includes which releases have
    been withdrawn.
  • automation of the process of keeping cockroach_releases.yaml
    up to date. The docs team already maintains all this information in a
    structured format [1], so we leverage that here.
  • Predecessor logic is moved from pkg/util to pkg/testutils, where
    it makes more sense, since that code is only used in tests.
  • Introduction of RandomPredecessor and RandomPredecessorHistory,
    which make use of the new data file to pick a random non-withdrawn
    predecessor instead of the latest one.

The first commit in this PR introduces these changes, while the second and
third make use of the new functionality in the tpcc/mixed-headroom roachtests
and the mixedversion framework, respectively.

[1] https://github.com/cockroachdb/docs/blob/main/src/current/_data/releases.yml

Epic: CRDB-19321

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@renatolabs renatolabs force-pushed the rc/predecessors branch 3 times, most recently from 3615a84 to f6b6254 Compare June 21, 2023 15:15
@renatolabs
Copy link
Contributor Author

Ready for review now. I have mixed-versions roachtests running in https://teamcity.cockroachdb.com/viewLog.html?buildId=10619835&tab=queuedBuildOverviewTab.

@renatolabs renatolabs marked this pull request as ready for review June 21, 2023 15:16
@renatolabs renatolabs requested review from a team as code owners June 21, 2023 15:16
@renatolabs renatolabs requested review from herkolategan and smg260 and removed request for a team June 21, 2023 15:16
@renatolabs
Copy link
Contributor Author

schemachange/mixed-version failed in the build above 👆:

"message": "***UNEXPECTED ERROR; Received an unexpected execution error.: ERROR: at or near \"not\": syntax error: unimplemented: this syntax (SQLSTATE 0A000)"

Not sure if this is a schemachange issue or randgen generating an invalid statement. @fqazi FYI.

Either way, doesn't seem related to the changes introduced here.

Copy link
Member

@srosenberg srosenberg left a comment

Choose a reason for hiding this comment

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

Reviewed 22 of 44 files at r1, 12 of 14 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, and @smg260)


pkg/cmd/release/update_releases.go line 31 at r5 (raw file):

var updateReleasesTestFileCmd = &cobra.Command{
	Use:   "update-releases-file",

This is intended to be run manually, right?


pkg/cmd/release/update_releases.go line 200 at r5 (raw file):

		if d.Predecessor == "" {
			if noPredecessors != "" {
				return fmt.Errorf("two release series without known predecessors: %q and %q", name, noPredecessors)

Is "release series" the right terminology? It's just unfamiliar to me.

Copy link
Member

@srosenberg srosenberg left a comment

Choose a reason for hiding this comment

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

Nice work! I am especially excited about the application of RandomPredecessor :)

:lgtm:

Reviewed 3 of 44 files at r1, 1 of 14 files at r5.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, and @smg260)


pkg/testutils/release/releases.go line 95 at r5 (raw file):

// instead of returning a list of the latest patch releases, it will
// return a random non-withdrawn patch release for each release series.
func RandomPredecessorHistory(rng *rand.Rand, v *version.Version, k int) ([]string, error) {

Nice!

Up to this point, we have manually maintained a `predecessor_version.json`
file that maps each release series to the latest published
release of the predecessor series. This information is then used
by tests in mixed-version and upgrade contexts.

With the end goal of testing upgrades that do not use the latest
published version, this commit enriches the release information file
to also include which releases have been _withdrawn_. With that
information, the test harness can choose any patch release of the
predecessor release series excluding withdrawn releases, which could
lead to undesired flakes and noise.

In summary, the following changes are made:

* replacement of `predecessor_version.json` file for
`cockroach_releases.yaml`. The new file includes which releases have
been withdrawn.
* automation of the process of keeping `cockroach_releases.yaml`
up to date. The docs team already maintains all this information in a
structured format [1], so we leverage that here.
* Predecessor logic is moved from `pkg/util` to `pkg/testutils`, where
it makes more sense, since that code is only used in tests.
* Introduction of `RandomPredecessor` and `RandomPredecessorHistory`,
which make use of the new data file to pick a random non-withdrawn
predecessor instead of the latest one.

In following commits, tests (such as roachtests and logic tests) may
choose to use this new functionality instead of always picking the
latest predecessor.

[1] https://github.com/cockroachdb/docs/blob/main/src/current/_data/releases.yml

Epic: CRDB-19321

Release note: None
This makes use of the newly introduced `release.RandomPredecessor`
function to run upgrade tests from a random predecessor release (as
long as it's not been withdrawn).

Epic: CRDB-19321

Release note: None
Copy link
Contributor Author

@renatolabs renatolabs left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @herkolategan, @smg260, and @srosenberg)


pkg/cmd/release/update_releases.go line 31 at r5 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

This is intended to be run manually, right?

For now, yes. There's some ongoing work to automate this and run these commands automatically on a TC build, but we're not there yet.


pkg/cmd/release/update_releases.go line 200 at r5 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

Is "release series" the right terminology? It's just unfamiliar to me.

To the best of my knowledge, this is the terminology used by the release team.

@renatolabs
Copy link
Contributor Author

Rebased and fixed conflicts with master.

@rafiss FYI, cockroach-go logic tests could also use this API to test upgrades from a random predecessor release instead of the latest patch release. Changing the line below to use release.RandomPredecessor should be all that's needed. I'll leave the decision of whether to adopt it up to you since there might be test failures to deal with.

bootstrapVersion, err := version.PredecessorVersion(*upgradeVersion)

@renatolabs
Copy link
Contributor Author

bors r=srosenberg

TFTR!

@craig
Copy link
Contributor

craig bot commented Jul 11, 2023

Build succeeded:

@craig craig bot merged commit c566d1f into cockroachdb:master Jul 11, 2023
renatolabs added a commit to renatolabs/cockroach that referenced this pull request Jul 12, 2023
In cockroachdb#105231, the order returned by the predecessor history function was
inadvertently changed: instead of returning a slice from oldest to
newest release, the order was instead reversed (newest to oldest).

That breaks the (currently only) caller of `PredecessorHistory` with a
parameter greater than 1: `tpcc/mixed-headroom/multiple-upgrades`. As
a result, it would attempt an impossible upgrade path from a 23.1
release to a 22.2 release.

This commit fixes the API by returning the release histories from
oldest to newest as expected, as this is the more sensible API.

Fixes: cockroachdb#106716

Release note: None
craig bot pushed a commit that referenced this pull request Jul 13, 2023
106718: testutils: return predecessor history from least to most recent r=srosenberg a=renatolabs

In #105231, the order returned by the predecessor history function was inadvertently changed: instead of returning a slice from oldest to newest release, the order was instead reversed (newest to oldest).

That breaks the (currently only) caller of `PredecessorHistory` with a parameter greater than 1: `tpcc/mixed-headroom/multiple-upgrades`. As a result, it would attempt an impossible upgrade path from a 23.1 release to a 22.2 release.

This commit fixes the API by returning the release histories from oldest to newest as expected, as this is the more sensible API.

Fixes: #106716

Release note: None

Co-authored-by: Renato Costa <[email protected]>
renatolabs added a commit to renatolabs/cockroach that referenced this pull request Jul 17, 2023
In cockroachdb#105231, the order returned by the predecessor history function was
inadvertently changed: instead of returning a slice from oldest to
newest release, the order was instead reversed (newest to oldest).

That breaks the (currently only) caller of `PredecessorHistory` with a
parameter greater than 1: `tpcc/mixed-headroom/multiple-upgrades`. As
a result, it would attempt an impossible upgrade path from a 23.1
release to a 22.2 release.

This commit fixes the API by returning the release histories from
oldest to newest as expected, as this is the more sensible API.

Fixes: cockroachdb#106716

Release note: None
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