Merge #75087 #75279

75087: bazci,ci,dev: update how `stress` is invoked r=rail a=rickystewart

We give both `bazci` and `dev` a subcommand to merge `test.xml` files,
and update `dev` and the various `stress` invocations in CI accordingly
to make use of the `-shardable-artifacts` feature.

Closes #74325.

Release note: None

75279: roachprod: do not mutate EBSVolumes r=jlinder a=rail

Previously, `roachprod create` appended volumes to
`providerOpts.EBSVolumes`, assuming it's done once per run. In reality,
the code is run every time when we create an instance, which leads to data
races and accumulation of extra EBS volumes.

This patch creates a copy of `providerOpts.EBSVolumes` before making any
changes to it.

Release note: None

Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Rail Aliiev <[email protected]>

build/teamcity/cockroach/nightlies/stress_impl.sh

@@ -38,7 +38,7 @@ do
                                           --test_env=COCKROACH_NIGHTLY_STRESS=true \
                                           --test_env=GO_TEST_JSON_OUTPUT_FILE=$GO_TEST_JSON_OUTPUT_FILE \
                                           --test_timeout="$TESTTIMEOUTSECS" \
-                                          --run_under "@com_github_cockroachdb_stress//:stress $STRESSFLAGS" \
+                                          --run_under "@com_github_cockroachdb_stress//:stress -bazel -shardable-artifacts 'GO_TEST_JSON_OUTPUT_FILE=cat,XML_OUTPUT_FILE=$BAZEL_BIN/pkg/cmd/bazci/bazci_/bazci merge-test-xmls' $STRESSFLAGS" \
                                           --define "gotags=$TAGS" \
                                           --nocache_test_results \
                                           ${EXTRA_BAZEL_FLAGS} \

pkg/cmd/bazci/bazci.go

@@ -11,6 +11,7 @@ package main
 
 import (
 	"bytes"
+	"encoding/xml"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -24,9 +25,10 @@ import (
 )
 
 const (
-	buildSubcmd        = "build"
-	testSubcmd         = "test"
-	mungeTestXMLSubcmd = "munge-test-xml"
+	buildSubcmd         = "build"
+	testSubcmd          = "test"
+	mergeTestXMLsSubcmd = "merge-test-xmls"
+	mungeTestXMLSubcmd  = "munge-test-xml"
 )
 
 var (
@@ -98,8 +100,8 @@ func parseArgs(args []string, argsLenAtDash int) (*parsedArgs, error) {
 	if len(args) < 2 {
 		return nil, errUsage
 	}
-	if args[0] != buildSubcmd && args[0] != testSubcmd && args[0] != mungeTestXMLSubcmd {
-		return nil, errors.Newf("First argument must be `build`, `test`, or `munge-test-xml`; got %v", args[0])
+	if args[0] != buildSubcmd && args[0] != testSubcmd && args[0] != mungeTestXMLSubcmd && args[0] != mergeTestXMLsSubcmd {
+		return nil, errors.Newf("First argument must be `build`, `test`, `merge-test-xmls`, or `munge-test-xml`; got %v", args[0])
 	}
 	var splitLoc int
 	if argsLenAtDash < 0 {
@@ -213,11 +215,14 @@ func bazciImpl(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	// Special case: munge-test-xml doesn't require running Bazel at all.
+	// Special case: munge-test-xml/merge-test-xmls don't require running Bazel at all.
 	// Perform the munge then exit immediately.
 	if parsedArgs.subcmd == mungeTestXMLSubcmd {
 		return mungeTestXMLs(*parsedArgs)
 	}
+	if parsedArgs.subcmd == mergeTestXMLsSubcmd {
+		return mergeTestXMLs(*parsedArgs)
+	}
 
 	info, err := getBuildInfo(*parsedArgs)
 	if err != nil {
@@ -267,6 +272,23 @@ func mungeTestXMLs(args parsedArgs) error {
 	return nil
 }
 
+func mergeTestXMLs(args parsedArgs) error {
+	var xmlsToMerge []bazelutil.TestSuites
+	for _, file := range args.targets {
+		contents, err := ioutil.ReadFile(file)
+		if err != nil {
+			return err
+		}
+		var testSuites bazelutil.TestSuites
+		err = xml.Unmarshal(contents, &testSuites)
+		if err != nil {
+			return err
+		}
+		xmlsToMerge = append(xmlsToMerge, testSuites)
+	}
+	return bazelutil.MergeTestXMLs(xmlsToMerge, os.Stdout)
+}
+
 func configArgList() []string {
 	ret := []string{}
 	for _, config := range configs {

pkg/cmd/dev/BUILD.bazel

@@ -14,6 +14,7 @@ go_library(
         "lint.go",
         "logic.go",
         "main.go",
+        "merge_test_xmls.go",
         "test.go",
         "util.go",
     ],

pkg/cmd/dev/dev.go

@@ -115,6 +115,7 @@ Typical usage:
 		makeDoctorCmd(ret.doctor),
 		makeGenerateCmd(ret.generate),
 		makeGoCmd(ret.gocmd),
+		makeMergeTestXMLsCmd(ret.mergeTestXMLs),
 		makeTestLogicCmd(ret.testlogic),
 		makeLintCmd(ret.lint),
 		makeTestCmd(ret.test),

pkg/cmd/dev/merge_test_xmls.go

@@ -0,0 +1,49 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package main
+
+import (
+	"encoding/xml"
+	"io/ioutil"
+	"os"
+
+	bazelutil "github.com/cockroachdb/cockroach/pkg/build/util"
+	"github.com/spf13/cobra"
+)
+
+func makeMergeTestXMLsCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.Command {
+	mergeTestXMLsCommand := &cobra.Command{
+		Use:   "merge-test-xmls XML1 [XML2...]",
+		Short: "Merge the given test XML's (utility command)",
+		Long:  "Merge the given test XML's (utility command)",
+		Args:  cobra.MinimumNArgs(1),
+		RunE:  runE,
+	}
+	mergeTestXMLsCommand.Hidden = true
+	return mergeTestXMLsCommand
+}
+
+func (d *dev) mergeTestXMLs(cmd *cobra.Command, xmls []string) error {
+	var suites []bazelutil.TestSuites
+	for _, file := range xmls {
+		suitesToAdd := bazelutil.TestSuites{}
+		input, err := ioutil.ReadFile(file)
+		if err != nil {
+			return err
+		}
+		err = xml.Unmarshal(input, &suitesToAdd)
+		if err != nil {
+			return err
+		}
+		suites = append(suites, suitesToAdd)
+	}
+	return bazelutil.MergeTestXMLs(suites, os.Stdout)
+}

pkg/cmd/dev/test.go

@@ -210,7 +210,10 @@ func (d *dev) test(cmd *cobra.Command, commandLine []string) error {
 		}
 		stressCmdArgs = append(stressCmdArgs, stressArgs)
 		args = append(args, "--run_under",
-			fmt.Sprintf("%s %s", stressTarget, strings.Join(stressCmdArgs, " ")))
+			// NB: Run with -bazel, which propagates `TEST_TMPDIR` to `TMPDIR`,
+			// and -shardable-artifacts set such that we can merge the XML output
+			// files.
+			fmt.Sprintf("%s -bazel -shardable-artifacts 'XML_OUTPUT_FILE=%s merge-test-xmls' %s", stressTarget, getDevBin(), strings.Join(stressCmdArgs, " ")))
 	}
 
 	if filter != "" {

pkg/cmd/dev/testdata/recording/test.txt

@@ -112,7 +112,7 @@ bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
 
-bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=86400 --run_under '@com_github_cockroachdb_stress//:stress ' '--test_filter=TestStartChild*' --test_output streamed
+bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=86400 --run_under '@com_github_cockroachdb_stress//:stress -bazel -shardable-artifacts '"'"'XML_OUTPUT_FILE=dev merge-test-xmls'"'"' ' '--test_filter=TestStartChild*' --test_output streamed
 ----
 ----
 //pkg/util/tracing:tracing_test                                          PASSED in 12.3s
@@ -129,7 +129,7 @@ bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
 
-bazel test --local_cpu_resources=12 --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=86400 --run_under '@com_github_cockroachdb_stress//:stress -p=12 ' '--test_filter=TestStartChild*' --test_output streamed
+bazel test --local_cpu_resources=12 --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=86400 --run_under '@com_github_cockroachdb_stress//:stress -bazel -shardable-artifacts '"'"'XML_OUTPUT_FILE=dev merge-test-xmls'"'"' -p=12 ' '--test_filter=TestStartChild*' --test_output streamed
 ----
 ----
 //pkg/util/tracing:tracing_test                                          PASSED in 12.3s
@@ -146,7 +146,7 @@ bazel query 'kind(go_test, //pkg/util/tracing:all)'
 ----
 //pkg/util/tracing:tracing_test
 
-bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=70 --run_under '@com_github_cockroachdb_stress//:stress -maxtime=10s ' '--test_filter=TestStartChild*' --test_arg -test.v --test_output streamed
+bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=70 --run_under '@com_github_cockroachdb_stress//:stress -bazel -shardable-artifacts '"'"'XML_OUTPUT_FILE=dev merge-test-xmls'"'"' -maxtime=10s ' '--test_filter=TestStartChild*' --test_arg -test.v --test_output streamed
 ----
 ----
 ==================== Test output for //pkg/util/tracing:tracing_test:

pkg/cmd/dev/testdata/test.txt

@@ -38,19 +38,19 @@ dev test --stress pkg/util/tracing --filter TestStartChild*
 ----
 find pkg/util/tracing -type d
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
-bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=86400 --run_under '@com_github_cockroachdb_stress//:stress ' '--test_filter=TestStartChild*' --test_output streamed
+bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=86400 --run_under '@com_github_cockroachdb_stress//:stress -bazel -shardable-artifacts '"'"'XML_OUTPUT_FILE=dev merge-test-xmls'"'"' ' '--test_filter=TestStartChild*' --test_output streamed
 
 dev test --stress pkg/util/tracing --filter TestStartChild* --cpus=12
 ----
 find pkg/util/tracing -type d
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
-bazel test --local_cpu_resources=12 --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=86400 --run_under '@com_github_cockroachdb_stress//:stress -p=12 ' '--test_filter=TestStartChild*' --test_output streamed
+bazel test --local_cpu_resources=12 --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=86400 --run_under '@com_github_cockroachdb_stress//:stress -bazel -shardable-artifacts '"'"'XML_OUTPUT_FILE=dev merge-test-xmls'"'"' -p=12 ' '--test_filter=TestStartChild*' --test_output streamed
 
 dev test --stress pkg/util/tracing --filter TestStartChild* --timeout=10s -v
 ----
 find pkg/util/tracing -type d
 bazel query 'kind(go_test, //pkg/util/tracing:all)'
-bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=70 --run_under '@com_github_cockroachdb_stress//:stress -maxtime=10s ' '--test_filter=TestStartChild*' --test_arg -test.v --test_output streamed
+bazel test --test_sharding_strategy=disabled //pkg/util/tracing:tracing_test --test_env=GOTRACEBACK=all --test_timeout=70 --run_under '@com_github_cockroachdb_stress//:stress -bazel -shardable-artifacts '"'"'XML_OUTPUT_FILE=dev merge-test-xmls'"'"' -maxtime=10s ' '--test_filter=TestStartChild*' --test_arg -test.v --test_output streamed
 
 dev test //pkg/testutils --timeout=10s
 ----

pkg/cmd/dev/util.go

@@ -276,3 +276,11 @@ func logCommand(cmd string, args ...string) {
 	fullArgs = append(fullArgs, args...)
 	log.Printf("$ %s", shellescape.QuoteCommand(fullArgs))
 }
+
+// getDevBin returns the path to the running dev executable.
+func getDevBin() string {
+	if isTesting {
+		return "dev"
+	}
+	return os.Args[0]
+}

pkg/cmd/github-pull-request-make/main.go

@@ -223,6 +223,15 @@ func main() {
 	if forceBazelStr, ok := os.LookupEnv(forceBazelEnv); ok {
 		forceBazel, _ = strconv.ParseBool(forceBazelStr)
 	}
+	var bazciPath string
+	if bazel.BuiltWithBazel() || forceBazel {
+		// NB: bazci is expected to be put in `PATH` by the caller.
+		var err error
+		bazciPath, err = exec.LookPath("bazci")
+		if err != nil {
+			log.Fatal(err)
+		}
+	}
 
 	crdb, err := os.Getwd()
 	if err != nil {
@@ -308,8 +317,8 @@ func main() {
 				args = append(args, "--test_arg=-test.timeout", fmt.Sprintf("--test_arg=%s", timeout))
 				// Give the entire test 1 more minute than the duration to wrap up.
 				args = append(args, fmt.Sprintf("--test_timeout=%d", int((duration+1*time.Minute).Seconds())))
-				args = append(args, "--run_under", fmt.Sprintf("%s -stderr -maxfails 1 -maxtime %s -p %d", bazelStressTarget, duration, parallelism))
-				// NB: bazci is expected to be put in `PATH` by the caller.
+
+				args = append(args, "--run_under", fmt.Sprintf("%s -bazel -shardable-artifacts 'XML_OUTPUT_FILE=%s merge-test-xmls' -stderr -maxfails 1 -maxtime %s -p %d", bazelStressTarget, bazciPath, duration, parallelism))
 				cmd := exec.Command("bazci", args...)
 				cmd.Stdout = os.Stdout
 				cmd.Stderr = os.Stderr

pkg/roachprod/vm/aws/aws.go

@@ -921,20 +921,21 @@ func (p *Provider) runInstance(
 	if p.IAMProfile != "" {
 		args = append(args, "--iam-instance-profile", "Name="+p.IAMProfile)
 	}
-
+	// Make a local copy of providerOpts.EBSVolumes to prevent data races
+	ebsVolumes := providerOpts.EBSVolumes
 	// The local NVMe devices are automatically mapped.  Otherwise, we need to map an EBS data volume.
 	if !opts.SSDOpts.UseLocalSSD {
-		if len(providerOpts.EBSVolumes) == 0 && providerOpts.DefaultEBSVolume.Disk.VolumeType == "" {
+		if len(ebsVolumes) == 0 && providerOpts.DefaultEBSVolume.Disk.VolumeType == "" {
 			providerOpts.DefaultEBSVolume.Disk.VolumeType = defaultEBSVolumeType
 			providerOpts.DefaultEBSVolume.Disk.DeleteOnTermination = true
 		}
 
 		if providerOpts.DefaultEBSVolume.Disk.VolumeType != "" {
 			// Add default volume to the list of volumes we'll setup.
-			v := providerOpts.EBSVolumes.newVolume()
+			v := ebsVolumes.newVolume()
 			v.Disk = providerOpts.DefaultEBSVolume.Disk
 			v.Disk.DeleteOnTermination = true
-			providerOpts.EBSVolumes = append(providerOpts.EBSVolumes, v)
+			ebsVolumes = append(ebsVolumes, v)
 		}
 	}
 
@@ -946,9 +947,9 @@ func (p *Provider) runInstance(
 			DeleteOnTermination: true,
 		},
 	}
-	providerOpts.EBSVolumes = append(providerOpts.EBSVolumes, osDiskVolume)
+	ebsVolumes = append(ebsVolumes, osDiskVolume)
 
-	mapping, err := json.Marshal(providerOpts.EBSVolumes)
+	mapping, err := json.Marshal(ebsVolumes)
 	if err != nil {
 		return err
 	}