c2c: deflake c2c/shutdown roachtests by msbutler · Pull Request #102382 · cockroachdb/cockroach

msbutler · 2023-04-26T22:03:25Z

c2c: deflake c2c/shutdown roachtests

This patch addresses to roachtest failure modes:
- Prevents roachtest failure if a query fails during a node shutdown.

- Prevents the src cluster from returning a single node topology, which could
  cause the stream ingestion job to hang if the participating src node gets
shut down. Longer term, automatic replanning will prevent this. Specifically,
this patch changes the kv workload to split and scatter the kv table across the
cluster before the c2c job begins.

Fixes #101898
Fixes #102111

This patch also makes it easier to reproduce c2c roachtest failures by plumbing
a random seed to several components of the roachtest driver.

Release note: None


c2c: rename completeStreamIngestion to applyCutoverTime

Release note: none


workload: add --scatter flag to kv workload

The user can now run `./workload init kv --scatter ....` which scatters the kv
table across the cluster after the initial data load. This flag is best used
with `--splits`, `--max-block-bytes`, and `--insert-count`.

Epic: none

Release note: none

cockroach-teamcity · 2023-04-26T22:03:35Z

This change is

lidorcarmel

looks good, a couple of small things..

lidorcarmel · 2023-05-08T17:30:13Z

pkg/cmd/roachtest/tests/cluster_to_cluster.go

 			// Every C2C phase should last at least 30 seconds, so introduce a little
 			// bit of random waiting before node shutdown to ensure the shutdown occurs
 			// once we're settled into the target phase.
-			randomSleep := time.Duration(rand.Intn(6))
+			//
+			// Sleep for a minimum of 5 seconds to ensure the stream processors have
+			// observed the cutover signal.
+			randomSleep := time.Duration(5 + rand.Intn(6))
 			rrd.t.L().Printf("In target phase! Take a %d second power nap", randomSleep)
 			time.Sleep(randomSleep * time.Second)


I know my comment isn't really about this pr but should waitForTargetPhase do what it says, and just wait for the target phase without any sleep? and then the caller (node shutdown or something else) do the sleep instead?
when reading this code it's not so clear why node shutdown is mentioned in the comment.
WDYT?

Makes sense, see the change i made.

lidorcarmel · 2023-05-08T17:32:50Z

pkg/cmd/roachtest/tests/cluster_to_cluster.go

 	removeTenantRateLimiters(rd.t, rd.setup.dst.sysSQL, rd.setup.dst.name)

-	lv := makeLatencyVerifier("stream-ingestion", 0, 2*time.Minute, rd.t.L(), getStreamIngestionJobInfo, rd.t.Status, false)
+	lv := makeLatencyVerifier("stream-ingestion", 0, 2*time.Minute, rd.t.L(), getStreamIngestionJobInfo, rd.t.Status, true)


why do we want to ignore errors here?
don't we want the test to fail if we can't read from crdb_internal.system_jobs?

good q: any query can fail if a node shuts down around the time of query execution. To ensure the latencyVerifier stays alive during a node shutdown event, we tolerate any of its failed queries. I made the code clearer here.

stevendanna

I wonder if it would also help to explicitly scatter the ranges.

The user can now run `./workload init kv --scatter ....` which scatters the kv table across the cluster after the initial data load. This flag is best used with `--splits`, `--max-block-bytes`, and `--insert-count`. Epic: none Release note: none

Release note: none

msbutler · 2023-05-10T19:50:39Z

good idea. I added a patch to the workload cmd here. My ad hoc tests suggests that c2c is much more likely to be distributed!

renatolabs

All ignorable nits, don't mind me.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lidorcarmel and @msbutler)

pkg/cmd/roachtest/tests/cluster_to_cluster.go line 243 at r5 (raw file):

func (kv replicateKV) sourceInitCmd(tenantName string, nodes option.NodeListOption) string {
	cmd := roachtestutil.NewCommand(`./workload init kv`)

Nit: you can chain all of these calls which arguably makes it nicer to read:

return roachtestutil.NewCommand("./workload init kv").
    Flag("foo", 1).
    Flag("bar", 2).
    String()

pkg/cmd/roachtest/tests/cluster_to_cluster.go line 245 at r5 (raw file):

	cmd := roachtestutil.NewCommand(`./workload init kv`)
	cmd.MaybeFlag(kv.initRows > 0, "insert-count", kv.initRows)
	cmd.MaybeFlag(kv.initRows > 0, "max-block-bytes", kv.maxBlockBytes)

Did you want to check for maxBlockBytes here?

pkg/cmd/roachtest/tests/cluster_to_cluster.go line 248 at r5 (raw file):

	cmd.Flag("splits", 100)
	cmd.Option("scatter")
	cmd.Arg(fmt.Sprintf("{pgurl%s:%s}", nodes, tenantName))

Nit: Arg is for single argument, and it already understands Printf-style formatting, so you can just do Arg("{pgurl%s:%s}", nodes, tenantName)

pkg/cmd/roachtest/tests/cluster_to_cluster.go line 1006 at r5 (raw file):

	// once we're fully settled into the target phase (e.g. the stream ingestion
	// processors have observed the cutover signal).
	randomSleep := time.Duration(5 + rrd.rng.Intn(6))

Nit: making randomSleep a time.Duration (i.e., number of nanoseconds), even though the actual intended meaning is for it to be a number of seconds is slightly confusing. Suggestion:

dur := (5 + rrd.rng.Intn(6)) * time.Second
t.L().Printf("taking a %s power nap", dur)
time.Sleep(dur)

This patch addresses to roachtest failure modes: - Prevents roachtest failure if a query fails during a node shutdown. - Prevents the src cluster from returning a single node topology, which could cause the stream ingestion job to hang if the participating src node gets shut down. Longer term, automatic replanning will prevent this. Specifically, this patch changes the kv workload to split and scatter the kv table across the cluster before the c2c job begins. Fixes cockroachdb#101898 Fixes cockroachdb#102111 This patch also makes it easier to reproduce c2c roachtest failures by plumbing a random seed to several components of the roachtest driver. Release note: None

msbutler

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lidorcarmel and @renatolabs)

pkg/cmd/roachtest/tests/cluster_to_cluster.go line 243 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Nit: you can chain all of these calls which arguably makes it nicer to read:
return roachtestutil.NewCommand("./workload init kv").
    Flag("foo", 1).
    Flag("bar", 2).
    String()

TIL. Done.

pkg/cmd/roachtest/tests/cluster_to_cluster.go line 245 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Did you want to check for maxBlockBytes here?

Yeah I did this bc the parameter only seems relevant if i actually insert data, and bc I distrust the workload package's parameter handling logic :D

pkg/cmd/roachtest/tests/cluster_to_cluster.go line 248 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Nit: Arg is for single argument, and it already understands Printf-style formatting, so you can just do Arg("{pgurl%s:%s}", nodes, tenantName)

Done

pkg/cmd/roachtest/tests/cluster_to_cluster.go line 1006 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Nit: making randomSleep a time.Duration (i.e., number of nanoseconds), even though the actual intended meaning is for it to be a number of seconds is slightly confusing. Suggestion:
dur := (5 + rrd.rng.Intn(6)) * time.Second
t.L().Printf("taking a %s power nap", dur)
time.Sleep(dur)

Done

msbutler · 2023-05-11T16:51:09Z

thanks for the nits @renatolabs !

msbutler · 2023-05-11T18:01:15Z

TFTRs!!

bors r=stevendanna

craig · 2023-05-11T18:21:12Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-05-11T20:28:14Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-05-11T20:28:21Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from d16c011 to blathers/backport-release-23.1-102382: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

msbutler added T-disaster-recovery backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only labels Apr 26, 2023

msbutler requested review from lidorcarmel and stevendanna April 26, 2023 22:03

msbutler self-assigned this Apr 26, 2023

msbutler requested a review from a team as a code owner April 26, 2023 22:03

msbutler requested review from herkolategan and renatolabs and removed request for a team April 26, 2023 22:03

msbutler removed request for herkolategan and renatolabs April 26, 2023 22:03

msbutler force-pushed the butler-deflake-shutdown branch 4 times, most recently from fc91a0c to 81a07f9 Compare April 27, 2023 21:52

msbutler changed the title ~~c2c: prevent single node producer job topology in roachtests~~ c2c: deflake c2c/shutdown roachtests Apr 27, 2023

msbutler force-pushed the butler-deflake-shutdown branch 2 times, most recently from 08dd02c to ad9c15d Compare April 28, 2023 18:47

msbutler requested a review from a team as a code owner May 1, 2023 18:06

lidorcarmel reviewed May 8, 2023

View reviewed changes

msbutler force-pushed the butler-deflake-shutdown branch 4 times, most recently from 9f5357e to 60c7fe4 Compare May 9, 2023 19:00

msbutler mentioned this pull request May 10, 2023

roachtest: c2c/shutdown/dest/coordinator failed - failed ingestion job resume after node failure #103008

Closed

stevendanna approved these changes May 10, 2023

View reviewed changes

msbutler force-pushed the butler-deflake-shutdown branch 3 times, most recently from 62d5a16 to 40e4a0e Compare May 10, 2023 18:53

msbutler force-pushed the butler-deflake-shutdown branch from 40e4a0e to 949e12e Compare May 10, 2023 19:07

msbutler added 2 commits May 10, 2023 15:43

c2c: rename completeStreamIngestion to applyCutoverTime

91d8022

Release note: none

msbutler force-pushed the butler-deflake-shutdown branch from 949e12e to bedd7fc Compare May 10, 2023 19:47

renatolabs reviewed May 10, 2023

View reviewed changes

msbutler force-pushed the butler-deflake-shutdown branch from bedd7fc to 093e2dd Compare May 11, 2023 16:48

msbutler commented May 11, 2023

View reviewed changes

craig bot merged commit aa2c52b into cockroachdb:master May 11, 2023

msbutler mentioned this pull request May 12, 2023

release-23.1: c2c: deflake c2c/shutdown roachtests #103195

Merged

Conversation

msbutler commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Apr 26, 2023

Uh oh!

lidorcarmel left a comment

Choose a reason for hiding this comment

Uh oh!

lidorcarmel May 8, 2023

Choose a reason for hiding this comment

Uh oh!

msbutler May 8, 2023

Choose a reason for hiding this comment

Uh oh!

lidorcarmel May 8, 2023

Choose a reason for hiding this comment

Uh oh!

msbutler May 8, 2023

Choose a reason for hiding this comment

Uh oh!

stevendanna left a comment

Choose a reason for hiding this comment

Uh oh!

msbutler commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

renatolabs left a comment

Choose a reason for hiding this comment

Uh oh!

msbutler left a comment

Choose a reason for hiding this comment

Uh oh!

msbutler commented May 11, 2023

Uh oh!

msbutler commented May 11, 2023

Uh oh!

craig bot commented May 11, 2023

Uh oh!

craig bot commented May 11, 2023

Uh oh!

blathers-crl bot commented May 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

msbutler commented Apr 26, 2023 •

edited

Loading

msbutler commented May 10, 2023 •

edited

Loading