Commit 6bfc8db
153269: roachtest/mixedversion: sync workload binary versions with the cluster version r=DarrylWong a=williamchoe3
Fixes #147374
### Problem
The current workload binary is no longer backwards compatible with certain workloads i.e. `bank` (see original issue above for more information). Previously, the workload binary would just use whatever default binary was staged on the workload node by default. In a test, the test author could explicitly download an older version of workload to circumvent version compatibility issues or use `Workload`'s `override` ([PR](https://github.com/cockroachdb/cockroach/pull/151165/files#diff-11eb39229832689838a549c5cd62b0adeb799fc4e58468ab17268ffcc86c6bd4L829 )) parameter to download a binary matching the cluster version to the workload node when the workload hook is being executed.
#### Change
While the later's solution works, it makes more sense to always just use a workload binary that matches the cluster. If a test wants to use a specific workload binary version though, that functionality remains possible as well.
During `startStep`, while we are staging the cluster at it's initial version, we also stage ~~that version's binary~~ all the versioned binaries in the upgrade plan on the workload node(s). Previously we just left the workload node alone at this step. ~~Then, introduced a new test framework specific step `stageWorkloadBinaryStep` which gets added in `test.Planner.afterUpgradeSteps` which get added to the plan after a cluster finalizes it's upgrade. `stageWorkloadBinaryStep` stages the finalized cluster version's binary on the workload node(s).~~
Removed the call to `UploadCockroach` from `Workload` as when the workload init or run hooks are called, the cluster binary will already exist on the workload node. ~~Kept `override` parameter, but now if override is set, `Workload` will simply just respect the binary that get's passed into the command instead of defaulting the current cluster version.~~
* If a test wants to use a specific Workload binary, they are able to do that if they want without using the Framework's helper function
Added a new helper method to `Helper` which can be used in hooks to abstract a lower level roachtest framework call
```go
clusterupgrade.BinaryPathForVersion(t.rt, h.System.FromVersion, "cockroach")
-->
CockroachBinaryForWorkload(t)
```
Added new mixedversion test option which will add the framework step to stage all versioned cockroach binaries in the upgrade plan to the WorkloadNode
```go
mixedversion.WithWorkloadNodes(c.WorkloadNode())
```
### Example
Given a plan e.g. during the `run startup hooks concurrently` set of steps
```
Plan:
Upgrades: v25.2.5 → <current>
Deployment mode: system-only
Mutators: cluster_setting[kv.transaction.write_buffering.enabled], cluster_setting[kv.rangefeed.buffered_sender.enabled]
Plan:
├── install fixtures for version "v25.2.5" (1)
├── start cluster at version "v25.2.5" (2)
├── wait for all nodes (:1-4) to acknowledge cluster version '25.2' on system tenant (3)
├── stage workload binary on workload node(s) :5 for version(s) v25.2.5, <current> (4) <-- NEW
├── run startup hooks concurrently
│ ├── run "maybe enable tenant features", after 30s delay (5)
│ ├── run "load TPCC dataset", after 500ms delay (6)
│ ├── run "load bank dataset", after 30s delay (7)
│ └── set cluster setting "kv.rangefeed.buffered_sender.enabled" to 'true' on system tenant, after 3m0s delay (8)
└── upgrade cluster from "v25.2.5" to "<current>"
```
### Verification
Adhoc Nightly against all mixed version tests https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_Nightlies_RoachtestNightlyGceBazel/20482572?buildTab=overview&expandBuildDeploymentsSection=false&expandBuildChangesSection=true&hideTestsFromDependencies=false&expandBuildTestsSection=true&hideProblemsFromDependencies=false&expandBuildProblemsSection=true
Added a `gomock` for Cluster in cluster/mock for unit tests
* Removed duplicate Cluster mock in clusterstats
* Wanted to colocate the mocks with the actual interface they are mocking. Then each package that wants to use the mock just needs to import that mock instead of creating a new mock in that package and creating a new generated file for that mock
*
Added datadriven unit test that captures the new post upgrade internal framework step
While looking at `Test` I saw these test specific fields, (not sure what this specific methodology is called), but just curious because my initial thought was to add a mock Cluster implementation, because I have workload node related logic that needs to call cluster interface methods. I guess i could've maybe went with something like below, but I personally don't like intertwining prod logic and test logic. I guess is there any reason we wouldn't want the mock cluster? Besides the boiler plate it creates in the unit tests?
https://github.com/cockroachdb/cockroach/blob/b4738823bcc55547ff431b99684576370760e56d/pkg/cmd/roachtest/roachtestutil/mixedversion/planner_test.go
```
// Test is the main struct callers of this package interact with.
Test struct {
// the following are test-only fields, allowing tests to simulate
// cluster properties without passing a cluster.Cluster
// implementation.
_arch *vm.CPUArch
_isLocal *bool
_getFailer func(name string) (*failures.Failer, error)
```
### mixedversion test refactor
Key
👌: no workload, no change needed
➕: added mixedversion.WithWorkloadNodes(c.WorkloadNode()) test option to stage versioned binaries on workload node
✅: removed hardcoded "./cockroach workload ..." call to something like "%s workload... ", h.CockroachBinaryForWorkload(t)
⚠️ : can't add versioned call because workload logic not defined in a user hook.
* Most of these tests use Workload() which will override the binary with the versioned one by default
* Some of these tests have logic around the Run cmd so calling Workload doesn't provide an easy fix, but the test should continue to work with the unversioned (default) binary
❌: not refactoring bc it's not using bank and the logic is tighty coupled with shared non mixedversion roachtests so would need to decouple, which I think would make the test logic harder to understand and not worth having a 1 line helper
Test Changes
```
👌acceptance/validate-system-schema-after-version-upgrade [sql-foundations] randomized,timeout: 1h0m0s
👌acceptance/version-upgrade [test-eng] randomized,timeout: 2h0m0s
➕✅admission-control/elastic-workload/mixed-version [kv] timeout: 3h0m0s
➕⚠️ backup-restore/mixed-version [disaster-recovery] randomized,timeout: 8h0m0s
➕⚠️ c2c/mixed-version [disaster-recovery]
* 2 clusters, 1 workload node, put the test option to upgrade the workload node in both clusters, techniaclly 1 of them might be redundant, but i just wanna be sure by the time either cluster makes a workload call it's good, not sure if it's the 1st cluster, 2nd cluster, or random which one goes first so just putting in both, also binary existence is checked before staging so no redundent downloads will be done
➕✅cdc/mixed-version/checkpointing [cdc] randomized,timeout: 3h0m0s
➕✅cdc/mixed-versions [cdc] randomized,timeout: 3h0m0s
👌change-replicas/mixed-version [kv] randomized,timeout: 1h0m0s
👌db-console/mixed-version-cypress [obs-prs] timeout: 2h0m0s
* uses workload node for another task that's not running workload so no change is needed
➕✅db-console/mixed-version-endpoints [obs-prs] randomized,timeout: 1h0m0s
👌declarative_schema_changer/job-compatibility-mixed-version-V242-V243 [sql-foundations] randomized
👌~~➕✅~~decommission/mixed-versions [kv] randomized
* ~~this test didn't have a workload node previously and I didn't see a reason not to use one (unlike some other tests that want the extra pressure on the node)~~
* data loading is fine to do on a cluster node (see comments below)
👌follower-reads/mixed-version/single-region [kv] randomized
👌follower-reads/mixed-version/survival=region/locality=global/reads=strong [kv] randomized
👌http-register-routes/mixed-version [obs-prs] randomized,timeout: 1h0m0s
~~➕✅~~import/mixed-versions [sql-queries] (skipped: Issue #143870) randomized
* don't need to use an explicit workload node here (see comments below)
👌jobs/mixed-versions [disaster-recovery] randomized
➕✅⚠️ ldr/mixed-version [disaster-recovery]
* the init is in a hook, i can user the helper, the run isn't in a hook it uses Workload, so i didn't replace it
➕❌multi-region/mixed-version [test-eng] randomized,timeout: 36h0m0s
❌multitenant-upgrade [server] randomized,timeout: 5h0m0s
❌rebalance/by-load/leases/mixed-version [kv] randomized
❌rebalance/by-load/replicas/mixed-version [kv] randomized
❌schemachange/mixed-versions [sql-foundations] randomized
❌schemachange/mixed-versions-compat [sql-foundations]
❌schemachange/secondary-index-multi-version [sql-foundations] randomized
➕⚠️ sql-stats/mixed-version [obs-prs] randomized,timeout: 1h0m0s
➕✅tpcc/mixed-headroom/chaos/n6cpu16 [test-eng] randomized,timeout: 7h0m0s
➕✅tpcc/mixed-headroom/n5cpu16 [test-eng] randomized,timeout: 7h0m0s
👌validate-system-schema-after-version-upgrade/separate-process [sql-foundations]
```
#### Notes
`pkg/cmd/roachtest/clusterstats/BUILD.bazel` I made some edits initially, removed them, and now ./dev gen is generating go_library / go_test deps in a different order 🤷♂️
* Saw in `pkg/cmd/roachtest/roachtestutil/clusterupgrade/clusterupgrade.go` in `clusterupgrade` pkg, `UploadWorkload` but chose not to use it because it's a currently unused path that also eventually calls a deprecated Cluster API function and `UploadCockroach` & `UploadWorkload` are both just wrappers around `uploadBinaryVersion`
153964: roachtest: allow specifying a set of valid architectures r=herkolategan a=DarrylWong
Previously, tests could only specify one valid architecture or enable all of them to be metamorphically chosen. This changes the cluster spec to allow multiple architectures to be specified.
This is now needed because we recently started running FIPS metamorphically on our nightlies. Some tests are not compatible with ARM or FIPS, and we want to only disable execution on those architectures.
Fixes: #153833
Fixes: #154110
Informs: #152781
154476: build: update patched Go to the latest commit r=rail,RaduBerinde a=rickystewart
To consume cockroachdb/go@889a976.
Co-authored-by: William Choe <[email protected]>
Co-authored-by: DarrylWong <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
File tree
48 files changed
+681
-236
lines changed- build
- bazelutil
- teamcity/internal/release/build-and-publish-patched-go
- pkg
- cmd/roachtest
- clusterstats
- cluster/mock
- roachtestutil
- clusterupgrade
- mixedversion
- testdata/planner
- spec
- tests
- perturbation
- gen
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
48 files changed
+681
-236
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
167 | 167 | | |
168 | 168 | | |
169 | 169 | | |
170 | | - | |
171 | | - | |
172 | | - | |
173 | | - | |
174 | | - | |
175 | | - | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
176 | 176 | | |
177 | | - | |
| 177 | + | |
178 | 178 | | |
179 | 179 | | |
180 | 180 | | |
| |||
659 | 659 | | |
660 | 660 | | |
661 | 661 | | |
662 | | - | |
663 | | - | |
| 662 | + | |
664 | 663 | | |
665 | | - | |
| 664 | + | |
666 | 665 | | |
667 | 666 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1188 | 1188 | | |
1189 | 1189 | | |
1190 | 1190 | | |
1191 | | - | |
1192 | | - | |
1193 | | - | |
1194 | | - | |
1195 | | - | |
1196 | | - | |
1197 | | - | |
| 1191 | + | |
| 1192 | + | |
| 1193 | + | |
| 1194 | + | |
| 1195 | + | |
| 1196 | + | |
| 1197 | + | |
1198 | 1198 | | |
1199 | 1199 | | |
1200 | 1200 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1266 | 1266 | | |
1267 | 1267 | | |
1268 | 1268 | | |
| 1269 | + | |
1269 | 1270 | | |
1270 | 1271 | | |
1271 | 1272 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3188 | 3188 | | |
3189 | 3189 | | |
3190 | 3190 | | |
3191 | | - | |
3192 | | - | |
3193 | | - | |
3194 | | - | |
3195 | | - | |
3196 | 3191 | | |
3197 | 3192 | | |
3198 | 3193 | | |
3199 | 3194 | | |
3200 | 3195 | | |
3201 | 3196 | | |
3202 | | - | |
3203 | | - | |
3204 | | - | |
3205 | | - | |
3206 | | - | |
3207 | | - | |
3208 | | - | |
3209 | | - | |
3210 | | - | |
3211 | | - | |
3212 | | - | |
3213 | | - | |
3214 | | - | |
| 3197 | + | |
| 3198 | + | |
| 3199 | + | |
| 3200 | + | |
3215 | 3201 | | |
| 3202 | + | |
| 3203 | + | |
| 3204 | + | |
3216 | 3205 | | |
3217 | 3206 | | |
3218 | 3207 | | |
| |||
3232 | 3221 | | |
3233 | 3222 | | |
3234 | 3223 | | |
| 3224 | + | |
| 3225 | + | |
| 3226 | + | |
| 3227 | + | |
| 3228 | + | |
| 3229 | + | |
| 3230 | + | |
| 3231 | + | |
| 3232 | + | |
| 3233 | + | |
| 3234 | + | |
| 3235 | + | |
| 3236 | + | |
| 3237 | + | |
| 3238 | + | |
| 3239 | + | |
| 3240 | + | |
| 3241 | + | |
| 3242 | + | |
| 3243 | + | |
| 3244 | + | |
| 3245 | + | |
| 3246 | + | |
| 3247 | + | |
| 3248 | + | |
| 3249 | + | |
| 3250 | + | |
| 3251 | + | |
| 3252 | + | |
| 3253 | + | |
| 3254 | + | |
| 3255 | + | |
| 3256 | + | |
| 3257 | + | |
| 3258 | + | |
| 3259 | + | |
| 3260 | + | |
| 3261 | + | |
| 3262 | + | |
| 3263 | + | |
| 3264 | + | |
| 3265 | + | |
| 3266 | + | |
| 3267 | + | |
| 3268 | + | |
| 3269 | + | |
| 3270 | + | |
| 3271 | + | |
| 3272 | + | |
| 3273 | + | |
| 3274 | + | |
| 3275 | + | |
| 3276 | + | |
| 3277 | + | |
| 3278 | + | |
3235 | 3279 | | |
3236 | 3280 | | |
3237 | 3281 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
909 | 909 | | |
910 | 910 | | |
911 | 911 | | |
| 912 | + | |
| 913 | + | |
| 914 | + | |
| 915 | + | |
| 916 | + | |
| 917 | + | |
| 918 | + | |
| 919 | + | |
| 920 | + | |
| 921 | + | |
| 922 | + | |
| 923 | + | |
| 924 | + | |
| 925 | + | |
| 926 | + | |
| 927 | + | |
| 928 | + | |
| 929 | + | |
| 930 | + | |
| 931 | + | |
| 932 | + | |
| 933 | + | |
| 934 | + | |
| 935 | + | |
| 936 | + | |
| 937 | + | |
| 938 | + | |
| 939 | + | |
| 940 | + | |
| 941 | + | |
| 942 | + | |
| 943 | + | |
| 944 | + | |
| 945 | + | |
| 946 | + | |
| 947 | + | |
| 948 | + | |
| 949 | + | |
| 950 | + | |
| 951 | + | |
| 952 | + | |
| 953 | + | |
| 954 | + | |
| 955 | + | |
| 956 | + | |
| 957 | + | |
| 958 | + | |
| 959 | + | |
| 960 | + | |
| 961 | + | |
| 962 | + | |
| 963 | + | |
| 964 | + | |
| 965 | + | |
| 966 | + | |
| 967 | + | |
| 968 | + | |
| 969 | + | |
| 970 | + | |
| 971 | + | |
| 972 | + | |
| 973 | + | |
| 974 | + | |
| 975 | + | |
| 976 | + | |
| 977 | + | |
| 978 | + | |
| 979 | + | |
| 980 | + | |
| 981 | + | |
| 982 | + | |
| 983 | + | |
| 984 | + | |
| 985 | + | |
| 986 | + | |
| 987 | + | |
| 988 | + | |
| 989 | + | |
| 990 | + | |
| 991 | + | |
| 992 | + | |
| 993 | + | |
| 994 | + | |
| 995 | + | |
| 996 | + | |
| 997 | + | |
| 998 | + | |
| 999 | + | |
| 1000 | + | |
| 1001 | + | |
| 1002 | + | |
| 1003 | + | |
| 1004 | + | |
| 1005 | + | |
| 1006 | + | |
| 1007 | + | |
| 1008 | + | |
| 1009 | + | |
| 1010 | + | |
| 1011 | + | |
| 1012 | + | |
| 1013 | + | |
| 1014 | + | |
| 1015 | + | |
| 1016 | + | |
| 1017 | + | |
| 1018 | + | |
| 1019 | + | |
| 1020 | + | |
| 1021 | + | |
| 1022 | + | |
| 1023 | + | |
| 1024 | + | |
| 1025 | + | |
| 1026 | + | |
| 1027 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
18 | | - | |
19 | 18 | | |
20 | 19 | | |
21 | 20 | | |
| |||
36 | 35 | | |
37 | 36 | | |
38 | 37 | | |
| 38 | + | |
39 | 39 | | |
40 | 40 | | |
41 | 41 | | |
| |||
45 | 45 | | |
46 | 46 | | |
47 | 47 | | |
48 | | - | |
49 | 48 | | |
50 | 49 | | |
51 | 50 | | |
52 | 51 | | |
53 | 52 | | |
54 | 53 | | |
| 54 | + | |
55 | 55 | | |
56 | | - | |
| 56 | + | |
57 | 57 | | |
58 | 58 | | |
59 | 59 | | |
| |||
64 | 64 | | |
65 | 65 | | |
66 | 66 | | |
67 | | - | |
68 | 67 | | |
69 | 68 | | |
70 | 69 | | |
| |||
91 | 90 | | |
92 | 91 | | |
93 | 92 | | |
94 | | - | |
95 | | - | |
96 | | - | |
97 | | - | |
98 | | - | |
99 | | - | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | | - | |
105 | | - | |
0 commit comments