cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

roachtest: --use-spot now properly defaults to "no SpotVMs"

Open srosenberg opened this issue 1 year ago • 4 comments
trafficstars

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

srosenberg avatar May 15 '24 22:05 srosenberg

This change is Reviewable

cockroach-teamcity avatar May 15 '24 22:05 cockroach-teamcity

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.

srosenberg avatar May 15 '24 23:05 srosenberg

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? :)

srosenberg avatar May 16 '24 02:05 srosenberg

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? :)

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?

nameisbhaskar avatar May 16 '24 02:05 nameisbhaskar

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.

srosenberg avatar May 16 '24 14:05 srosenberg

Removed --use-spot-always and augmented --use-spot [never|always|auto] as discussed. PTAL!

srosenberg avatar May 21 '24 17:05 srosenberg

Another smoke test with SELECT_PROBABILITY=0.4.

Out of 50 selected with SpotVM, one failed with preemption.

srosenberg avatar May 22 '24 18:05 srosenberg

TFTR!

bors r=DarrylWong,herkolategan,nameisbhaskar

srosenberg avatar May 22 '24 18:05 srosenberg

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. 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.

blathers-crl[bot] avatar May 22 '24 19:05 blathers-crl[bot]