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: add support for subtests #25408

Merged
merged 6 commits into from
May 11, 2018

Conversation

petermattis
Copy link
Collaborator

Fixes #23915

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis petermattis requested review from benesch and bdarnell May 10, 2018 17:24
@petermattis
Copy link
Collaborator Author

The first two commits are #25394. Feel free to review the whole kit and caboodle here.

I haven't been able to successfully run this remotely due to current wifi conditions.

@petermattis petermattis force-pushed the pmattis/roachtest-subtests branch from 19d699c to 6610abb Compare May 10, 2018 17:41
@petermattis
Copy link
Collaborator Author

PS

~ roachtest run -n jepsen | grep RUN | wc -l
      78

@petermattis petermattis force-pushed the pmattis/roachtest-subtests branch 2 times, most recently from 822247a to fb2a0b3 Compare May 11, 2018 14:05
@petermattis petermattis force-pushed the pmattis/roachtest-subtests branch from fb2a0b3 to 9da16de Compare May 11, 2018 14:46
@petermattis
Copy link
Collaborator Author

Fixed a bunch of small bugs this morning. Seems to be working. At least, I can run roachtest run jepsen/sequential and have only the sequential jepsen test run with the various nemeses and it only creates the cluster once.

@petermattis petermattis force-pushed the pmattis/roachtest-subtests branch from 9da16de to b07ff83 Compare May 11, 2018 15:04
@bdarnell
Copy link
Contributor

:lgtm:


Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.


pkg/cmd/roachtest/jepsen.go, line 135 at r1 (raw file):

	// work around JSCH auth error: https://github.com/jepsen-io/jepsen/blob/master/README.md
	for _, ip := range workerIPs {
		host, _, err := net.SplitHostPort(ip)

InternalIP includes the port number? That's confusing. The method should be renamed to InternalAddr instead of InternalIP. Why didn't you need this for nodeFlags above?


Comments from Reviewable

@petermattis petermattis force-pushed the pmattis/roachtest-subtests branch from b07ff83 to 14beb2a Compare May 11, 2018 19:31
@petermattis
Copy link
Collaborator Author

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion.


pkg/cmd/roachtest/jepsen.go, line 135 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

InternalIP includes the port number? That's confusing. The method should be renamed to InternalAddr instead of InternalIP. Why didn't you need this for nodeFlags above?

I did need something similar for nodeFlags. I think you saw the output of a messed up rebased which accidentally dropped this from nodeFlags. I had a similar thought about InternalAddr when driving around just now. I've made that change.


Comments from Reviewable

@petermattis
Copy link
Collaborator Author

bors r=bdarnell

craig bot pushed a commit that referenced this pull request May 11, 2018
25408: roachtest: add support for subtests r=bdarnell a=petermattis

Fixes #23915

Release note: None

Co-authored-by: Peter Mattis <[email protected]>
@craig
Copy link
Contributor

craig bot commented May 11, 2018

Build succeeded

@craig craig bot merged commit 14beb2a into cockroachdb:master May 11, 2018
@petermattis petermattis deleted the pmattis/roachtest-subtests branch May 11, 2018 20:50
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.

3 participants