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: perform spec validation on existing clusters #26256

Merged
merged 1 commit into from
May 30, 2018

Conversation

nvanbenschoten
Copy link
Member

This change adds in a new validation step when running tests with the
--cluster flag to verify that the cluster matches the test spec. This
is particularly important for benchmarks, which can produce vastly
different results depending on the machine type they run on.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis
Copy link
Collaborator

:lgtm:


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, all commit checks successful.


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

		//
		// TODO DURING REIVIEW: we could use the roachprod.Cloud struct directly
		// for this if we were ok establishing the dependency.

I'd prefer to avoid that dependency. This looks good to me.


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

		type jsonOutput struct {
			Clusters map[string]struct {
				Vms []struct {

s/Vms/VMs/g


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

				}
			}
			// TODO DURING REVIEW: should we assert correct zones?

That seems overly restrictive. Perhaps we wanted to use different zones for some reason.


Comments from Reviewable

This change adds in a new validation step when running tests with the
`--cluster` flag to verify that the cluster matches the test spec. This
is particularly important for benchmarks, which can produce vastly
different results depending on the machine type they run on.

Release note: None
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/checkNodes branch from 785430a to c4c8935 Compare May 30, 2018 22:49
@nvanbenschoten
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.


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

Previously, petermattis (Peter Mattis) wrote…

I'd prefer to avoid that dependency. This looks good to me.

Done.


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

Previously, petermattis (Peter Mattis) wrote…

s/Vms/VMs/g

Done.


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

Previously, petermattis (Peter Mattis) wrote…

That seems overly restrictive. Perhaps we wanted to use different zones for some reason.

Yeah, that's what I figured. We can add in some extra zone validation if we find that we need it.


Comments from Reviewable

@nvanbenschoten
Copy link
Member Author

bors r+

craig bot pushed a commit that referenced this pull request May 30, 2018
26256: roachtest: perform spec validation on existing clusters r=nvanbenschoten a=nvanbenschoten

This change adds in a new validation step when running tests with the
`--cluster` flag to verify that the cluster matches the test spec. This
is particularly important for benchmarks, which can produce vastly
different results depending on the machine type they run on.

Release note: None

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

craig bot commented May 30, 2018

Build succeeded

@craig craig bot merged commit c4c8935 into cockroachdb:master May 30, 2018
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/checkNodes branch May 30, 2018 23:30
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