cockroach
cockroach copied to clipboard
roachtest: --use-spot now properly defaults to "no SpotVMs"
In [1], the default of UseSpotVM was reinterpreted to mean the framework decides whether or not to create a spot VM. This may impede local (performance) testing when a potential preemption is unwelcome; i.e., may result in loss of work.
This change reverts the original default of "no SpotVMs". It now interprets --use-spot [true] to mean that the framework decides whether or not to create a spot VM, whereas --use-spot false (default) unconditionally disables the use of spot VMs. Furthermore, to allow unconditional creation of spot VMs, the flag --use-spot-always is added. Finally, when running in CI, we pass --use-spot true, at this time, to allow the framework to conditionally create spot VMs, i.e., based on current heuristics.
[1] https://github.com/cockroachdb/cockroach/pull/116778
Epic: none Fixes: #77376 Release note: None
Tested the options --use-spot=true, --use-spot=false and --use-spot-always=true on my gceworker; no issues found.
Triggered GCE Run in TC, with SELECT_PROBABILITY=0.6 as a sanity check.
Thanks for the change! Just a suggestion, can we not make it in a single flag with enumeration?
always,default,never?
We could except we want the default to be never. Ideally, we also want to keep the original flag for backward compatibility. Maybe always, never (default), and maybe? :)
Thanks for the change! Just a suggestion, can we not make it in a single flag with enumeration?
always,default,never?We could except we want the
defaultto benever. Ideally, we also want to keep the original flag for backward compatibility. Maybealways,never(default), andmaybe? :)
got it. actually, it should be just a boolean. We are having this flag just because of the 50% condition that we have today. Basically, If a specific test sets UseSpot true, this new flag can override that going forward. So, it may be ok to keep to flags and remove "use-spot" later?
So, it may be ok to keep to flags and remove "use-spot" later?
We won't be able to remove "use-spot" because we need to allow for disabling spot usage, by default and on request.
Removed --use-spot-always and augmented --use-spot [never|always|auto] as discussed. PTAL!
Another smoke test with SELECT_PROBABILITY=0.4.
Out of 50 selected with SpotVM, one failed with preemption.
TFTR!
bors r=DarrylWong,herkolategan,nameisbhaskar
Build succeeded:
Encountered an error creating backports. Some common things that can go wrong:
- The backport branch might have already existed.
- There was a merge conflict.
- The backport branch contained merge commits.
You might need to create your backport manually using the backport tool.
error creating merge commit from 7c7b7676fd5d7df86541db434a07f56fe9cb935c to blathers/backport-release-23.1-124258: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []
you may need to manually resolve merge conflicts with the backport tool.
Backport to branch 23.1.x failed. See errors above.
error creating merge commit from 7c7b7676fd5d7df86541db434a07f56fe9cb935c to blathers/backport-release-23.2-124258: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []
you may need to manually resolve merge conflicts with the backport tool.
Backport to branch 23.2.x failed. See errors above.
error creating merge commit from 7c7b7676fd5d7df86541db434a07f56fe9cb935c to blathers/backport-release-24.1-124258: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []
you may need to manually resolve merge conflicts with the backport tool.
Backport to branch 24.1.x failed. See errors above.
:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.