Skip to content

roachtest: stop using ClusterSpec.Cloud in test code#111324

Merged
craig[bot] merged 5 commits intocockroachdb:masterfrom
RaduBerinde:make-cluster-spec-cleanup
Oct 5, 2023
Merged

roachtest: stop using ClusterSpec.Cloud in test code#111324
craig[bot] merged 5 commits intocockroachdb:masterfrom
RaduBerinde:make-cluster-spec-cleanup

Conversation

@RaduBerinde
Copy link
Copy Markdown
Member

@RaduBerinde RaduBerinde commented Sep 27, 2023

In this PR we fix all test registration code which was depending on the flags passed to roachtest (indirectly, through TestSpec). In the process, I tried to clean up various things I had to touch.

This makes progress in the direction of #104029 and of making the registry and TestSpec not depend at all on the flags (the flags should come in later, when we create an actual cluster from the spec).

roachtest: spec: simplify Option

This commit reduces boilerplate in the Option code by making it a
func instead of an interface with an apply func. This way each option
can define the function inline instead of having to define a type.

Epic: none
Release note: None

roachtest: spec: provide options for GCE/AWS settings

This commit moves GCE and AWS specific settings to their own
inline structs and adds Options for them.

Epic: none
Release note: None

roachtest: clean up instance type specification

ClusterSpec now provides options for GCE and AWS machine types.

Epic: none
Release note: None

roachtest: clean up zones specification

ClusterSpec now provides options for GCE and AWS zones
specification.

Epic: none
Release note: None

roachtest: stop using ClusterSpec.Cloud in test code

This change removes all remaining uses of ClusterSpec.Cloud except
those internal to roachtest. Code that is part of running a test now
uses Cluster.Cloud() instead.

Informs: #104029
Release note: None

@RaduBerinde RaduBerinde requested a review from a team as a code owner September 27, 2023 03:55
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@RaduBerinde RaduBerinde force-pushed the make-cluster-spec-cleanup branch 4 times, most recently from d863fa7 to ed5bb3e Compare September 30, 2023 20:21
@RaduBerinde RaduBerinde requested a review from a team as a code owner September 30, 2023 20:21
@RaduBerinde
Copy link
Copy Markdown
Member Author

Updated (resolved conflicts).

Copy link
Copy Markdown

@renatolabs renatolabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde, @smg260, and @srosenberg)


build/release/teamcity-mark-build.sh line 31 at r5 (raw file):

  log_into_gcloud
  gawscloud container images add-tag "${gcr_repository}:${TC_BUILD_BRANCH}" "${gcr_repository}:latest-${release_branch}-${build_label}-build"

This doesn't look right?

Copy link
Copy Markdown
Contributor

@smg260 smg260 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all the OnlyGCE actually only GCE? At least one example that worked locally is ledger/nodes=6/multi-az, whether intended to, or not.

Reviewed 8 of 18 files at r4, 20 of 28 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @srosenberg)

@RaduBerinde RaduBerinde force-pushed the make-cluster-spec-cleanup branch from ed5bb3e to 3633eb9 Compare October 3, 2023 15:25
Copy link
Copy Markdown
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTRs!!

It's a multi AZ test, feels like there's no point to run locally (other than perhaps debugging the test). Note that you will always be able to run whatever you want by adding a flag to override the advertised compatibilities.

I will wait for #111633 to go in and address any conflicts.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @renatolabs, @smg260, and @srosenberg)


build/release/teamcity-mark-build.sh line 31 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

This doesn't look right?

Wow.. fat fingers. Fixed.

Copy link
Copy Markdown
Contributor

@smg260 smg260 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 38 files at r6, 37 of 37 files at r7, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @renatolabs and @srosenberg)

This commit reduces boilerplate in the `Option` code by making it a
func instead of an interface with an apply func. This way each option
can define the function inline instead of having to define a type.

Epic: none
Release note: None
This commit moves GCE and AWS specific settings to their own
inline structs and adds Options for them.

Epic: none
Release note: None
`ClusterSpec` now provides options for GCE and AWS machine types.

Epic: none
Release note: None
`ClusterSpec` now provides options for GCE and AWS zones
specification.

Epic: none
Release note: None
This change removes all remaining uses of `ClusterSpec.Cloud` except
those internal to roachtest. Code that is part of running a test now
uses `Cluster.Cloud()` instead.

Informs: cockroachdb#104029
Release note: None
@RaduBerinde RaduBerinde force-pushed the make-cluster-spec-cleanup branch from 3633eb9 to 22a79ff Compare October 4, 2023 20:38
@RaduBerinde
Copy link
Copy Markdown
Member Author

@RaduBerinde
Copy link
Copy Markdown
Member Author

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 5, 2023

Build succeeded:

@craig craig bot merged commit 0ab77b7 into cockroachdb:master Oct 5, 2023
@RaduBerinde RaduBerinde deleted the make-cluster-spec-cleanup branch November 7, 2025 15:11
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.

4 participants