roachtest: port docker-based cdc tests to roachtest by danhhz · Pull Request #31707 · cockroachdb/cockroach

danhhz · 2018-10-22T20:23:54Z

The docker ones occasionally flaked on teamcity. The acceptance test
momentum right now is definitely with roachtests, so move to them so we
get all the hottest bug fixes.

This also helps set up end-to-end avro testing, which was going to be
hard with the docker based tests because some port nonsense. (The full
gory details are in a huge comment in the file this commit deletes.)

Also makes the cdc roachtests work with --local.

Also make the bank workload's update fire more often. Previously it only
did anything if the account had enough balance to cover the transfer.
This models the real world, but in the test (where the invariant is just
that the accounts sum to 0) it was just making extra work.

Closes #30740
Closes #27292

Release note: None

danhhz · 2018-10-22T21:10:14Z

The teamcity failure made me realize that we don't currently have a way to run this on every PR because of the enterprise license requirement. It was using the TestingEnableEnterprise hook before but that doesn't work with a binary. And there are security concerns with doing it via env vars in an every-pr build. Maybe just running it in the nightlies is sufficient. Thoughts @mrtracy?

mrtracy

As for the inability to run these on every PR; is there anything being captured by the bank test (and existing roachtests) that isn't being effectively captured by unit tests, other than kafka interaction? And, is there any way for us to get any effective kafka sink tests as unit tests?

This roachtest still provides value, I think hooking the validator up to kafka is a good innovation here. However, we need to make sure we're not being too skimpy in our checkin tests.

mrtracy · 2018-10-23T07:10:11Z

pkg/cmd/roachtest/cdc.go

+	m.Wait()
+}
+
+func runCDCErrors(ctx context.Context, t *test, c *cluster) {


This doesn't need to be a roachtest at all; none of these cases even contact kafka!

mrtracy · 2018-10-23T07:11:31Z

pkg/cmd/roachtest/cdc.go

+	})
+	m.Go(func(ctx context.Context) error {
+		defer workloadCancel()
+		l, err := c.l.ChildLogger(`changefeed`)


Apparently we should use t.l.ChildLogger, instead of c.. Don't know why, was just informed of that in my last PR though.

mrtracy · 2018-10-23T07:19:05Z

pkg/cmd/roachtest/cdc.go

+	m := newMonitor(workloadCtx, c, crdbNodes)
+	var doneAtomic int64
+	m.Go(func(ctx context.Context) error {
+		err := c.RunE(ctx, workloadNode, `./workload run bank {pgurl:0}`)


How does this know when to stop? To me this looks like it will run forever.

cockroach-teamcity · 2018-10-23T14:42:14Z

This change is

danhhz

Kafka interaction is the only thing unique about these tests at this point. The only way I could think to do that with unit tests is the thing I'm deleting. It was super fragile, hard to extend, and took weeks to get set up, I've really become convinced it wasn't the way to go. (Seriously, go read the huge comment block in the file I deleted in this commit.)

Now that I think about it, we may be able to bring these back on merge-to-master tests, but I'd like to leave that for a a followup.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/roachtest/cdc.go, line 200 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

How does this know when to stop? To me this looks like it will run forever.

it's cancelled using the context when the below goroutine exits: defer workloadCancel()

pkg/cmd/roachtest/cdc.go, line 208 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Apparently we should use t.l.ChildLogger, instead of c.. Don't know why, was just informed of that in my last PR though.

Oh, must be part of andrei's refactor. Done

pkg/cmd/roachtest/cdc.go, line 275 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

This doesn't need to be a roachtest at all; none of these cases even contact kafka!

Heh, thanks for pushing me on my laziness. It was connecting to the sink before doing these checks, but that's silly. Done

Took this opportunity to clean up TestChangefeedErrors which was identical between sinkless and enterprise, so it was needlessly running twice.

benesch · 2018-10-23T16:10:57Z

Now that I think about it, we may be able to bring these back on merge-to-master tests, but I'd like to leave that for a a followup.

I'd recommend against doing that, actually. If you fail the merge-to-master build you'll prevent binaries from getting built. (And if you somehow soft fail it, no one will ever notice that it failed.)

Maybe I'm misunderstanding what you meant, though. I think a TeamCity build that separately listened for pushes to master could be workable.

danhhz · 2018-10-23T16:16:32Z

Yeah, I'm not sold on this, so happy to give up on it if it doesn't make sense. Just to be clear we're talking about the same thing, my strawman is the test wouldn't be run while iterating on a PR, but would be run by bors and prevent the merge if we somehow broke it. Also a concern here is having something fail only when bors-ing, which is going to be even more frustrating than usual if it flakes.

The docker ones occasionally flaked on teamcity. The acceptance test momentum right now is definitely with roachtests, so move to them so we get all the hottest bug fixes. This also helps set up end-to-end avro testing, which was going to be hard with the docker based tests because some port nonsense. (The full gory details are in a huge comment in the file this commit deletes.) Also makes the cdc roachtests work with --local. Also make the bank workload's update fire more often. Previously it only did anything if the account had enough balance to cover the transfer. This models the real world, but in the test (where the invariant is just that the accounts sum to 0) it was just making extra work. Closes cockroachdb#30740 Closes cockroachdb#27292 Release note: None

mrtracy · 2018-10-23T23:34:35Z

This is LGTM, questions about nightly tests aside.

danhhz · 2018-10-24T14:50:40Z

@benesch came up with a great idea about how to use a Vault plugin to solve the every-pr enterprise license problem for good. In the meantime, it's not ideal, but I'm comfortable with only running these tests in the nightly build.

TFTR!

bors r=mrtracy

31707: roachtest: port docker-based cdc tests to roachtest r=mrtracy a=danhhz The docker ones occasionally flaked on teamcity. The acceptance test momentum right now is definitely with roachtests, so move to them so we get all the hottest bug fixes. This also helps set up end-to-end avro testing, which was going to be hard with the docker based tests because some port nonsense. (The full gory details are in a huge comment in the file this commit deletes.) Also makes the cdc roachtests work with --local. Also make the bank workload's update fire more often. Previously it only did anything if the account had enough balance to cover the transfer. This models the real world, but in the test (where the invariant is just that the accounts sum to 0) it was just making extra work. Closes #30740 Closes #27292 Release note: None Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>

craig · 2018-10-24T15:21:18Z

Build succeeded

GitHub CI (Cockroach)

andreimatei

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/roachtest/cdc.go, line 360 at r2 (raw file):

		Nodes:      nodes(4),
		Stable:     true, // DO NOT COPY to new tests
		SubTests: []testSpec{{

Any reason for a test with a single subtest (as opposed to just a test)? I don't think this arrangement currently carries any semantics; does it?
Asking cause I'm generally trying to get rid of subtests.

andreimatei

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/cmd/roachtest/cdc.go, line 364 at r2 (raw file):

			Stable: true, // DO NOT COPY to new tests
			Run: func(ctx context.Context, t *test, c *cluster) {
				c.Wipe(ctx)

also, is there a point to this wipe()?

danhhz · 2018-10-24T18:19:50Z

There will be more tests here in the future, but I'm fine making this not a subtest

andreimatei · 2018-10-24T18:21:35Z

ack

…

On Wed, Oct 24, 2018 at 2:20 PM Daniel Harrison ***@***.***> wrote: There will be more tests here in the future, but I'm fine making this not a subtest — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#31707 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXBcZCPsnxcGGpsK5JCcDvy2YTyy3Pmks5uoK9ZgaJpZM4X0HpR> .

danhhz requested review from a team and mrtracy October 22, 2018 20:23

danhhz force-pushed the cdc_acceptance branch from 9cd301a to 32f85a4 Compare October 22, 2018 21:05

danhhz force-pushed the cdc_acceptance branch from 32f85a4 to 85f35c0 Compare October 22, 2018 21:33

mrtracy suggested changes Oct 23, 2018

View reviewed changes

danhhz force-pushed the cdc_acceptance branch from 85f35c0 to 76303e8 Compare October 23, 2018 15:11

danhhz commented Oct 23, 2018

View reviewed changes

danhhz force-pushed the cdc_acceptance branch from 76303e8 to 0fecc22 Compare October 23, 2018 16:18

mrtracy approved these changes Oct 23, 2018

View reviewed changes

mrtracy mentioned this pull request Oct 24, 2018

cdc: Re-enable Fingerprint Validator #31774

Merged

craig bot merged commit 0fecc22 into cockroachdb:master Oct 24, 2018

andreimatei reviewed Oct 24, 2018

View reviewed changes

danhhz deleted the cdc_acceptance branch October 25, 2018 15:47

danhhz mentioned this pull request Nov 30, 2018

release-2.1: various changefeed fixes #32757

Merged

Conversation

danhhz commented Oct 22, 2018

Uh oh!

danhhz commented Oct 22, 2018

Uh oh!

mrtracy left a comment

Choose a reason for hiding this comment

Uh oh!

mrtracy Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

mrtracy Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

mrtracy Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

cockroach-teamcity commented Oct 23, 2018

Uh oh!

danhhz left a comment

Choose a reason for hiding this comment

Uh oh!

benesch commented Oct 23, 2018

Uh oh!

danhhz commented Oct 23, 2018

Uh oh!

mrtracy commented Oct 23, 2018

Uh oh!

danhhz commented Oct 24, 2018

Uh oh!

craig bot commented Oct 24, 2018

Build succeeded

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

danhhz commented Oct 24, 2018

Uh oh!

andreimatei commented Oct 24, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants