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: allow running roachtests on AWS #31314

Merged
merged 1 commit into from Oct 12, 2018

Conversation

Projects
None yet
3 participants
@petermattis
Contributor

petermattis commented Oct 12, 2018

Add a --cloud flag (default "gce") which allows running roachtests on
"aws". Refactor some of the GCE specific code to also support AWS.

Fixes #29334

Release note: None

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 12, 2018

Member

This change is Reviewable

Member

cockroach-teamcity commented Oct 12, 2018

This change is Reviewable

@petermattis petermattis requested a review from benesch Oct 12, 2018

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Oct 12, 2018

Contributor

This is still a WIP as roachprod fails when trying to create a cluster on i3 machines as it is using the Google device names. We'll also need to add another teamcity config for running some subset of the roachtests on AWS.

Contributor

petermattis commented Oct 12, 2018

This is still a WIP as roachprod fails when trying to create a cluster on i3 machines as it is using the Google device names. We'll also need to add another teamcity config for running some subset of the roachtests on AWS.

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Oct 12, 2018

Contributor

Oh, I think roachprod is ok. The problem is RemountNoBarrier.

Contributor

petermattis commented Oct 12, 2018

Oh, I think roachprod is ok. The problem is RemountNoBarrier.

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Oct 12, 2018

Contributor

Fixed up RemountNoBarrier and switched to specifying --aws-machine-type-ssd instead of --aws-machine-type. Everything seems to be working in simple testing. For example, roachtest run --cloud=aws kv95/encrypt=false/nodes=1.

Contributor

petermattis commented Oct 12, 2018

Fixed up RemountNoBarrier and switched to specifying --aws-machine-type-ssd instead of --aws-machine-type. Everything seems to be working in simple testing. For example, roachtest run --cloud=aws kv95/encrypt=false/nodes=1.

@petermattis petermattis requested a review from andreimatei Oct 12, 2018

@andreimatei

LGTM

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/cmd/roachtest/cluster.go, line 393 at r1 (raw file):

	fmt.Fprintf(os.Stderr, "unknown machine type: %s\n", s)
	os.Exit(1)

here and elsewhere: log.Fatalf()? Is there a reason there's a lot of os.Exit(1) in roachtest?


pkg/cmd/roachtest/main.go, line 152 at r1 (raw file):

			&artifacts, "artifacts", "artifacts", "path to artifacts directory")
		cmd.Flags().StringVar(
			&cloud, "cloud", cloud, "cloud provider to use (local aws gce)")

what's "local"? How does that, and this flag in general, interact with "--local"? Should that flag go away?

roachtest: allow running roachtests on AWS
Add a `--cloud` flag (default "gce") which allows running roachtests on
"aws". Refactor some of the GCE specific code to also support AWS.

Fixes #29334

Release note: None
@petermattis

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/cmd/roachtest/cluster.go, line 393 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

here and elsewhere: log.Fatalf()? Is there a reason there's a lot of os.Exit(1) in roachtest?

I can't recall offhand. I used fmt.Fprintf and os.Exit for consistency with other roachtest code. I'd like to leave this as-is for this PR. Feel free to determine if there is a good reason for this (i.e. spelunk git blame) and switch to using log.Fatal if there isn't. Note that using util/log isn't a good idea for command line tools as we don't want a stacktrace.


pkg/cmd/roachtest/main.go, line 152 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

what's "local"? How does that, and this flag in general, interact with "--local"? Should that flag go away?

I stole part of the flag description from roachprod. Specifying --cloud=local would not have worked. Perhaps we can make that work, though I think I'd want to keep --local as a synonym for --cloud=local. You're doing a bunch of work in this area right now and if I muck with it concurrently we'll likely just have merge conflicts. Feel free to sanitize this in the rest of your efforts, or leave it and I'll fix it up once you're done.

I've removed local from the flag description.

@petermattis

This comment has been minimized.

Show comment
Hide comment
@petermattis

petermattis Oct 12, 2018

Contributor

bors r=andreimatei

Contributor

petermattis commented Oct 12, 2018

bors r=andreimatei

craig bot pushed a commit that referenced this pull request Oct 12, 2018

Merge #31314
31314: roachtest: allow running roachtests on AWS r=andreimatei a=petermattis

Add a `--cloud` flag (default "gce") which allows running roachtests on
"aws". Refactor some of the GCE specific code to also support AWS.

Fixes #29334

Release note: None

Co-authored-by: Peter Mattis <petermattis@gmail.com>
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot commented Oct 12, 2018

Build succeeded

@craig craig bot merged commit 8f1a283 into cockroachdb:master Oct 12, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@petermattis petermattis deleted the petermattis:pmattis/roachtest-aws branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment