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

roachtest: port remaining mixed-version roachtests to the new framework #110528

Closed
13 tasks done
srosenberg opened this issue Sep 13, 2023 · 7 comments · Fixed by #131467
Closed
13 tasks done

roachtest: port remaining mixed-version roachtests to the new framework #110528

srosenberg opened this issue Sep 13, 2023 · 7 comments · Fixed by #131467
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) meta-issue Contains a list of several other issues. T-testeng TestEng Team

Comments

@srosenberg
Copy link
Member

srosenberg commented Sep 13, 2023

The original roachtest mixed-version framework [1] was born sometime in 2017. While it has gone through many iterations and improvements since then, its limitations have inspired a new, declarative approach [2], which attempts to increase coverage of the mixed-version states while reducing boilerplate, thereby allowing the author to focus on the salient bits, namely workloads and invariants. The new mixed-version framework has already proven its worth by finding 12+ new bugs, having ported only two pre-existing roachtests,

Most recently, an initial port of cdc/mixed-versions was merged. This leaves the following roachtests which remain to be ported,

In virtue of the new framework, even a mechanical approach to porting the above is likely to yield a higher coverage of the mixed-version states. However, we ask that the authors take a deliberate approach by studying the limitations of the current implementation and correcting them, when feasible, with the help of the new framework.

In addition to the above, TestEng is planning to add a long-running workload utilizing sqlancer in the near term. (Unlike existing workloads, sqlancer is more antagonistic and randomized, albeit synthetic.) Mid-to-longer term, automated failure injection and other features will be added to the framework, thereby increasing overall coverage of error-handling (e.g., verifying that migration steps are indeed idempotent).

[1] #18057
[2] https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/roachtestutil/mixedversion/README.md

Jira issue: CRDB-31478

@srosenberg
Copy link
Member Author

Linking follow-up CDC issue to add more randomization: #108943

@rafiss rafiss added the T-testeng TestEng Team label Sep 19, 2023
@blathers-crl
Copy link

blathers-crl bot commented Sep 19, 2023

cc @cockroachdb/test-eng

@rafiss rafiss added the meta-issue Contains a list of several other issues. label Sep 19, 2023
@nvanbenschoten
Copy link
Member

TestEng is planning to add a long-running workload utilizing sqlancer in the near term. (Unlike existing workloads, sqlancer is more antagonistic and randomized, albeit synthetic.)

Does sqlancer run transactions or mutations? If not, it will be limited in the coverage that it provides, especially of KV, Replication, and Storage. We would benefit from some workload that also performs mutations, so we should consider also adding a long-running workload like TPC-C into the mix.

@srosenberg
Copy link
Member Author

Does sqlancer run transactions or mutations? If not, it will be limited in the coverage that it provides, especially of KV, Replication, and Storage. We would benefit from some workload that also performs mutations, so we should consider also adding a long-running workload like TPC-C into the mix.

Its transactions are fairly basic; it does run mutations [1]. However, it's relatively easy to extend it. E.g., I'd like to add more concurrent (schema) mutations.

we should consider also adding a long-running workload like TPC-C into the mix

Agreed, we already have it in the mix.

[1] https://github.com/sqlancer/sqlancer/blob/cddff69308e9bcdbf0ed9abc5b77b57590431ff7/src/sqlancer/cockroachdb/CockroachDBProvider.java#L52

DarrylWong added a commit to DarrylWong/fork that referenced this issue Oct 31, 2023
This commit deletes the version/mixed roachtests. These tests
do very little, only testing that upgrades finish. This
is tested by other mixed-version tests, rendering these tests
redundant.

Note that these tests did test upgrades for clusters of size 5,
something we don't do elsewhere. However it isn't worth it to
have a seperate test for this.

Release note: None

Epic: CRDB-19321
Fixes: cockroachdb#110538
Informs: cockroachdb#110528
craig bot pushed a commit that referenced this issue Oct 31, 2023
113280: streamingccl: add setting to disable mux rangefeeds r=msbutler a=stevendanna

Since mux rangefeeds are still default off, we want the ability to opt out of them if we find an issue with them.

Epic: none

Release note: None

113323: roachtest: delete version/mixed tests r=renatolabs a=DarrylWong

This commit deletes the version/mixed roachtests. These tests do very little, only testing that upgrades finish. This is tested by other mixed-version tests, rendering these tests redundant.

Note that these tests did test upgrades for clusters of size 5, something we don't do elsewhere. However it isn't worth it to have a separate test for this.

Release note: None

Epic: CRDB-19321
Fixes: #110538
Informs: #110528

Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: DarrylWong <[email protected]>
blathers-crl bot pushed a commit that referenced this issue Oct 31, 2023
This commit deletes the version/mixed roachtests. These tests
do very little, only testing that upgrades finish. This
is tested by other mixed-version tests, rendering these tests
redundant.

Note that these tests did test upgrades for clusters of size 5,
something we don't do elsewhere. However it isn't worth it to
have a seperate test for this.

Release note: None

Epic: CRDB-19321
Fixes: #110538
Informs: #110528
kvoli added a commit to kvoli/cockroach that referenced this issue Dec 4, 2023
Port`rebalance/by-load/leases/mixed-version` and
`rebalance/by-load/replicas/mixed-version` to the new mixed-version
framework as described in cockroachdb#110528.

Part of: cockroachdb#110528
Resolves: cockroachdb#115134
Resolves: cockroachdb#114549
Release note: None
kvoli added a commit to kvoli/cockroach that referenced this issue Dec 5, 2023
Port`rebalance/by-load/leases/mixed-version` and
`rebalance/by-load/replicas/mixed-version` to the new mixed-version
framework as described in cockroachdb#110528.

Part of: cockroachdb#110528
Resolves: cockroachdb#115134
Resolves: cockroachdb#115135
Release note: None
kvoli added a commit to kvoli/cockroach that referenced this issue Dec 5, 2023
Port `rebalance/by-load/leases/mixed-version` and
`rebalance/by-load/replicas/mixed-version` to the new mixed-version
framework as described in cockroachdb#110528.

Part of: cockroachdb#110528
Resolves: cockroachdb#115134
Resolves: cockroachdb#115135
Release note: None
craig bot pushed a commit that referenced this issue Dec 8, 2023
114695: builtins: remove crdb_internal.gc_tenant r=rafiss a=rafiss

This is an internal function that was deprecated a while ago. It's no longer used since DestroyTenant is used instead.

Epic: None
Release note: None

115559: roachtest: port rebalance/by-load/*/mixed-version to new framework r=nvanbenschoten a=kvoli

Port `rebalance/by-load/leases/mixed-version` and
`rebalance/by-load/replicas/mixed-version` to the new mixed-version
framework as described in #110528.

Part of: #110528
Resolves: #115134
Resolves: #115135
Release note: None


---

`ts_util.go` provides helper functions for querying the cluster
timeseries database over http. These methods previously made use of
`httputil.PostJSON` for requesting timeseries. This commit updates to
use `httputil.PostProtobuf`, in order to allow ignoring optional fields
when the server doesn't support them; namely `TenantID`.

Epic: none
Release note: None

115732: workflows: update to run unit tests in GitHub Actions job r=rail a=rickystewart

Epic: CRDB-8308
Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Austen McClernon <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Dec 11, 2023
…mework

Fixes cockroachdb#115133.

This commit ports the `follower-reads/mixed-version/single-region`
roachtest to the new mixed-version framework, as described in cockroachdb#110528.

One note for reviewers is that the new mixed-version framework requires
clusters to have 4 nodes (unless we want to disable fixtures), so part
of the work here was to adapt the test to work with 4 nodes. That's
worthwhile anyway, because testing the case where a gateway has no
replica (leaseholder or follower) for the table is interesting.

Another note is that the fixtures that the framework restores already
have a database called "test", so this commit renames its own multi-region
database to "mr_db" to avoid confusion.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Dec 11, 2023
…mework

Fixes cockroachdb#115133.

This commit ports the `follower-reads/mixed-version/single-region`
roachtest to the new mixed-version framework, as described in cockroachdb#110528.

One note for reviewers is that the new mixed-version framework requires
clusters to have 4 nodes (unless we want to disable fixtures), so part
of the work here was to adapt the test to work with 4 nodes. That's
worthwhile anyway, because testing the case where a gateway has no
replica (leaseholder or follower) for the table is interesting.

Another note is that the fixtures that the framework restores already
have a database called "test", so this commit renames its own multi-region
database to "mr_db" to avoid confusion.

Release note: None
craig bot pushed a commit that referenced this issue Dec 11, 2023
115317: roachtest: port follower-reads/mixed-version/single-region to new framework r=nvanbenschoten a=nvanbenschoten

Fixes #115133.

This commit ports the `follower-reads/mixed-version/single-region` roachtest to the new mixed-version framework, as described in #110528.

One note for reviewers is that the new mixed-version framework requires clusters to have 4 nodes (unless we want to disable fixtures), so part of the work here was to adapt the test to work with 4 nodes. That's worthwhile anyway, because testing the case where a gateway has no replica (leaseholder or follower) for the table is interesting.

Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Jan 4, 2024
…mework

Fixes cockroachdb#115133.

This commit ports the `follower-reads/mixed-version/single-region`
roachtest to the new mixed-version framework, as described in cockroachdb#110528.

One note for reviewers is that the new mixed-version framework requires
clusters to have 4 nodes (unless we want to disable fixtures), so part
of the work here was to adapt the test to work with 4 nodes. That's
worthwhile anyway, because testing the case where a gateway has no
replica (leaseholder or follower) for the table is interesting.

Another note is that the fixtures that the framework restores already
have a database called "test", so this commit renames its own multi-region
database to "mr_db" to avoid confusion.

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Jan 4, 2024
…mework

Fixes cockroachdb#115133.

This commit ports the `follower-reads/mixed-version/single-region`
roachtest to the new mixed-version framework, as described in cockroachdb#110528.

One note for reviewers is that the new mixed-version framework requires
clusters to have 4 nodes (unless we want to disable fixtures), so part
of the work here was to adapt the test to work with 4 nodes. That's
worthwhile anyway, because testing the case where a gateway has no
replica (leaseholder or follower) for the table is interesting.

Another note is that the fixtures that the framework restores already
have a database called "test", so this commit renames its own multi-region
database to "mr_db" to avoid confusion.

Release note: None
@srosenberg
Copy link
Member Author

Last remaining test to be ported is decommission/mixed-versions, thus moving the parent issue to KV.

@srosenberg srosenberg added T-kv KV Team and removed T-testeng TestEng Team labels Sep 26, 2024
@renatolabs
Copy link
Contributor

Last remaining test to be ported is decommission/mixed-versions, thus moving the parent issue to KV.

We ported decommissions/mixed-versions in #131252. I'll keep this issue open until we remove the newUpgradeTest dependency from the automation that generates fixtures each release (generate-fixtures), then we'll be able to delete the old upgrade API and that would be a nice way to close this issue.

@renatolabs renatolabs added the T-testeng TestEng Team label Sep 26, 2024
Copy link

blathers-crl bot commented Sep 26, 2024

cc @cockroachdb/test-eng

@renatolabs renatolabs removed the T-kv KV Team label Sep 26, 2024
craig bot pushed a commit that referenced this issue Sep 27, 2024
131467: roachtest: update `generate-fixtures` to use `clusterupgrade` functions r=srosenberg a=renatolabs

This commit updates the `generate-fixtures` roachtest, used to automate the generation of fixtures every release, so that it uses plain `clusterupgrade` functions instead of the `newUpgradeTest` API.

The old "step" functions only provided thin wrappers around the functionality of `clusterupgrade`. With this change, we are now able to completely remove the `newUpgradeTest` API which has been deprecated for a while in favor of the `mixedversion` framework.

Fixes: #110528

Release note: None

Co-authored-by: Renato Costa <[email protected]>
@craig craig bot closed this as completed in d08e2f1 Sep 27, 2024
cthumuluru-crdb pushed a commit to cthumuluru-crdb/cockroach that referenced this issue Oct 1, 2024
This commit updates the `generate-fixtures` roachtest, used to
automate the generation of fixtures every release, so that it uses
plain `clusterupgrade` functions instead of the `newUpgradeTest` API.

The old "step" functions only provided thin wrappers around the
functionality of `clusterupgrade`. With this change, we are now able
to completely remove the `newUpgradeTest` API which has been
deprecated for a while in favor of the `mixedversion` framework.

Fixes: cockroachdb#110528

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) meta-issue Contains a list of several other issues. T-testeng TestEng Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants