From 0d335de59a4b8f846ce39b6ed893a6b815410161 Mon Sep 17 00:00:00 2001 From: Tobias Grieger Date: Wed, 2 Nov 2022 05:59:46 +0100 Subject: [PATCH] wip --- pkg/cmd/roachtest/github.go | 61 +++++++++++-------- .../roachtestutil/consistency_check.go | 10 ++- 2 files changed, 45 insertions(+), 26 deletions(-) diff --git a/pkg/cmd/roachtest/github.go b/pkg/cmd/roachtest/github.go index e9d019900f0a..b2719fd68491 100644 --- a/pkg/cmd/roachtest/github.go +++ b/pkg/cmd/roachtest/github.go @@ -17,6 +17,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/cmd/internal/issues" "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry" + "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil" "github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test" "github.com/cockroachdb/cockroach/pkg/internal/team" rperrors "github.com/cockroachdb/cockroach/pkg/roachprod/errors" @@ -33,14 +34,6 @@ type githubIssues struct { teamLoader func() (team.Map, error) } -type issueCategory int - -const ( - otherErr issueCategory = iota - clusterCreationErr - sshErr -) - func newGithubIssues( disable bool, c *clusterImpl, vmCreateOpts *vm.CreateOpts, l *logger.Logger, ) *githubIssues { @@ -68,26 +61,27 @@ func (g *githubIssues) shouldPost(t test.Test) bool { t.Spec().(*registry.TestSpec).Cluster.NodeCount > 0 } +type issueOverride struct { + owner registry.Owner + testName string + msgPrefix string +} + func (g *githubIssues) createPostRequest( - t test.Test, cat issueCategory, message string, + t test.Test, override issueOverride, message string, ) issues.PostRequest { var mention []string var projColID int issueOwner := t.Spec().(*registry.TestSpec).Owner + if o := override.owner; o != "" { + issueOwner = o + } issueName := t.Name() - - messagePrefix := "" - // Overrides to shield eng teams from potential flakes - if cat == clusterCreationErr { - issueOwner = registry.OwnerDevInf - issueName = "cluster_creation" - messagePrefix = fmt.Sprintf("test %s was skipped due to ", t.Name()) - } else if cat == sshErr { - issueOwner = registry.OwnerTestEng - issueName = "ssh_problem" - messagePrefix = fmt.Sprintf("test %s failed due to ", t.Name()) + if n := override.testName; n != "" { + issueName = n } + messagePrefix := override.msgPrefix teams, err := g.teamLoader() if err != nil { @@ -161,19 +155,36 @@ func (g *githubIssues) MaybePost(t *testImpl, message string) error { return nil } - cat := otherErr - + var o issueOverride // Overrides to shield eng teams from potential flakes firstFailure := t.firstFailure() if failureContainsError(firstFailure, errClusterProvisioningFailed) { - cat = clusterCreationErr + o.owner = registry.OwnerDevInf + o.testName = "cluster_creation" + o.msgPrefix = fmt.Sprintf("test %s was skipped due to ", t.Name()) } else if failureContainsError(firstFailure, rperrors.ErrSSH255) { - cat = sshErr + o.owner = registry.OwnerTestEng + o.testName = "ssh_problem" + o.msgPrefix = fmt.Sprintf("test %s failed due to ", t.Name()) + } else if failureContainsError(firstFailure, roachtestutil.ErrMarkConsistencyCheckFailed) { + o.owner = registry.OwnerKV + o.testName = "consistency_check" + o.msgPrefix = fmt.Sprintf("consistency check failed after running %s", t.Name()) + } else { + for _, err := range firstFailure.errors { + owner, ok := registry.OwnerFromErr(err) + if !ok { + continue + } + o.owner = owner + o.msgPrefix = "owner overridden by error" + break + } } return g.issuePoster( context.Background(), issues.UnitTestFormatter, - g.createPostRequest(t, cat, message), + g.createPostRequest(t, o, message), ) } diff --git a/pkg/cmd/roachtest/roachtestutil/consistency_check.go b/pkg/cmd/roachtest/roachtestutil/consistency_check.go index 01f4885d7df9..5bfd6f9ff44a 100644 --- a/pkg/cmd/roachtest/roachtestutil/consistency_check.go +++ b/pkg/cmd/roachtest/roachtestutil/consistency_check.go @@ -20,6 +20,10 @@ import ( "github.com/cockroachdb/errors" ) +// ErrMarkConsistencyCheckFailed marks errors that indicate a replica divergence +// problem detected in CheckReplicaDivergenceOnDB. +var ErrMarkConsistencyCheckFailed = errors.New("consistency check failed") + // CheckReplicaDivergenceOnDB runs a stats-only consistency check via the // provided DB. It ignores any errors running the checks and will only propagate // an error resulting from an actual detected discrepancy. In other words, a nil @@ -64,5 +68,9 @@ WHERE t.status NOT IN ('RANGE_CONSISTENT', 'RANGE_INDETERMINATE')`, statsOnly)) l.Printf("consistency check failed with %v; ignoring", err) return nil } - return finalErr + + if finalErr != nil { + return errors.Mark(finalErr, ErrMarkConsistencyCheckFailed) + } + return nil }