Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

roachtest: use pd-ssd as default, even when VolumeSize is set #104762

Merged

Conversation

lidorcarmel
Copy link
Contributor

The cluster_to_cluster roachtest was setting VolumeSize, and assuming that the default pd-ssd is used, but instead the roachtest was running with standard pd (HDD). If VolumeSize is not set then roachtest is using local ssd, and if it is set then it uses whatever is in the GCEVolumeType. normally roachtest users don't set GCEVolumeType, and therefore roachtest uses an empty string for the pd type (which again means HDD).

Instead, this pr keep the default volume type unless the user asked for something else.

Epic: CRDB-25146

Release note: None

The cluster_to_cluster roachtest was setting VolumeSize, and assuming
that the default pd-ssd is used, but instead the roachtest was running
with standard pd (HDD). If VolumeSize is not set then roachtest is using
local ssd, and if it is set then it uses whatever is in the GCEVolumeType.
normally roachtest users don't set GCEVolumeType, and therefore roachtest
uses an empty string for the pd type (which again means HDD).

Instead, this pr keep the default volume type unless the user asked for
something else.

Epic: CRDB-25146

Release note: None
@lidorcarmel lidorcarmel requested a review from a team as a code owner June 13, 2023 03:15
@lidorcarmel lidorcarmel requested review from herkolategan and srosenberg and removed request for a team June 13, 2023 03:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@lidorcarmel lidorcarmel requested a review from msbutler June 13, 2023 03:15
@msbutler
Copy link
Collaborator

nice catch lidor! Just noting that,

  • this diff will affect the restore roachtests as well
  • the roachprod docs do suggest that pd-ssd is the default:
roachprod create -h | grep 'gce-pd-volume-type'
      --gce-pd-volume-type string                                                Type of the persistent disk volume, only used if local-ssd=false (default "pd-ssd")

But before making this change, do we know what customers usually use? pd-ssd or standard pd?

@msbutler
Copy link
Collaborator

msbutler commented Jun 13, 2023

ah, well actually, this might have been a regression introduced a few weeks ago in #103757

@lidorcarmel
Copy link
Contributor Author

do we know what customers usually use? pd-ssd or standard pd?

pd-ssd.

this might have been a regression introduced a few weeks ago in #103757

can you please explain?

@msbutler
Copy link
Collaborator

the above PR added this line:
https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/roachtest/spec/cluster_spec.go#L186

@lidorcarmel
Copy link
Contributor Author

Thanks!
bors r+ single on

@healthy-pod
Copy link
Contributor

bors r-

@craig
Copy link
Contributor

craig bot commented Jun 13, 2023

Canceled.

@healthy-pod
Copy link
Contributor

bors r+

@healthy-pod
Copy link
Contributor

bors single off

@craig
Copy link
Contributor

craig bot commented Jun 13, 2023

Build failed:

@lidorcarmel
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 14, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jun 14, 2023

Build succeeded:

@craig craig bot merged commit 63a6daa into cockroachdb:master Jun 14, 2023
@lidorcarmel lidorcarmel deleted the lidor_roachtest_volume_type_override branch July 14, 2023 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants