roachtest: prevent aws roachtest panic #100286
Conversation
After cockroachdb#99723 merged as a bandaid for cockroachdb#98783, the aws roachtest nightly began to panic because of a different roachtest papercut cockroachdb#96655. Specifically, because roachtest filters which tests run on which cloud within the evaluation of the test closure, tests meant to run on gce will still get registered in an AWS run. During the registration of the gce test `restore/tpce/400GB/gce/nodes=4/cpus=8/lowmem` _on aws_, the aws test harness panics because the aws roachprod implementation does not have a low memory cpu configuration. This patch prevents this panic and should be reverted once the pr cockroachdb#99402 merges. Epic: None Release note: None
|
This change is |
|
Let’s rerun the nightly after this one is merged. |
|
TFTR! bors r=rail |
|
Build succeeded: |
|
just kicked off an aws nightly here https://teamcity.cockroachdb.com/viewQueued.html?itemId=9365886&tab=queuedBuildOverviewTab |
| @@ -430,7 +430,7 @@ func (hw hardwareSpecs) makeClusterSpecs(r registry.Registry) spec.ClusterSpec { | |||
| } | |||
| s := r.MakeClusterSpec(hw.nodes, clusterOpts...) | |||
|
|
|||
| if s.Cloud == "aws" && s.VolumeSize != 0 { | |||
| if backupCloud == spec.AWS && s.Cloud == spec.AWS && s.VolumeSize != 0 { | |||
There was a problem hiding this comment.
The second conjunct is a tautology because s.Cloud is empty in this case. Incidentally, none of the existing roachtests set s.Cloud to anything other than gce. 🤷
There was a problem hiding this comment.
@srosenberg I don't believe that's the case, at least on master and 23.1 anymore. In an ad hoc test, I added a print statement in this branch to confirm we take it. We've also confirmed in later 23.1 restore roachtest failures, that we properly removed the d in the aws machine type specification. (see here).
Also, if s.Cloud were not set to aws, I don't think any aws roachtests would work? We seem to rely on this variable to set the right machine type.
There was a problem hiding this comment.
Nm, you're right. I missed the fact that Cloud is passed (r.Cloud below) from the testRegistryImply, which in turns gets initialized before tests are registered,
spec.MakeClusterSpec(r.cloud, r.instanceType, nodeCount, finalOpts...)
After #99723 merged as a bandaid for #98783, the aws roachtest nightly began to panic because of a different roachtest papercut #96655. Specifically, because roachtest filters which tests run on which cloud within the evaluation of the test closure, tests meant to run on gce will still get registered in an AWS run. During the registration of the gce test
restore/tpce/400GB/gce/nodes=4/cpus=8/lowmemon aws, the aws test harness panics because the aws roachprod implementation does not have a low memory cpu configuration. This patch prevents this panic and should be reverted once the pr #99402 merges.Epic: None
Release note: None