roachtest: expand tag functionality to allow filtering by conjunctive… #99402

smg260 · 2023-03-23T17:11:19Z

… tags.

oday, tests are matched if they have any of the specified tags. This patch introduces the ability to match tests which have all specified tags, whilst attempting to maintain backward compatibility.

Commits
1 - expand tag filtering (AND, !)
2 - convert TestSpec.Tags to map[string]struct{}
3 - suppress TeamCity log statement

Epic: none
Fixes: #96655

Release note: None

cockroach-teamcity · 2023-03-23T17:11:28Z

This change is

smg260 · 2023-03-23T17:47:57Z

This PR has deliberately avoid implementing a full syntax to support complex logical tag expressions in favour of simplicity and being mostly backward compatible; some changes have been made[1].

Today, we can filter tests based on any number of tag: args, with tests required to only contain one of them to be considered a match.
e.g. roachtest list tag:owner-admission-control tag:weekly will return all tests which are owned by admission-control or are tagged as "weekly" (21 in the current branch)

However, there is no way to show all weekly tagged admission-control owned tests. This PR introduces the ability to specify a csv for each tag: which treats the tag values within each CSV as conjunctive.
e.g .roachtest list tag:owner-admission-control,weekly will return all tests which have both tags. (6 in current branch)

This can be combined with the original "any-of" behaviour.
e.g .roachtest list tag:owner-admission-control,weekly tag:owner-kv,weekly will return weekly tagged tests from kv or admission-control (7 in current branch)

Tags which must not be present can also be specified.
e.g. roachtest list 'tag:aws,!owner-test-eng' will return all aws tagged tests not owned by test-eng

The linked issue cites roachtest_util.sh which contains an unwieldly regex expression to isolate tests that can be run on AWS. This PR adds the aws tag to the matched tests to simplify the command to using a single tag instead.
e.g. roachtest list tag:aws. Note: due to the backward compatibility of this change, the existing incarnation of the script would still work as expected.

[1]

tag filters are no longer regular expressions, and instead require a full word match. e.g. tag:adm will no longer match admission-control.
there is no longer special treatment of default tags
if only a name filter is specified, then the test will be matched regardless of what tags it contains. Current behaviour will not match if the test contains a tag and none are specified in the filter. e.g. list admission-control/index-overload will return the match a test in this PR, but not in master.
the number of skipped ignored tests for AWS will show has much higher now (all tests - aws) as we're just filtering off tags, which matches all tests first, then ignores them (vs off the name)

smg260 · 2023-03-24T16:52:01Z

TC run for aws nightly

Compared to previous runs on master, tpccbench/nodes=3/cpu=16/enc=true was missing, but is now appropriately tagged in this PR

After cockroachdb#99723 merged as a bandaid for cockroachdb#98783, the aws roachtest nightly began to panic because of a different roachtest papercut cockroachdb#96655. Specifically, because roachtest filters which tests run on which cloud within the evaluation of the test closure, tests meant to run on gce will still get registered in an AWS run. During the registration of the gce test `restore/tpce/400GB/gce/nodes=4/cpus=8/lowmem` _on aws_, the aws test harness panics because the aws roachprod implementation does not have a low memory cpu configuration. This patch prevents this panic and should be reverted once the pr cockroachdb#99402 merges. Epic: None Release note: None

99312: sqlsmith: add DEFAULT expressions to newly added columns r=mgartner a=mgartner Sqlsmith now builds `ALTER TABLE .. ADD COLUMN .. DEFAULT` statements with default expressions that have different types than the column type. This is allowed if the default expression type can be assignment-casted to the column's type. Fixes #98133 Release note: None 99348: testutils: move default test tenant message r=rharding6373 a=herkolategan In order to reduce logging noise but still inform test authors of the default test tenant, the message has been moved to where there is a `testing.TB` interface. Epic: CRDB-18499 99835: opt/execbuilder: add panic catching to buildRoutinePlanGenerator r=mgartner a=mgartner This commit adds a panic catcher to callback functions created in execbuilder and invoked during evaluation of UDFs and correlated subqueries. It matches the panic catcher logic in `buildApplyJoin`. Fixes #98786 Release note: None 100267: roachtest: own autoupgrade to TestEng r=renatolabs a=tbg Discussed in #99479. Epic: none Release note: None 100286: roachtest: prevent aws roachtest panic r=rail a=msbutler After #99723 merged as a bandaid for #98783, the aws roachtest nightly began to panic because of a different roachtest papercut #96655. Specifically, because roachtest filters which tests run on which cloud within the evaluation of the test closure, tests meant to run on gce will still get registered in an AWS run. During the registration of the gce test `restore/tpce/400GB/gce/nodes=4/cpus=8/lowmem` _on aws_, the aws test harness panics because the aws roachprod implementation does not have a low memory cpu configuration. This patch prevents this panic and should be reverted once the pr #99402 merges. Epic: None Release note: None 100294: tenantcapabilitiestestutils: add a missing default case r=ajwerner a=ajwerner The test should fail if we ever add a new type of capability and use it in the data driven test but don't update the test to handle it. Epic: none Follow-up from #100217 (review) Release note: None 100296: rpc: correctly check for nil before cast r=ajwerner a=andrewbaptist As part of the fix of #99104, a cast without a nil check was introduced. This PR addresses that by only casting if it is known to be not nil. Epic: none Fixes: #100275 Release note: None Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Herko Lategan <[email protected]> Co-authored-by: Tobias Grieger <[email protected]> Co-authored-by: Michael Butler <[email protected]> Co-authored-by: ajwerner <[email protected]> Co-authored-by: Andrew Baptist <[email protected]>

After #99723 merged as a bandaid for #98783, the aws roachtest nightly began to panic because of a different roachtest papercut #96655. Specifically, because roachtest filters which tests run on which cloud within the evaluation of the test closure, tests meant to run on gce will still get registered in an AWS run. During the registration of the gce test `restore/tpce/400GB/gce/nodes=4/cpus=8/lowmem` _on aws_, the aws test harness panics because the aws roachprod implementation does not have a low memory cpu configuration. This patch prevents this panic and should be reverted once the pr #99402 merges. Epic: None Release note: None

smg260 · 2023-04-03T20:52:27Z

This PR does not fix one of the quirks of roachtests, which in the case of #100286, resulted in a panic.

The issue stems from the fact that all tests are registered, regardless of what cloud is running. Roachtest authors are responsible for providing a ClusterSpec as part of the TestSpec which gets registered - in the aforementioned example, this is causing a panic. There potentially needs to be error handling when making a clusterspec, which would prevent adding of that test to the registry.

srosenberg

There potentially needs to be error handling when making a clusterspec, which would prevent adding of that test to the registry.

Right. Methinks we could easily validate all clusterspecs against corresponding cloud APIs. This could be done immediately after choosing which roachtests to execute but before allocating any resources.

Compared to previous runs on master, tpccbench/nodes=3/cpu=16/enc=true was missing, but is now appropriately tagged in this PR

How did you find the missing one? Also, looking at master there are 68 executed by only 66 on your branch. Are we missing any?

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

srosenberg

Left a couple of minor comments, otherwise looks good!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @msbutler, @renatolabs, and @smg260)

build/teamcity/util/roachtest_util.sh line 66 at r9 (raw file):

  aws)
    if [ -z "${FILTER}" ]; then
      FILTER="tag:aws"

Much nicer!

pkg/cmd/roachtest/main.go line 193 at r9 (raw file):

   roachtest list acceptance copy/bank/.*false
   roachtest list tag:acceptance

Might be worth updating an example since nothing today uses tag:acceptance. I did a quick and dirty version of roachtest listAll to enumerate all registered tags; pick any from list below,

roachtest listAll |grep tag|awk '{print $NF}'|sort -u|awk -F":" '{print $2}' |tr ',' '\n' |sort -u |sed -E "s/(owner)-.*/\1/g" |sort -u |tr '\n' ','

aws-weekly,awsdms,default,driver,fixtures,manual,orm,owner,pebble,pebble_nightly_write,pebble_nightly_ycsb,pebble_nightly_ycsb_race,quick,release_qualification,roachtest,smoketest,tool,weekly

smg260

How did you find the missing one?

Good ol' side by side browser my branch vs master roachtest nightly.

Are we missing any?

$ ./bin/roachtest list tag:aws | wc -l
68

Numbers are in sync now

Dismissed @srosenberg from a discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @herkolategan, @msbutler, @renatolabs, and @srosenberg)

pkg/cmd/roachtest/main.go line 193 at r9 (raw file):

Previously, srosenberg (Stan Rosenberg) wrote…

Might be worth updating an example since nothing today uses tag:acceptance. I did a quick and dirty version of roachtest listAll to enumerate all registered tags; pick any from list below,
roachtest listAll |grep tag|awk '{print $NF}'|sort -u|awk -F":" '{print $2}' |tr ',' '\n' |sort -u |sed -E "s/(owner)-.*/\1/g" |sort -u |tr '\n' ','

aws-weekly,awsdms,default,driver,fixtures,manual,orm,owner,pebble,pebble_nightly_write,pebble_nightly_ycsb,pebble_nightly_ycsb_race,quick,release_qualification,roachtest,smoketest,tool,weekly

Good idea. Done.

herkolategan

Reviewed 1 of 11 files at r1, 1 of 1 files at r3, 1 of 6 files at r8, 2 of 8 files at r9, 50 of 50 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @msbutler, @renatolabs, and @srosenberg)

renatolabs · 2023-04-04T16:23:29Z

pkg/cmd/roachtest/main.go

@@ -210,6 +220,8 @@ Examples:
 	}
 	listCmd.Flags().BoolVar(
 		&listBench, "bench", false, "list benchmarks instead of tests")
+	listCmd.Flags().StringVar(
+		&cloud, "cloud", cloud, "cloud provider to use (aws, azure, or gce)")


Not clear to me what this new parameter does 🤔

renatolabs · 2023-04-04T16:24:17Z

build/teamcity/util/roachtest_util.sh

-      # isn't supported on AWS at time of writing.
-      TESTS="awsdms|kv(0|95)|^ycsb|tpcc/(headroom/n4cpu16)|tpccbench/(nodes=3/cpu=16)|scbench/randomload/(nodes=3/ops=2000/conc=1)|backup/(KMS/AWS/n3cpu4)|restore/.*/aws"
+    if [ -z "${FILTER}" ]; then
+      FILTER="tag:aws"


smg260

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

pkg/cmd/roachtest/main.go line 224 at r10 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Not clear to me what this new parameter does 🤔

Brings consistency between run and list. A recent PR introduced a test (since patched( which caused a panic because we were running it on AWS; and the panic happened during test registration/MakeClusterSpec due to an invalid memory spec for AWS

Having the cloud here allows us to see what would happen if we list test after their registered with values other than "GCE". Allowed me to replicate the panic by just doing a list. Most people won't use this parameter, but it has value.

… tags. Today, tests are matched if they have any-of the specified tags. This patch introduces the ability to also match tests which have "all-of" specified tags, along with tag negation. This example will list all tests tagged with "aws" or with both "weekly" and "owner-test-eng" tags: "roachtest list tag:aws tag:weekly,owner-test-eng" An example of tag negation which would list all of the tests with tag "weekly" but not "aws": "roachtest list tag:weekly,!aws" Epic: none Fixes: cockroachdb#96655 Release note: None

…t" and "aws" tagged tests are run See cockroachdb#100605

smg260 · 2023-04-05T14:46:39Z

TFTR

bors r=srosenberg,herkolategan

craig · 2023-04-05T15:28:20Z

Build succeeded:

Bazel Essential CI (Cockroach)

smg260 force-pushed the 96655_roachtest_cloud_tags branch from 5f7d901 to 849109d Compare March 23, 2023 18:23

smg260 marked this pull request as ready for review March 23, 2023 19:07

smg260 requested a review from a team as a code owner March 23, 2023 19:07

smg260 requested review from herkolategan, renatolabs and srosenberg and removed request for a team March 23, 2023 19:07

smg260 marked this pull request as draft March 23, 2023 19:08

smg260 force-pushed the 96655_roachtest_cloud_tags branch 5 times, most recently from 3e25cd4 to f1ed77d Compare March 24, 2023 16:39

smg260 marked this pull request as ready for review March 24, 2023 16:50

smg260 requested a review from a team as a code owner March 24, 2023 16:51

smg260 force-pushed the 96655_roachtest_cloud_tags branch 3 times, most recently from 38fe0ee to fd89815 Compare March 28, 2023 18:06

smg260 requested a review from msbutler March 29, 2023 16:54

msbutler mentioned this pull request Mar 31, 2023

roachtest: prevent aws roachtest panic #100286

Merged

blathers-crl bot mentioned this pull request Mar 31, 2023

release-23.1: roachtest: prevent aws roachtest panic #100308

Merged

smg260 force-pushed the 96655_roachtest_cloud_tags branch 2 times, most recently from fd7651f to 5cb0195 Compare April 3, 2023 20:24

smg260 force-pushed the 96655_roachtest_cloud_tags branch from 5cb0195 to f0cd82c Compare April 3, 2023 21:08

srosenberg reviewed Apr 3, 2023

View reviewed changes

srosenberg approved these changes Apr 3, 2023

View reviewed changes

smg260 force-pushed the 96655_roachtest_cloud_tags branch from f0cd82c to 189cbad Compare April 4, 2023 00:11

smg260 commented Apr 4, 2023

View reviewed changes

herkolategan approved these changes Apr 4, 2023

View reviewed changes

smg260 mentioned this pull request Apr 4, 2023

roachtest: refactor actual tags assigned to tests #100605

Closed

renatolabs reviewed Apr 4, 2023

View reviewed changes

smg260 force-pushed the 96655_roachtest_cloud_tags branch from 189cbad to f349f9c Compare April 4, 2023 16:27

smg260 commented Apr 4, 2023

View reviewed changes

smg260 force-pushed the 96655_roachtest_cloud_tags branch from f349f9c to f9bd3f5 Compare April 4, 2023 18:58

Miral Gadani added 4 commits April 4, 2023 19:49

roachtest: add tag filters to GCE nightly roachtest to ensure "defaul…

ca3bd58

…t" and "aws" tagged tests are run See cockroachdb#100605

roachtest: convert tags from []string to map[string]struct{}

2587fbe

roachtest: log ##teamcity[testFinished..] only on teamcity

85e005b

smg260 force-pushed the 96655_roachtest_cloud_tags branch from f9bd3f5 to 85e005b Compare April 4, 2023 19:50

craig bot merged commit c14ba56 into cockroachdb:master Apr 5, 2023

This was referenced Sep 22, 2023

release-23.1: roachtest: expand tag functionality to allow filtering by conjunctive… #111137

Merged

release-22.2: roachtest: expand tag functionality to allow filtering by conjunctive… #111233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: expand tag functionality to allow filtering by conjunctive… #99402

roachtest: expand tag functionality to allow filtering by conjunctive… #99402

smg260 commented Mar 23, 2023 •

edited

Loading

cockroach-teamcity commented Mar 23, 2023

smg260 commented Mar 23, 2023 •

edited

Loading

smg260 commented Mar 24, 2023 •

edited

Loading

smg260 commented Apr 3, 2023

srosenberg left a comment

srosenberg left a comment

smg260 left a comment

herkolategan left a comment

renatolabs Apr 4, 2023

renatolabs Apr 4, 2023

msbutler Apr 5, 2023

smg260 left a comment

smg260 commented Apr 5, 2023

craig bot commented Apr 5, 2023

roachtest: expand tag functionality to allow filtering by conjunctive… #99402

roachtest: expand tag functionality to allow filtering by conjunctive… #99402

Conversation

smg260 commented Mar 23, 2023 • edited Loading

cockroach-teamcity commented Mar 23, 2023

smg260 commented Mar 23, 2023 • edited Loading

smg260 commented Mar 24, 2023 • edited Loading

smg260 commented Apr 3, 2023

srosenberg left a comment

Choose a reason for hiding this comment

srosenberg left a comment

Choose a reason for hiding this comment

smg260 left a comment

Choose a reason for hiding this comment

herkolategan left a comment

Choose a reason for hiding this comment

renatolabs Apr 4, 2023

Choose a reason for hiding this comment

renatolabs Apr 4, 2023

Choose a reason for hiding this comment

msbutler Apr 5, 2023

Choose a reason for hiding this comment

smg260 left a comment

Choose a reason for hiding this comment

smg260 commented Apr 5, 2023

craig bot commented Apr 5, 2023

smg260 commented Mar 23, 2023 •

edited

Loading

smg260 commented Mar 23, 2023 •

edited

Loading

smg260 commented Mar 24, 2023 •

edited

Loading