roachtest: introduce registry.ErrorWithOwner

This function allows the caller to assign ownership directly when
creating an error, which is very useful when we know that a failure
during a certain part of the test should always be investigated by a
certain team (for instance, if a test fails during setup, Test Eng
should investigate, as the owners of the test infrastructure).

Epic: none

Release note: None

Author: renatolabs

pkg/cmd/roachtest/github.go

@@ -27,6 +27,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm/gce"
+	"github.com/cockroachdb/errors"
 )
 
 type githubIssues struct {
@@ -147,37 +148,47 @@ func (g *githubIssues) createPostRequest(
 	var mention []string
 	var projColID int
 
-	issueOwner := spec.Owner
-	issueName := testName
-	issueClusterName := ""
+	var (
+		issueOwner    = spec.Owner
+		issueName     = testName
+		messagePrefix string
+		infraFlake    bool
+	)
+
+	// handleErrorWithOwnership updates the local variables in this
+	// function that contain the name of the issue being created,
+	// message prefix, and team that will own it.
+	handleErrorWithOwnership := func(err registry.ErrorWithOwnership) {
+		issueOwner = err.Owner
+		infraFlake = err.InfraFlake
+
+		if err.TitleOverride != "" {
+			issueName = err.TitleOverride
+			messagePrefix = fmt.Sprintf("test %s failed: ", testName)
+		}
+	}
 
-	messagePrefix := ""
-	var infraFlake bool
-	firstFailure := failures[0]
-	// Overrides to shield eng teams from potential flakes
+	issueClusterName := ""
+	errWithOwnership := failuresSpecifyOwner(failures)
 	switch {
-	case failuresContainsError(failures, errVMPreemption):
-		issueOwner = registry.OwnerTestEng
-		issueName = "vm_preemption"
-		messagePrefix = fmt.Sprintf("test %s failed due to ", testName)
-		infraFlake = true
-	case failureContainsError(firstFailure, errClusterProvisioningFailed):
-		issueOwner = registry.OwnerTestEng
-		issueName = "cluster_creation"
-		messagePrefix = fmt.Sprintf("test %s was skipped due to ", testName)
-		infraFlake = true
-	case failureContainsError(firstFailure, rperrors.ErrSSH255):
-		issueOwner = registry.OwnerTestEng
-		issueName = "ssh_problem"
-		messagePrefix = fmt.Sprintf("test %s failed due to ", testName)
-		infraFlake = true
-	case failureContainsError(firstFailure, gce.ErrDNSOperation):
-		issueOwner = registry.OwnerTestEng
-		issueName = "dns_problem"
-		messagePrefix = fmt.Sprintf("test %s failed due to ", testName)
-		infraFlake = true
-	case failureContainsError(firstFailure, errDuringPostAssertions):
-		messagePrefix = fmt.Sprintf("test %s failed during post test assertions (see test-post-assertions.log) due to ", testName)
+	case errWithOwnership != nil:
+		handleErrorWithOwnership(*errWithOwnership)
+
+	// The following error come from various entrypoints in roachprod,
+	// but we know that they should be handled by TestEng whenever they
+	// happen during a test.
+	case failuresContainsError(failures, rperrors.ErrSSH255):
+		handleErrorWithOwnership(registry.ErrorWithOwner(
+			registry.OwnerTestEng, errors.New("SSH problem"),
+			registry.WithTitleOverride("ssh_problem"),
+			registry.InfraFlake,
+		))
+	case failuresContainsError(failures, gce.ErrDNSOperation):
+		handleErrorWithOwnership(registry.ErrorWithOwner(
+			registry.OwnerTestEng, errors.New("DNS problem"),
+			registry.WithTitleOverride("dns_problem"),
+			registry.InfraFlake,
+		))
 	}
 
 	// Issues posted from roachtest are identifiable as such, and they are also release blockers

pkg/cmd/roachtest/github_test.go

@@ -30,7 +30,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/vm/gce"
 	"github.com/cockroachdb/cockroach/pkg/testutils/echotest"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -128,6 +127,8 @@ func TestCreatePostRequest(t *testing.T) {
 		return failure{squashedErr: ref}
 	}
 
+	const testName = "github_test"
+
 	// TODO(radu): these tests should be converted to datadriven tests which
 	// output the full rendering of the github issue message along with the
 	// metadata.
@@ -143,6 +144,9 @@ func TestCreatePostRequest(t *testing.T) {
 		failures                []failure
 		expectedPost            bool
 		expectedLabels          []string
+		expectedTeam            string
+		expectedName            string
+		expectedMessagePrefix   string
 		expectedReleaseBlocker  bool
 		expectedSkipTestFailure bool
 		expectedParams          map[string]string
@@ -154,6 +158,8 @@ func TestCreatePostRequest(t *testing.T) {
 			failures:          []failure{createFailure(errors.New("other"))},
 			expectedPost:      true,
 			expectedLabels:    []string{"C-test-failure"},
+			expectedTeam:      "@cockroachdb/unowned",
+			expectedName:      testName,
 			expectedParams: prefixAll(map[string]string{
 				"cloud":            "gce",
 				"encrypted":        "false",
@@ -171,9 +177,14 @@ func TestCreatePostRequest(t *testing.T) {
 			localSSD:         true,
 			metamorphicBuild: true,
 			arch:             vm.ArchARM64,
-			failures:         []failure{createFailure(errClusterProvisioningFailed)},
-			expectedPost:     true,
-			expectedLabels:   []string{"T-testeng", "X-infra-flake"},
+			failures: []failure{
+				createFailure(errClusterProvisioningFailed(errors.New("gcloud error"))),
+			},
+			expectedPost:          true,
+			expectedLabels:        []string{"T-testeng", "X-infra-flake"},
+			expectedTeam:          "@cockroachdb/test-eng",
+			expectedName:          "cluster_creation",
+			expectedMessagePrefix: testName + " failed",
 			expectedParams: prefixAll(map[string]string{
 				"cloud":            "gce",
 				"encrypted":        "false",
@@ -195,6 +206,9 @@ func TestCreatePostRequest(t *testing.T) {
 			failures:              []failure{createFailure(rperrors.ErrSSH255)},
 			expectedPost:          true,
 			expectedLabels:        []string{"T-testeng", "X-infra-flake"},
+			expectedTeam:          "@cockroachdb/test-eng",
+			expectedName:          "ssh_problem",
+			expectedMessagePrefix: testName + " failed",
 			expectedParams: prefixAll(map[string]string{
 				"cloud":            "gce",
 				"ssd":              "0",
@@ -210,25 +224,24 @@ func TestCreatePostRequest(t *testing.T) {
 			failures:          []failure{createFailure(errors.New("other"))},
 			expectedLabels:    []string{"C-test-failure"},
 		},
-		// 5. Error during post test assertions
-		{
-			nonReleaseBlocker: true,
-			failures:          []failure{createFailure(errDuringPostAssertions)},
-			expectedLabels:    []string{"C-test-failure"},
-		},
-		// 6. Error during dns operation.
+		// 5. Error during dns operation.
 		{
-			nonReleaseBlocker: true,
-			failures:          []failure{createFailure(gce.ErrDNSOperation)},
-			expectedPost:      true,
-			expectedLabels:    []string{"T-testeng", "X-infra-flake"},
+			nonReleaseBlocker:     true,
+			failures:              []failure{createFailure(gce.ErrDNSOperation)},
+			expectedPost:          true,
+			expectedLabels:        []string{"T-testeng", "X-infra-flake"},
+			expectedTeam:          "@cockroachdb/test-eng",
+			expectedName:          "dns_problem",
+			expectedMessagePrefix: testName + " failed",
 		},
-		// 7. Assert that extra labels in the test spec are added to the issue.
+		// 6. Assert that extra labels in the test spec are added to the issue.
 		{
 			extraLabels:    []string{"foo-label"},
 			failures:       []failure{createFailure(errors.New("other"))},
 			expectedPost:   true,
 			expectedLabels: []string{"C-test-failure", "release-blocker", "foo-label"},
+			expectedTeam:   "@cockroachdb/unowned",
+			expectedName:   testName,
 			expectedParams: prefixAll(map[string]string{
 				"cloud":            "gce",
 				"encrypted":        "false",
@@ -241,13 +254,15 @@ func TestCreatePostRequest(t *testing.T) {
 				"coverageBuild":    "false",
 			}),
 		},
-		// 8. Verify that release-blocker label is not applied on metamorphic builds
+		// 7. Verify that release-blocker label is not applied on metamorphic builds
 		// (for now).
 		{
 			metamorphicBuild: true,
 			failures:         []failure{createFailure(errors.New("other"))},
 			expectedPost:     true,
 			expectedLabels:   []string{"C-test-failure", "B-metamorphic-enabled"},
+			expectedTeam:     "@cockroachdb/unowned",
+			expectedName:     testName,
 			expectedParams: prefixAll(map[string]string{
 				"cloud":            "gce",
 				"encrypted":        "false",
@@ -260,13 +275,15 @@ func TestCreatePostRequest(t *testing.T) {
 				"coverageBuild":    "false",
 			}),
 		},
-		// 9. Verify that release-blocker label is not applied on coverage builds (for
+		// 8. Verify that release-blocker label is not applied on coverage builds (for
 		// now).
 		{
 			extraLabels:    []string{"foo-label"},
 			coverageBuild:  true,
 			failures:       []failure{createFailure(errors.New("other"))},
 			expectedPost:   true,
+			expectedTeam:   "@cockroachdb/unowned",
+			expectedName:   testName,
 			expectedLabels: []string{"C-test-failure", "B-coverage-enabled", "foo-label"},
 			expectedParams: prefixAll(map[string]string{
 				"cloud":            "gce",
@@ -280,29 +297,40 @@ func TestCreatePostRequest(t *testing.T) {
 				"coverageBuild":    "true",
 			}),
 		},
-		// 10. Verify preemption failure are routed to test-eng and marked as infra-flake,
+		// 9. Verify preemption failure are routed to test-eng and marked as infra-flake,
 		// even if the first failure is another handled error.
 		{
-			nonReleaseBlocker: true,
-			failures:          []failure{createFailure(gce.ErrDNSOperation), createFailure(errVMPreemption)},
-			expectedPost:      true,
-			expectedLabels:    []string{"T-testeng", "X-infra-flake"},
+			nonReleaseBlocker:     true,
+			failures:              []failure{createFailure(gce.ErrDNSOperation), createFailure(vmPreemptionError("my_VM"))},
+			expectedPost:          true,
+			expectedName:          "vm_preemption",
+			expectedTeam:          "@cockroachdb/test-eng",
+			expectedMessagePrefix: testName + " failed",
+			expectedLabels:        []string{"T-testeng", "X-infra-flake"},
 		},
-		// 11. Verify preemption failure are routed to test-eng and marked as infra-flake, when the
+		// 10. Verify preemption failure are routed to test-eng and marked as infra-flake, when the
 		// first failure is a non-handled error.
 		{
-			nonReleaseBlocker: true,
-			failures:          []failure{createFailure(errors.New("random")), createFailure(errVMPreemption)},
-			expectedPost:      true,
-			expectedLabels:    []string{"T-testeng", "X-infra-flake"},
+			nonReleaseBlocker:     true,
+			failures:              []failure{createFailure(errors.New("random")), createFailure(vmPreemptionError("my_VM"))},
+			expectedPost:          true,
+			expectedTeam:          "@cockroachdb/test-eng",
+			expectedName:          "vm_preemption",
+			expectedMessagePrefix: testName + " failed",
+			expectedLabels:        []string{"T-testeng", "X-infra-flake"},
 		},
-		// 12. Verify preemption failure are routed to test-eng and marked as infra-flake, when the only error is
+		// 11. Verify preemption failure are routed to test-eng and marked as infra-flake, when the only error is
 		// preemption failure
 		{
 			nonReleaseBlocker: true,
-			failures:          []failure{{errors: []error{errVMPreemption}}},
-			expectedPost:      true,
-			expectedLabels:    []string{"T-testeng", "X-infra-flake"},
+			failures: []failure{
+				{errors: []error{vmPreemptionError("my_VM")}},
+			},
+			expectedPost:          true,
+			expectedTeam:          "@cockroachdb/test-eng",
+			expectedName:          "vm_preemption",
+			expectedMessagePrefix: testName + " failed",
+			expectedLabels:        []string{"T-testeng", "X-infra-flake"},
 		},
 	}
 
@@ -312,7 +340,7 @@ func TestCreatePostRequest(t *testing.T) {
 			clusterSpec := reg.MakeClusterSpec(1, spec.Arch(testCase.arch))
 
 			testSpec := &registry.TestSpec{
-				Name:              "github_test",
+				Name:              testName,
 				Owner:             OwnerUnitTest,
 				Cluster:           clusterSpec,
 				NonReleaseBlocker: testCase.nonReleaseBlocker,
@@ -350,57 +378,35 @@ func TestCreatePostRequest(t *testing.T) {
 				teamLoader:   teamLoadFn,
 			}
 
+			req, err := github.createPostRequest(
+				testName, ti.start, ti.end, testSpec, testCase.failures,
+				testCase.message, testCase.metamorphicBuild, testCase.coverageBuild,
+			)
 			if testCase.loadTeamsFailed {
 				// Assert that if TEAMS.yaml cannot be loaded then function errors.
-				_, err := github.createPostRequest("github_test", ti.start, ti.end, testSpec, testCase.failures, testCase.message, testCase.metamorphicBuild, testCase.coverageBuild)
-				assert.Error(t, err, "Expected an error in createPostRequest when loading teams fails, but got nil")
-			} else {
-				req, err := github.createPostRequest("github_test", ti.start, ti.end, testSpec, testCase.failures, testCase.message, testCase.metamorphicBuild, testCase.coverageBuild)
-				assert.NoError(t, err, "Expected no error in createPostRequest")
-
-				r := &issues.Renderer{}
-				req.HelpCommand(r)
-				file := fmt.Sprintf("help_command_createpost_%d.txt", idx+1)
-				echotest.Require(t, r.String(), filepath.Join("testdata", file))
-
-				if testCase.expectedParams != nil {
-					require.Equal(t, testCase.expectedParams, req.ExtraParams)
-				}
-
-				expLabels := append([]string{"O-roachtest"}, testCase.expectedLabels...)
-				sort.Strings(expLabels)
-				labels := append([]string{}, req.Labels...)
-				sort.Strings(expLabels)
-				sort.Strings(labels)
-				require.Equal(t, expLabels, labels)
-
-				expectedTeam := "@cockroachdb/unowned"
-				expectedName := "github_test"
-				expectedMessagePrefix := ""
-				if failuresContainsError(testCase.failures, errVMPreemption) {
-					expectedTeam = "@cockroachdb/test-eng"
-					expectedName = "vm_preemption"
-					expectedMessagePrefix = "test github_test failed due to "
-				} else if errors.Is(testCase.failures[0].squashedErr, gce.ErrDNSOperation) {
-					expectedTeam = "@cockroachdb/test-eng"
-					expectedName = "dns_problem"
-					expectedMessagePrefix = "test github_test failed due to "
-				} else if errors.Is(testCase.failures[0].squashedErr, errClusterProvisioningFailed) {
-					expectedTeam = "@cockroachdb/test-eng"
-					expectedName = "cluster_creation"
-					expectedMessagePrefix = "test github_test was skipped due to "
-				} else if errors.Is(testCase.failures[0].squashedErr, rperrors.ErrSSH255) {
-					expectedTeam = "@cockroachdb/test-eng"
-					expectedName = "ssh_problem"
-					expectedMessagePrefix = "test github_test failed due to "
-				} else if errors.Is(testCase.failures[0].squashedErr, errDuringPostAssertions) {
-					expectedMessagePrefix = "test github_test failed during post test assertions (see test-post-assertions.log) due to "
-				}
-
-				require.Contains(t, req.MentionOnCreate, expectedTeam)
-				require.Equal(t, expectedName, req.TestName)
-				require.True(t, strings.HasPrefix(req.Message, expectedMessagePrefix), req.Message)
+				require.Error(t, err)
+				return
 			}
+
+			require.NoError(t, err)
+
+			r := &issues.Renderer{}
+			req.HelpCommand(r)
+			file := fmt.Sprintf("help_command_createpost_%d.txt", idx+1)
+			echotest.Require(t, r.String(), filepath.Join("testdata", file))
+
+			if testCase.expectedParams != nil {
+				require.Equal(t, testCase.expectedParams, req.ExtraParams)
+			}
+
+			expLabels := append([]string{"O-roachtest"}, testCase.expectedLabels...)
+			sort.Strings(expLabels)
+			sort.Strings(req.Labels)
+			require.Equal(t, expLabels, req.Labels)
+
+			require.Contains(t, req.MentionOnCreate, testCase.expectedTeam)
+			require.Equal(t, testCase.expectedName, req.TestName)
+			require.Contains(t, req.Message, testCase.expectedMessagePrefix)
 		})
 	}
 }

pkg/cmd/roachtest/registry/BUILD.bazel

@@ -4,6 +4,7 @@ go_library(
     name = "registry",
     srcs = [
         "encryption.go",
+        "errors.go",
         "filter.go",
         "owners.go",
         "registry_interface.go",