roachtest: introduce testCluster interface for pluggable cluster backends#169382
Open
golgeek wants to merge 5 commits intocockroachdb:masterfrom
Open
roachtest: introduce testCluster interface for pluggable cluster backends#169382golgeek wants to merge 5 commits intocockroachdb:masterfrom
golgeek wants to merge 5 commits intocockroachdb:masterfrom
Conversation
Introduce `testCluster`, an internal interface that captures the runner's view of a cluster. The runner currently accesses many `clusterImpl` fields directly; the interface forces these accesses through methods so that alternative cluster backends can be plugged in without touching runner internals. The interface embeds `cluster.Cluster` (the public test API) and adds the lifecycle and configuration methods the runner needs: destroy, save, wipe, cockroach staging, labels, and architecture/encryption knobs. `clusterImpl` satisfies the new interface; no behavior changes. Epic: none Release note: None
Replace `*clusterImpl` with `testCluster` throughout the runner, cluster registry, github issue reporting, and test monitor. All direct field accesses (e.g. `c.arch`, `c.encAtRest`, `c.clusterSettings`) are replaced with the corresponding interface methods introduced in the previous commit. A few methods that were missing from the interface surface are added here: `OS()`, `SetUseDRPC()`, `GetLiveMigrationVMs()`, `Extend()`, `Saved()`, `status()`, and `SetClusterSetting()`. In `github.go`, a nil-interface guard (`isNilTestCluster`) is added because the cluster may be nil when constructing issue info for tests that failed before cluster creation. Epic: none Release note: None
golgeek
requested review from
cpj2195 and
shailendra-patel
and removed request for
a team
Contributor
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Member
|
This change is |
Remove `assertConsistentReplicas` entirely: drop it from the `testCluster` interface and delete the implementation on `clusterImpl`. The method's only caller in `postTestAssertions` now uses `roachtestutil.CheckReplicaDivergenceOnDB` directly, wrapped in a 20-minute timeout. Behavior is unchanged. Epic: none Release note: None
Separate the cluster factory's public entry point (`newCluster`) from the roachprod-specific implementation (`newRoachprodCluster`). This lets a future managed-service backend provide its own `newCluster` without modifying the roachprod path. Introduce the type alias `roachprodCluster = clusterImpl` so that the roachprod backend can be referenced by a self-documenting name. Rename the `roachprodCluster` enum constant to `roachprodClusterType` to avoid the name collision. Epic: none Release note: None
Replace a `c.(*clusterImpl).encAtRest` type assertion in the test
helpers with an interface-based accessor
(`c.(interface{ EncryptedAtRest() bool })`). This removes the last
direct reference to `*clusterImpl` outside the cluster package,
allowing tests to work with any `testCluster` implementation.
Epic: none
Release note: None
golgeek
force-pushed
the
ludo/rt-prepare-alternative-clusters-backend
branch
from
b41b757 to
03aba12
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, the roachtest runner was tightly coupled to
*clusterImpl(the roachprod-backed cluster implementation). The runner, cluster registry, github reporter, and test monitor all accessedclusterImplfields directly, making it impossible to plug in an alternative cluster backend without modifying runner internals.This was inadequate because upcoming work to support managed-service clusters (e.g. CockroachDB Cloud) requires the runner to operate against clusters it does not provision via roachprod.
This patch introduces
testCluster, an internal interface that captures the runner's view of a cluster, and threads it through all runner plumbing in order to decouple the runner from the concrete roachprod implementation. The early commits define the interface and mechanically replace*clusterImplwithtestClusteracross the runner, registry, github reporter, and test monitor. Later commits clean up remaining concrete dependencies: the post-validation consistency check is inlined in the runner, the cluster factory is split into a public entry point and a roachprod-specific implementation, and the last type assertion against*clusterImplin test helpers is replaced with an interface-based accessor.clusterImplcontinues to be the only implementation; no behavior changes.Epic: none
Release note: None