roachtest: add acceptance/decommission #29488

petermattis · 2018-09-03T16:41:51Z

Move the decommission acceptance test to a new acceptance/decommission
roachtest.

Fixes #29151

Release note: None

cockroach-teamcity · 2018-09-03T16:41:58Z

This change is

petermattis · 2018-09-03T16:43:03Z

The first commit is #29470.

This removes the last usage of RunLocal from pkg/acceptance tests.

petermattis · 2018-09-03T19:02:00Z

github-pull-request-make is return an empty pkg from pkgsFromDiff which is causing test and testrace CI runs to fail. Taking a look.

petermattis · 2018-09-03T19:12:31Z

Added a commit which should fix the github-pull-request-make problem. We apparently don't delete tests very often.

tbg

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, 5 of 5 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/roachtest/decommission.go, line 261 at r3 (raw file):

		extraArgs ...string,
	) (string, error) {
		args := []string{"ssh", c.makeNodes(c.Node(runNode)), "--", cockroach}

Isn't this quite roundabout? Why not c.RunE(ctx, c.Node(runNode), "./cockroach", ...)

I think this might also reintroduce these hangs about passing stdout into child processes.

pkg/cmd/roachtest/decommission.go, line 273 at r3 (raw file):

		return buf.String(), err
	}

Any changes below this line that I should review? I skimmed it and it looks like the old test.

petermattis

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/roachtest/decommission.go, line 261 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Isn't this quite roundabout? Why not c.RunE(ctx, c.Node(runNode), "./cockroach", ...)

I think this might also reintroduce these hangs about passing stdout into child processes.

Hmm, good point. The reason I'm not using RunE is that I need to capture stdout. I guess I could avoid the teeing to c.l.stdout, though that is really nice for debugging. Let me take another look at this.

pkg/cmd/roachtest/decommission.go, line 273 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Any changes below this line that I should review? I skimmed it and it looks like the old test.

No, everything below here was directly adapted from the old test.

tbg · 2018-09-04T14:54:26Z

We have `RunWithBuffer`, doesn't that help?

On Tue, Sep 4, 2018 at 4:52 PM Peter Mattis ***@***.***> wrote: ***@***.**** commented on this pull request. *Reviewable <https://reviewable.io/reviews/cockroachdb/cockroach/29488>* status: [image:

] complete! 0 of 0 LGTMs obtained ------------------------------ *pkg/cmd/roachtest/decommission.go, line 261 at r3 <https://reviewable.io/reviews/cockroachdb/cockroach/29488#-LLZzUoV3cb9fxNMCfjE:-LL_33yZDjdyh-h1pe5n:b-jw6fwy> (raw file <https://github.com/cockroachdb/cockroach/blob/2ecf6678fb8b545f6644c1ec830fdd8f382e4c59/pkg/cmd/roachtest/decommission.go#L261>):* *Previously, tschottdorf (Tobias Schottdorf) wrote…* Isn't this quite roundabout? Why not c.RunE(ctx, c.Node(runNode), "./cockroach", ...) I think this might also reintroduce these hangs about passing stdout into child processes. Hmm, good point. The reason I'm not using RunE is that I need to capture stdout. I guess I could avoid the teeing to c.l.stdout, though that is really nice for debugging. Let me take another look at this. ------------------------------ *pkg/cmd/roachtest/decommission.go, line 273 at r3 <https://reviewable.io/reviews/cockroachdb/cockroach/29488#-LLZzr5i5Q5TIhepdEwM:-LL_3A5X9E2ZI2neUnJs:bnw7l6q> (raw file <https://github.com/cockroachdb/cockroach/blob/2ecf6678fb8b545f6644c1ec830fdd8f382e4c59/pkg/cmd/roachtest/decommission.go#L273>):* *Previously, tschottdorf (Tobias Schottdorf) wrote…* Any changes below this line that I should review? I skimmed it and it looks like the old test. No, everything below here was directly adapted from the old test. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#29488 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE135IuBt-D7Y9PMKXAu9pgC46dozyo2ks5uXpOqgaJpZM4WX32f> .

--

…

-- Tobias

petermattis

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/roachtest/decommission.go, line 261 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Hmm, good point. The reason I'm not using RunE is that I need to capture stdout. I guess I could avoid the teeing to c.l.stdout, though that is really nice for debugging. Let me take another look at this.

Ok, I've changed this to use cluster.RunWithBuffer. No more fancy teeing. Oh well.

benesch

Reviewed 1 of 1 files at r2, 5 of 5 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/github-pull-request-make/main.go, line 119 at r5 (raw file):

				case len(curTestName) > 0:
					if !(curPkgName == "build" && curTestName == "TestStyle") {
						fmt.Printf("adding test: name=%s pkg=%s\n", curTestName, curPkgName)

Did you mean to leave this in?

When deleting a go test it was possible to add a package with an empty name. This in turn would cause CI failure as the top-level cockroach package contains no Go sources. Release note: None

Move the decommission acceptance test to a new acceptance/decommission roachtest. Fixes cockroachdb#29151 Release note: None

petermattis

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/github-pull-request-make/main.go, line 119 at r5 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Did you mean to leave this in?

Nope. Removed.

petermattis · 2018-09-04T18:05:21Z

bors r=benesch,tschottdorf

petermattis · 2018-09-04T18:28:51Z

(bors crash, I think)

bors r=benesch,tschottdorf

craig · 2018-09-04T18:39:05Z

Merge conflict (retrying...)

petermattis · 2018-09-04T20:05:08Z

bors r=benesch,tschottdorf

29367: changefeedccl: error when a watched table backfills r=mrtracy,vivekmenezes a=danhhz When a table is currently being backfilled for a schema change (e.g. adding a column with a default value), it's unclear what the expectation is for any rows that are changed during the backfill. Our current invariant is that rows are emitted with an updated timestamp and a later SELECT ... AS OF SYSTEM TIME for that row would exactly match the emitted data. During the backfill, there is nothing we can emit that would definitely meet that invariant (because the backfill can be aborted and rolled back). In the meantime, this commit makes sure that we error whenever a backfill happens, even if it's fast enough that we never get it from leasing. This also paves the way for switching to RangeFeed, which doesn't have the convenient `fetchSpansForTargets` hook that the ExportRequest based poller was (ab)using. Closes #28643 Release note (bug fix): CHANGEFEEDs now error when a watched table backfills (instead of undefined behavior) 29427: docs: Fix replace and link in table_ref diagram r=jseldess a=jseldess Needed for cockroachdb/docs#3682. Release note: None 29488: roachtest: add acceptance/decommission r=benesch,tschottdorf a=petermattis Move the decommission acceptance test to a new acceptance/decommission roachtest. Fixes #29151 Release note: None 29538: stats: document stats-related commands as experimental r=RaduBerinde a=RaduBerinde Update the documentation inside `sql.y` to designate the stats-related statements as experimental. Release note: None 29546: roachtest: skip (intentionally) failing Kafka chaos test r=petermattis a=tschottdorf This test has no chance of passing until Kafka chaos is actually supported (see #28636). Touches #29196. Release note: None 29550: testcluster: make manual replication mode disable the merge queue r=petermattis a=benesch TestClusters have a manual replication mode for use in tests that need to precisely control replication on a cluster. Teach that mode to disable the merge queue in addition to the split and replicate queues. This decreases the number of tests that need to directly disable the merge queue. Release note: None 29552: ui: add attributes to login form so LastPass will autofill it r=vilterp a=vilterp LastPass wasn't confident enough to autofill and autologin without these attributes. Fixes #29529 (fixes for LastPass, but maybe not other PW managers) Release note (admin ui change): Add attributes to the login form to allow LastPass to properly recognize it. Co-authored-by: Daniel Harrison <[email protected]> Co-authored-by: Jesse Seldess <[email protected]> Co-authored-by: Peter Mattis <[email protected]> Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: Tobias Schottdorf <[email protected]> Co-authored-by: Nikhil Benesch <[email protected]> Co-authored-by: Pete Vilter <[email protected]>

craig · 2018-09-04T21:00:47Z

Build succeeded

GitHub CI (Cockroach)

29367: changefeedccl: error when a watched table backfills r=mrtracy,vivekmenezes a=danhhz When a table is currently being backfilled for a schema change (e.g. adding a column with a default value), it's unclear what the expectation is for any rows that are changed during the backfill. Our current invariant is that rows are emitted with an updated timestamp and a later SELECT ... AS OF SYSTEM TIME for that row would exactly match the emitted data. During the backfill, there is nothing we can emit that would definitely meet that invariant (because the backfill can be aborted and rolled back). In the meantime, this commit makes sure that we error whenever a backfill happens, even if it's fast enough that we never get it from leasing. This also paves the way for switching to RangeFeed, which doesn't have the convenient `fetchSpansForTargets` hook that the ExportRequest based poller was (ab)using. Closes #28643 Release note (bug fix): CHANGEFEEDs now error when a watched table backfills (instead of undefined behavior) 29427: docs: Fix replace and link in table_ref diagram r=jseldess a=jseldess Needed for cockroachdb/docs#3682. Release note: None 29488: roachtest: add acceptance/decommission r=benesch,tschottdorf a=petermattis Move the decommission acceptance test to a new acceptance/decommission roachtest. Fixes #29151 Release note: None 29538: stats: document stats-related commands as experimental r=RaduBerinde a=RaduBerinde Update the documentation inside `sql.y` to designate the stats-related statements as experimental. Release note: None 29546: roachtest: skip (intentionally) failing Kafka chaos test r=petermattis a=tschottdorf This test has no chance of passing until Kafka chaos is actually supported (see #28636). Touches #29196. Release note: None 29550: testcluster: make manual replication mode disable the merge queue r=petermattis a=benesch TestClusters have a manual replication mode for use in tests that need to precisely control replication on a cluster. Teach that mode to disable the merge queue in addition to the split and replicate queues. This decreases the number of tests that need to directly disable the merge queue. Release note: None 29552: ui: add attributes to login form so LastPass will autofill it r=vilterp a=vilterp LastPass wasn't confident enough to autofill and autologin without these attributes. Fixes #29529 (fixes for LastPass, but maybe not other PW managers) Release note (admin ui change): Add attributes to the login form to allow LastPass to properly recognize it. Co-authored-by: Daniel Harrison <[email protected]> Co-authored-by: Jesse Seldess <[email protected]> Co-authored-by: Peter Mattis <[email protected]> Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: Tobias Schottdorf <[email protected]> Co-authored-by: Nikhil Benesch <[email protected]> Co-authored-by: Pete Vilter <[email protected]>

petermattis requested review from tbg and benesch September 3, 2018 16:42

petermattis force-pushed the pmattis/roachtest-decommission branch from 83fdde2 to 2ecf667 Compare September 3, 2018 19:12

tbg reviewed Sep 4, 2018

View reviewed changes

petermattis commented Sep 4, 2018

View reviewed changes

petermattis force-pushed the pmattis/roachtest-decommission branch 2 times, most recently from 1e51020 to a8c050f Compare September 4, 2018 15:22

benesch approved these changes Sep 4, 2018

View reviewed changes

petermattis added 2 commits September 4, 2018 13:07

github-pull-request-make: don't add packages with empty names

dc68d7f

When deleting a go test it was possible to add a package with an empty name. This in turn would cause CI failure as the top-level cockroach package contains no Go sources. Release note: None

roachtest: add acceptance/decommission

9dd2f22

Move the decommission acceptance test to a new acceptance/decommission roachtest. Fixes cockroachdb#29151 Release note: None

petermattis commented Sep 4, 2018

View reviewed changes

petermattis force-pushed the pmattis/roachtest-decommission branch from a8c050f to 9dd2f22 Compare September 4, 2018 17:10

craig bot merged commit 9dd2f22 into cockroachdb:master Sep 4, 2018

petermattis deleted the pmattis/roachtest-decommission branch September 4, 2018 21:03

petermattis mentioned this pull request Sep 5, 2018

release-2.1: backport all of the acceptance -> roachtest changes #29589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: add acceptance/decommission #29488

roachtest: add acceptance/decommission #29488

petermattis commented Sep 3, 2018

cockroach-teamcity commented Sep 3, 2018

petermattis commented Sep 3, 2018

petermattis commented Sep 3, 2018

petermattis commented Sep 3, 2018

tbg left a comment

petermattis left a comment

tbg commented Sep 4, 2018 via email

petermattis left a comment

benesch left a comment

petermattis left a comment

petermattis commented Sep 4, 2018

petermattis commented Sep 4, 2018

craig bot commented Sep 4, 2018

petermattis commented Sep 4, 2018

craig bot commented Sep 4, 2018

roachtest: add acceptance/decommission #29488

roachtest: add acceptance/decommission #29488

Conversation

petermattis commented Sep 3, 2018

cockroach-teamcity commented Sep 3, 2018

petermattis commented Sep 3, 2018

petermattis commented Sep 3, 2018

petermattis commented Sep 3, 2018

tbg left a comment

Choose a reason for hiding this comment

petermattis left a comment

Choose a reason for hiding this comment

tbg commented Sep 4, 2018 via email

petermattis left a comment

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

petermattis left a comment

Choose a reason for hiding this comment

petermattis commented Sep 4, 2018

petermattis commented Sep 4, 2018

craig bot commented Sep 4, 2018

Merge conflict (retrying...)

petermattis commented Sep 4, 2018

craig bot commented Sep 4, 2018

Build succeeded