roachtest: randomly run with runtime assertions by default #111949

DarrylWong · 2023-10-06T20:32:02Z

This changes the semantics of t.Cockroach() to use a binary with runtime assertions enabled by default. Performance tests (indicated by the benchmark TestSpec) will be able to continue using the standard binary, without runtime assertions or metamorphic constants, as usual.

This commit also opts-out other tests that cannot easily be run with runtime assertions or metamorphic constants enabled, most often due to timeouts or metamorphic constant incompatibility.

Resolves #86678.
Informs #94986.

Epic: none

Release note: None

cockroach-teamcity · 2023-10-06T20:32:10Z

This change is

DarrylWong · 2023-10-24T17:15:44Z

pkg/cmd/roachtest/tests/kvbench.go

@@ -96,6 +96,7 @@ func registerKVBenchSpec(r registry.Registry, b kvBenchSpec) {
 		Suites:           registry.ManualOnly,
 		Tags:             registry.Tags("manual"),
 		Owner:            registry.OwnerKV,
+		Benchmark:        true, // TODO: Comments imply this is a benchmark test but this was not set


The test name and comments imply this is a benchmark test so I assume the missing benchmark spec is a mistake but if there is a reason why it shouldn't be set let me know.

DarrylWong · 2023-10-26T19:51:07Z

Should be RFAL

Started up Test Run here

And another here

rickystewart · 2023-10-26T20:26:38Z

pkg/cmd/roachtest/test_impl.go

-	cockroach          string // path to main cockroach binary
-	cockroachShort     string // path to cockroach-short binary compiled with --crdb_test build tag
+	cockroach      string // path to main cockroach binary
+	cockroachShort string // path to cockroach-short binary compiled with --crdb_test build tag


Nit: cockroachAssertionsEnabled (or similar, it seems elsewhere we use cockroachEA for "enabled assertions") is a better name than cockroachShort. Yes, the comment explains the real difference between these two things, but everyone sees the variable name, not everyone sees the comment.

rickystewart · 2023-10-26T20:26:59Z

build/teamcity/cockroach/nightlies/roachtest_compile_component.sh

@@ -83,7 +83,7 @@ case "$component" in
    ;;
  roachtest)
    # Roachtest binary.
-    bazel_args=(//pkg/cmd/roachtest)
+    bazel_args=(//pkg/cmd/roachtest --crdb_test)


What benefit does this give exactly?

Stealing from what Stan said:

Note, to fully support interop between a roachtest runner, (executing the roachtest binary), and a node, executing the cockroach binary compiled with assertions (i.e., crdb_test), the roachtest binary must also be compiled with assertions to establish the same serialization on the wire. E.g., KVNemesisSeq is used on the wire only under crdb_test builds. The corresponding (sequence) Container type is conditional on the binary being compiled under crdb_test. (See pkg/kv/kvpb/api.proto) Thus, a request serialized from the roachtest runner is compatible with the cockroach binary iff both binaries are built under crdb_test.

The solution here was to just compile all roachtest binaries with crdb_test since it touches very little code, rather than try and figure out a way to have team city selectively use roachtest binaries based on what the cockroach binary is compiled with.

I can add a comment though to make it more clear.

DarrylWong

but is there a specific reason why we're skipping runtime checks in a few tests that legitimately break?

Specific comments on kv/split tests below but in general no there isn't and I agree with the sentiment we should let them fail.

I'm confused about schemachange/bulkingest`

I want to say the connection failure was just an infra flake since I can't repro it with the same seed and it's never failed before w/ runtime assertions. But I also don't see anything that jumps out to me in the logs so I don't want to write it off just yet. Let me dig into this some more.

PR suggests the timeout was bumped to 60mins

The duration of how long to run the workload was bumped to 60 mins, the timeout is calculated as 2 * duration.

Would be interesting to run a full nightly build in this branch forcing metamorphic builds for all tests, to reveal some flakes we might not have noticed. Did you run a build like that? I'm interested to see the outcome.

Yeah, that's actually what I was doing most of the time. Here's two runs where I ran all of them #1 and #2. Unfortunately you can see they timed out, which is why I reverted it back for the final few runs.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, @rickystewart, and @srosenberg)

pkg/cmd/roachtest/tests/asyncpg.go line 54 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

In pgjdbc, we're forcing the value of this session var -- can't we do this here too?

The asyncpg tests drops the entire database several times throughout the test which also drops session vars. So even setting the default session value like in pgjdbc isn't enough.

pkg/cmd/roachtest/tests/multitenant_distsql.go line 65 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

We shouldn't skip runtime assertions because this test fails IMO: it's precisely why we're doing this! I think the issue will get a lot more eyes when it is reported as a roachtest failure. My reading of the issue you created is that it is a legitimate bug.

SGTM

pkg/cmd/roachtest/tests/multitenant_utils.go line 281 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Should we delete this?

Done.

pkg/cmd/roachtest/tests/split.go line 434 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Same comment re: letting this break.

The root cause of this issue was found and from my understanding it's not really an urgent issue. Also from what I've been told the faulty QPS function is deprecated anyway. When we have a metamorphic exclusion list, we can just disable kv-batch-size and let the other constants/assertions be on.

I'm not opposed to letting this break, I guess I'm more wondering what we do if KV determines the bug isn't worth fixing? Switch it back to t.StandardCockroach()?

renatolabs

Specific comments on kv/split tests below

👍 sounds good, the exception seems reasonable.

I want to say the connection failure was just an infra flake since I can't repro it

I think the connection failure is a red herring, the test timed out after 2h.

Here's two runs where I ran all of them #1 and #2

These are full runs, but are tests running with metamorphic builds all the time? I don't see any custom parameters to force that. Also, when I look at the logs for some tests in that #2 link, I only see the Running with runtime assertions enabled on global seed message on some tests, not all.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @herkolategan, @rickystewart, and @srosenberg)

pkg/cmd/roachtest/test_impl.go line 166 at r6 (raw file):

			// CI build, but we can't assume it exists in every roachtest call.
			if path := t.RuntimeAssertionsCockroach(); path != "" {
				t.l.Printf("Runtime assertions enabled")

For instant knowledge of whether we are running with assertions enabled or not, we could add a log message to both branches.

pkg/cmd/roachtest/tests/asyncpg.go line 54 at r5 (raw file):

Previously, DarrylWong wrote…

The asyncpg tests drops the entire database several times throughout the test which also drops session vars. So even setting the default session value like in pgjdbc isn't enough.

Got it, thanks. Would it make sense to run these tests with assertions enabled, but disabling metamorphic constants?

DarrylWong

I think the connection failure is a red herring, the test timed out after 2h.

Yeah after running it 5+ times I finally got it to timeout on creating indexes... Seems like an actual issue and not a flake but seems weird why it doesn't consistently fail on the same seed. Will have to investigate more.

These are full runs, but are tests running with metamorphic builds all the time?

Yeah I did it in an admittedly not very obvious way of just temporarily replacing t.StandardCockroach() with the runtime binary in the code itself partly because I was tired of typing in a flag every time. I could do another run if you want, more testing wouldn't hurt but it will probably time out.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, @rickystewart, and @srosenberg)

pkg/cmd/roachtest/test_impl.go line 166 at r6 (raw file):

Previously, renatolabs (Renato Costa) wrote…

For instant knowledge of whether we are running with assertions enabled or not, we could add a log message to both branches.

Done.

pkg/cmd/roachtest/tests/asyncpg.go line 54 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Got it, thanks. Would it make sense to run these tests with assertions enabled, but disabling metamorphic constants?

I was under the impression that just disabling metamorphic constants wasn't easily doable in roachtests since they get enabled on init. If it's simple functionality I'd be happy to try adding, but otherwise I wonder how much benefit it would be over a metamorphic exclusion list .

renatolabs

temporarily replacing t.StandardCockroach() with the runtime binary in the code itself

Ah, I see it in the commit now!

Seems like an actual issue

Could be, the last we hear from the index creation is

executing declarative schema change PostCommitPhase stage 2 of 7 with 1 BackfillType op (rollback=false) for CREATE INDEX

2h before the test timed out, so it got stuck there for a really long time. We should probably let sql-foundations investigate this issue when it occurs on master, as it doesn't seem related to anything we're doing.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @herkolategan, @rickystewart, and @srosenberg)

pkg/cmd/roachtest/tests/asyncpg.go line 54 at r5 (raw file):

Previously, DarrylWong wrote…

I was under the impression that just disabling metamorphic constants wasn't easily doable in roachtests since they get enabled on init. If it's simple functionality I'd be happy to try adding, but otherwise I wonder how much benefit it would be over a metamorphic exclusion list .

You can set COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true when starting the cockroach process, and that will disable use of metamorphic constants in CRDB. Runtime checks (gated with buildutil.CrdbTestBuild) will still run.

I'm saying there's still value in running tests with these runtime checks (but without changing any constants) instead of always using "standard cockroach" in cases like asyncpg. But this can be done in follow-up work too.

DarrylWong

We should probably let sql-foundations investigate this issue when it occurs on master,

Sounds good to me

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @renatolabs, @rickystewart, and @srosenberg)

pkg/cmd/roachtest/tests/asyncpg.go line 54 at r5 (raw file):

Previously, renatolabs (Renato Costa) wrote…

You can set COCKROACH_INTERNAL_DISABLE_METAMORPHIC_TESTING=true when starting the cockroach process, and that will disable use of metamorphic constants in CRDB. Runtime checks (gated with buildutil.CrdbTestBuild) will still run.

I'm saying there's still value in running tests with these runtime checks (but without changing any constants) instead of always using "standard cockroach" in cases like asyncpg. But this can be done in follow-up work too.

Yeah that seems easy enough. In that case, we should only be using StandardCockroach for excessive memory usage and timeouts.

Added it in for asyncpg, kv/split and copy/bank/rows=1000000 and made them all t.Cockroach() again. Initial runs seem good.

This changes the semantics of `t.Cockroach()` to use a binary with runtime assertions and metamorphic constants enabled by default. Performance tests (indicated by the benchmark TestSpec) will continue using the standard binary, without runtime assertions, as usual. This commit also opts-out other tests that cannot easily be run with runtime assertions or metamorphic constants enabled, most often due to timeouts and metamorphic constant incompatibility. Resolves cockroachdb#86678. Informs cockroachdb#94986. Epic: none Release note: None

renatolabs

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @herkolategan, @rickystewart, and @srosenberg)

renatolabs

Something I was thinking about: I think it shouldn't be too difficult to add a "cluster param" in [1] indicating whether the run used metamorphic builds or not. I think it would be nice to give teams an instant way to know if we used metamorphic builds without even having to click through TC artifacts. Thoughts?

[1]

cockroach/pkg/cmd/roachtest/github.go

Line 207 in cedd409

clusterParams := map[string]string{

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @herkolategan, @rickystewart, and @srosenberg)

Adds a label indicating that a roachtest failure was on a metamorphic build on github issue posts.

DarrylWong

Done, but unfortunately it's not the cleanest. The binary used is decided at runtime so it's not available through testSpec like the other params are. I had to pass it in as a separate argument which seems a little messy.

It should work but I'm happy to revert the commit and go back to the drawing board if you have any better suggestions.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @herkolategan, @renatolabs, @rickystewart, and @srosenberg)

mikeCRL

Confirmed no impacts to docs dependencies within the relevant files.

renatolabs · 2023-11-02T21:02:55Z

It should work but I'm happy to revert the commit and go back to the drawing board

No, what you did is kind of what I had in mind originally. There's no elegant way to aggregate random decision made at test run-time and solving that problem is out of scope for this PR. Given the value that this brings (immediately knowing if we used metamorphic binaries), I think this is a good trade-off.

DarrylWong · 2023-11-02T21:08:29Z

TFTRs!

bors r=renatolabs, mikeCRL, rickystewart, herkolategan

craig · 2023-11-02T21:44:10Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-11-02T22:19:12Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-11-02T22:19:18Z

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 581e671 to blathers/backport-release-23.2-111949: 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.2.x failed. See errors above.

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

DarrylWong force-pushed the roachtest-ea branch 8 times, most recently from 49bdf6a to 6a0b6e0 Compare October 12, 2023 19:58

renatolabs mentioned this pull request Oct 12, 2023

roachtest: prototype for metamorphic settings during upgrade tests #112226

Closed

DarrylWong force-pushed the roachtest-ea branch 3 times, most recently from 01c896e to 04c9709 Compare October 17, 2023 20:29

DarrylWong commented Oct 24, 2023

View reviewed changes

DarrylWong force-pushed the roachtest-ea branch from 04c9709 to 1d6e2db Compare October 25, 2023 16:01

This was referenced Oct 26, 2023

roachtest: allow roachtests to opt out of specific metamorphic constants #113164

Open

roachtest: multitenant/distsql tests access diagnostics.active_query_dumps.enabled setting and use Span after Finish #113170

Closed

DarrylWong force-pushed the roachtest-ea branch 2 times, most recently from edbf21b to 09f34ee Compare October 26, 2023 19:50

DarrylWong force-pushed the roachtest-ea branch from 09f34ee to 05598c3 Compare October 26, 2023 19:58

DarrylWong marked this pull request as ready for review October 26, 2023 20:08

DarrylWong requested review from a team as code owners October 26, 2023 20:08

DarrylWong requested review from srosenberg and renatolabs and removed request for a team October 26, 2023 20:08

rickystewart approved these changes Oct 26, 2023

View reviewed changes

DarrylWong force-pushed the roachtest-ea branch from 05598c3 to 77d45ad Compare October 26, 2023 21:15

DarrylWong commented Oct 31, 2023

View reviewed changes

DarrylWong force-pushed the roachtest-ea branch from 3c00c4d to 10f3709 Compare October 31, 2023 20:43

renatolabs reviewed Oct 31, 2023

View reviewed changes

DarrylWong force-pushed the roachtest-ea branch 2 times, most recently from c58ba1a to 339f002 Compare October 31, 2023 21:57

DarrylWong commented Oct 31, 2023

View reviewed changes

renatolabs reviewed Nov 1, 2023

View reviewed changes

DarrylWong force-pushed the roachtest-ea branch from 339f002 to 68e5974 Compare November 1, 2023 15:57

DarrylWong commented Nov 1, 2023

View reviewed changes

DarrylWong force-pushed the roachtest-ea branch from 68e5974 to 581e671 Compare November 1, 2023 16:51

renatolabs approved these changes Nov 2, 2023

View reviewed changes

renatolabs reviewed Nov 2, 2023

View reviewed changes

roachtest: add metamorphicbuild label to github posting

01d34f5

Adds a label indicating that a roachtest failure was on a metamorphic build on github issue posts.

DarrylWong commented Nov 2, 2023

View reviewed changes

DarrylWong added T-testeng TestEng Team backport-23.2.x Flags PRs that need to be backported to 23.2. labels Nov 2, 2023

mikeCRL approved these changes Nov 2, 2023

View reviewed changes

craig bot merged commit 722f902 into cockroachdb:master Nov 2, 2023
8 checks passed

This was referenced Nov 3, 2023

release-23.2: roachtest: randomly run with runtime assertions by default #113752

Merged

release-23.1: roachtest: randomly run with runtime assertions by default #113894

Merged

srosenberg mentioned this pull request Nov 16, 2023

roachtest: temporarily disable assertions & add B-metamorphic, B-coverage labels #114615

Closed

miretskiy mentioned this pull request Nov 16, 2023

roachtest/tests: introduce metamorphic testing to cdc #114504

Merged

DarrylWong mentioned this pull request Aug 12, 2024

roachtest: run nightlies with assertions enabled #128841

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: randomly run with runtime assertions by default #111949

roachtest: randomly run with runtime assertions by default #111949

DarrylWong commented Oct 6, 2023 •

edited

Loading

cockroach-teamcity commented Oct 6, 2023

DarrylWong Oct 24, 2023

DarrylWong commented Oct 26, 2023 •

edited

Loading

rickystewart Oct 26, 2023

rickystewart Oct 26, 2023

DarrylWong Oct 26, 2023 •

edited

Loading

DarrylWong left a comment

renatolabs left a comment

DarrylWong left a comment •

edited

Loading

renatolabs left a comment

DarrylWong left a comment •

edited

Loading

renatolabs left a comment

renatolabs left a comment

DarrylWong left a comment

mikeCRL left a comment

renatolabs commented Nov 2, 2023

DarrylWong commented Nov 2, 2023

craig bot commented Nov 2, 2023

craig bot commented Nov 2, 2023

blathers-crl bot commented Nov 2, 2023

roachtest: randomly run with runtime assertions by default #111949

roachtest: randomly run with runtime assertions by default #111949

Conversation

DarrylWong commented Oct 6, 2023 • edited Loading

cockroach-teamcity commented Oct 6, 2023

DarrylWong Oct 24, 2023

Choose a reason for hiding this comment

DarrylWong commented Oct 26, 2023 • edited Loading

rickystewart Oct 26, 2023

Choose a reason for hiding this comment

rickystewart Oct 26, 2023

Choose a reason for hiding this comment

DarrylWong Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

DarrylWong left a comment

Choose a reason for hiding this comment

renatolabs left a comment

Choose a reason for hiding this comment

DarrylWong left a comment • edited Loading

Choose a reason for hiding this comment

renatolabs left a comment

Choose a reason for hiding this comment

DarrylWong left a comment • edited Loading

Choose a reason for hiding this comment

renatolabs left a comment

Choose a reason for hiding this comment

renatolabs left a comment

Choose a reason for hiding this comment

DarrylWong left a comment

Choose a reason for hiding this comment

mikeCRL left a comment

Choose a reason for hiding this comment

renatolabs commented Nov 2, 2023

DarrylWong commented Nov 2, 2023

craig bot commented Nov 2, 2023

craig bot commented Nov 2, 2023

blathers-crl bot commented Nov 2, 2023

DarrylWong commented Oct 6, 2023 •

edited

Loading

DarrylWong commented Oct 26, 2023 •

edited

Loading

DarrylWong Oct 26, 2023 •

edited

Loading

DarrylWong left a comment •

edited

Loading

DarrylWong left a comment •

edited

Loading