From 7a5643d1038bb9f5daa7e72cabf849e92c5f4dbe Mon Sep 17 00:00:00 2001 From: Jakub Guzik Date: Wed, 24 Nov 2021 00:42:01 +0100 Subject: [PATCH] Fix single override creates many ProwJobs in the system This commit addresses the problem described in #22690. Override once issued, can spawn many the almost same prowjobs. The root cause of the problem takes the origin in the statuses returned by GitHub API. There are multiple statuses with the same context which reflects the history of the job. One repetition of context handling for override is enough, rest of the statuses can be ignored. This commit also adjusts unit tests to the real situation, removing maps as overlapping statuses are possible. Signed-off-by: Jakub Guzik --- prow/plugins/override/override.go | 6 +- prow/plugins/override/override_test.go | 315 +++++++++++++++---------- 2 files changed, 188 insertions(+), 133 deletions(-) diff --git a/prow/plugins/override/override.go b/prow/plugins/override/override.go index 4f706231aa5b..202edb77ea93 100644 --- a/prow/plugins/override/override.go +++ b/prow/plugins/override/override.go @@ -393,6 +393,7 @@ Only the following contexts were expected: } done := sets.String{} + contextsWithCreatedJobs := sets.String{} defer func() { if len(done) == 0 { @@ -405,8 +406,7 @@ Only the following contexts were expected: for _, status := range statuses { pre := presubmitForContext(presubmits, status.Context) - - if status.State == github.StatusSuccess || !(overrides.Has(status.Context) || pre != nil && overrides.Has(pre.Name)) { + if status.State == github.StatusSuccess || !(overrides.Has(status.Context) || pre != nil && overrides.Has(pre.Name)) || contextsWithCreatedJobs.Has(status.Context) { continue } @@ -428,12 +428,14 @@ Only the following contexts were expected: Description: description(user), URL: e.HTMLURL, } + log.WithFields(pjutil.ProwJobFields(&pj)).Info("Creating a new prowjob.") if _, err := oc.Create(context.TODO(), &pj, metav1.CreateOptions{}); err != nil { resp := fmt.Sprintf("Failed to create override job for %s", status.Context) log.WithError(err).Warn(resp) return oc.CreateComment(org, repo, number, plugins.FormatResponseRaw(e.Body, e.HTMLURL, user, resp)) } + contextsWithCreatedJobs.Insert(status.Context) } status.State = github.StatusSuccess status.Description = description(user) diff --git a/prow/plugins/override/override_test.go b/prow/plugins/override/override_test.go index 5087464d1626..4cfcb4a6ea56 100644 --- a/prow/plugins/override/override_test.go +++ b/prow/plugins/override/override_test.go @@ -117,9 +117,9 @@ func (foc *fakeOwnersClient) ParseFullConfig(path string) (repoowners.FullConfig type fakeClient struct { comments []string - statuses map[string]github.Status + statuses []github.Status branchProtection *github.BranchProtection - ps map[string]config.Presubmit + ps []config.Presubmit jobs sets.String owners ownersClient } @@ -148,7 +148,16 @@ func (c *fakeClient) CreateStatus(org, repo, ref string, s github.Status) error case ref != fakeSHA: return fmt.Errorf("bad ref: %s", ref) } - c.statuses[s.Context] = s + for i, status := range c.statuses { + if status.State != github.StatusSuccess && status.Context == s.Context { + c.statuses[i] = s + return nil + } + } + //handle branch protection case + if len(c.statuses) == 0 { + c.statuses = append(c.statuses, s) + } return nil } @@ -304,12 +313,12 @@ func TestHandle(t *testing.T) { issue bool state string comment string - contexts map[string]github.Status + contexts []github.Status branchProtection *github.BranchProtection - presubmits map[string]config.Presubmit + presubmits []config.Presubmit user string number int - expected map[string]github.Status + expected []github.Status jobs sets.String checkComments []string options plugins.Override @@ -319,14 +328,14 @@ func TestHandle(t *testing.T) { { name: "successfully override failure", comment: "/override broken-test", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", Description: description(adminUser), State: github.StatusSuccess, @@ -337,14 +346,14 @@ func TestHandle(t *testing.T) { { name: "successfully override pending", comment: "/override hung-test", - contexts: map[string]github.Status{ - "hung-test": { + contexts: []github.Status{ + { Context: "hung-test", State: github.StatusPending, }, }, - expected: map[string]github.Status{ - "hung-test": { + expected: []github.Status{ + { Context: "hung-test", Description: description(adminUser), State: github.StatusSuccess, @@ -354,14 +363,14 @@ func TestHandle(t *testing.T) { { name: "comment for incorrect context", comment: "/override whatever-you-want", - contexts: map[string]github.Status{ - "hung-test": { + contexts: []github.Status{ + { Context: "hung-test", State: github.StatusPending, }, }, - presubmits: map[string]config.Presubmit{ - "hung-test": { + presubmits: []config.Presubmit{ + { JobBase: config.JobBase{ Name: "hung-prow-job", }, @@ -370,8 +379,8 @@ func TestHandle(t *testing.T) { }, }, }, - expected: map[string]github.Status{ - "hung-test": { + expected: []github.Status{ + { Context: "hung-test", State: github.StatusPending, }, @@ -384,16 +393,16 @@ func TestHandle(t *testing.T) { { name: "refuse override from non-admin", comment: "/override broken-test", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, }, user: "rando", checkComments: []string{"unauthorized"}, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, @@ -402,16 +411,16 @@ func TestHandle(t *testing.T) { { name: "comment for override with no target", comment: "/override", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, }, user: "rando", checkComments: []string{"but none was given"}, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, @@ -420,24 +429,24 @@ func TestHandle(t *testing.T) { { name: "override multiple", comment: "/override broken-test\n/override hung-test", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusFailure, }, - "hung-test": { + { Context: "hung-test", State: github.StatusPending, }, }, - expected: map[string]github.Status{ - "hung-test": { - Context: "hung-test", + expected: []github.Status{ + { + Context: "broken-test", Description: description(adminUser), State: github.StatusSuccess, }, - "broken-test": { - Context: "broken-test", + { + Context: "hung-test", Description: description(adminUser), State: github.StatusSuccess, }, @@ -448,14 +457,14 @@ func TestHandle(t *testing.T) { name: "override with extra whitespace", // Note two spaces here to start, and trailing whitespace comment: "/override broken-test \n", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", Description: description(adminUser), State: github.StatusSuccess, @@ -467,14 +476,14 @@ func TestHandle(t *testing.T) { name: "ignore non-PRs", issue: true, comment: "/override broken-test", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, }, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, @@ -484,14 +493,14 @@ func TestHandle(t *testing.T) { name: "ignore closed issues", state: "closed", comment: "/override broken-test", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, }, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, @@ -501,14 +510,14 @@ func TestHandle(t *testing.T) { name: "ignore edits", action: github.GenericCommentActionEdited, comment: "/override broken-test", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, }, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, @@ -517,14 +526,14 @@ func TestHandle(t *testing.T) { { name: "ignore random text", comment: "/test broken-test", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, }, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", State: github.StatusPending, }, @@ -534,14 +543,14 @@ func TestHandle(t *testing.T) { name: "comment on get pr failure", number: fakePR * 2, comment: "/override broken-test", - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", State: github.StatusFailure, }, @@ -551,14 +560,14 @@ func TestHandle(t *testing.T) { { name: "comment on list statuses failure", comment: "/override fail-list", - contexts: map[string]github.Status{ - "fail-list": { + contexts: []github.Status{ + { Context: "fail-list", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "fail-list": { + expected: []github.Status{ + { Context: "fail-list", State: github.StatusFailure, }, @@ -571,14 +580,14 @@ func TestHandle(t *testing.T) { branchProtection: &github.BranchProtection{RequiredStatusChecks: &github.RequiredStatusChecks{ Contexts: []string{"fail-protection"}, }}, - contexts: map[string]github.Status{ - "broken-test": { + contexts: []github.Status{ + { Context: "broken-test", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "broken-test": { + expected: []github.Status{ + { Context: "broken-test", State: github.StatusFailure, }, @@ -588,15 +597,15 @@ func TestHandle(t *testing.T) { { name: "do not override passing contexts", comment: "/override passing-test", - contexts: map[string]github.Status{ - "passing-test": { + contexts: []github.Status{ + { Context: "passing-test", Description: "preserve description", State: github.StatusSuccess, }, }, - expected: map[string]github.Status{ - "passing-test": { + expected: []github.Status{ + { Context: "passing-test", State: github.StatusSuccess, Description: "preserve description", @@ -606,15 +615,15 @@ func TestHandle(t *testing.T) { { name: "create successful prow job", comment: "/override prow-job", - contexts: map[string]github.Status{ - "prow-job": { + contexts: []github.Status{ + { Context: "prow-job", Description: "failed", State: github.StatusFailure, }, }, - presubmits: map[string]config.Presubmit{ - "prow-job": { + presubmits: []config.Presubmit{ + { JobBase: config.JobBase{ Name: "prow-job", }, @@ -624,8 +633,8 @@ func TestHandle(t *testing.T) { }, }, jobs: sets.NewString("prow-job"), - expected: map[string]github.Status{ - "prow-job": { + expected: []github.Status{ + { Context: "prow-job", State: github.StatusSuccess, Description: description(adminUser), @@ -635,15 +644,15 @@ func TestHandle(t *testing.T) { { name: "successfully override prow job name", comment: "/override prow-job", - contexts: map[string]github.Status{ - "ci/prow/prow-job": { + contexts: []github.Status{ + { Context: "ci/prow/prow-job", Description: "failed", State: github.StatusFailure, }, }, - presubmits: map[string]config.Presubmit{ - "prow-job": { + presubmits: []config.Presubmit{ + { JobBase: config.JobBase{ Name: "prow-job", }, @@ -653,8 +662,8 @@ func TestHandle(t *testing.T) { }, }, jobs: sets.NewString("ci/prow/prow-job"), - expected: map[string]github.Status{ - "ci/prow/prow-job": { + expected: []github.Status{ + { Context: "ci/prow/prow-job", State: github.StatusSuccess, Description: description(adminUser), @@ -664,20 +673,20 @@ func TestHandle(t *testing.T) { { name: "override prow job and context", comment: "/override prow-job\n/override ci/prow/context", - contexts: map[string]github.Status{ - "ci/prow/context": { + contexts: []github.Status{ + { Context: "ci/prow/context", Description: "failed", State: github.StatusFailure, }, - "ci/prow/prow-job": { + { Context: "ci/prow/prow-job", Description: "failed", State: github.StatusFailure, }, }, - presubmits: map[string]config.Presubmit{ - "prow-job": { + presubmits: []config.Presubmit{ + { JobBase: config.JobBase{ Name: "prow-job", }, @@ -687,13 +696,13 @@ func TestHandle(t *testing.T) { }, }, jobs: sets.NewString("ci/prow/prow-job"), - expected: map[string]github.Status{ - "ci/prow/context": { + expected: []github.Status{ + { Context: "ci/prow/context", State: github.StatusSuccess, Description: description(adminUser), }, - "ci/prow/prow-job": { + { Context: "ci/prow/prow-job", State: github.StatusSuccess, Description: description(adminUser), @@ -703,15 +712,15 @@ func TestHandle(t *testing.T) { { name: "override same context and prow job", comment: "/override ci/prow/prow-job\n/override prow-job", - contexts: map[string]github.Status{ - "ci/prow/prow-job": { + contexts: []github.Status{ + { Context: "ci/prow/prow-job", Description: "failed", State: github.StatusFailure, }, }, - presubmits: map[string]config.Presubmit{ - "prow-job": { + presubmits: []config.Presubmit{ + { JobBase: config.JobBase{ Name: "prow-job", }, @@ -721,8 +730,8 @@ func TestHandle(t *testing.T) { }, }, jobs: sets.NewString("ci/prow/prow-job"), - expected: map[string]github.Status{ - "ci/prow/prow-job": { + expected: []github.Status{ + { Context: "ci/prow/prow-job", State: github.StatusSuccess, Description: description(adminUser), @@ -732,15 +741,15 @@ func TestHandle(t *testing.T) { { name: "override with explanation works", comment: "/override job\r\nobnoxious flake", // github ends lines with \r\n - contexts: map[string]github.Status{ - "job": { + contexts: []github.Status{ + { Context: "job", Description: "failed", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "job": { + expected: []github.Status{ + { Context: "job", Description: description(adminUser), State: github.StatusSuccess, @@ -753,15 +762,15 @@ func TestHandle(t *testing.T) { user: "code_owner", options: plugins.Override{AllowTopLevelOwners: true}, approvers: []string{"code_owner"}, - contexts: map[string]github.Status{ - "job": { + contexts: []github.Status{ + { Context: "job", Description: "failed", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "job": { + expected: []github.Status{ + { Context: "job", Description: description("code_owner"), State: github.StatusSuccess, @@ -774,15 +783,15 @@ func TestHandle(t *testing.T) { user: "Code_owner", options: plugins.Override{AllowTopLevelOwners: true}, approvers: []string{"code_owner"}, - contexts: map[string]github.Status{ - "job": { + contexts: []github.Status{ + { Context: "job", Description: "failed", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "job": { + expected: []github.Status{ + { Context: "job", Description: description("Code_owner"), State: github.StatusSuccess, @@ -794,15 +803,15 @@ func TestHandle(t *testing.T) { comment: "/override job", user: "non_code_owner", options: plugins.Override{AllowTopLevelOwners: true}, - contexts: map[string]github.Status{ - "job": { + contexts: []github.Status{ + { Context: "job", Description: "failed", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "job": { + expected: []github.Status{ + { Context: "job", Description: "failed", State: github.StatusFailure, @@ -818,15 +827,15 @@ func TestHandle(t *testing.T) { fmt.Sprintf("%s/%s", fakeOrg, fakeRepo): {"team-foo"}, }, }, - contexts: map[string]github.Status{ - "job": { + contexts: []github.Status{ + { Context: "job", Description: "failed", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "job": { + expected: []github.Status{ + { Context: "job", Description: description("user1"), State: github.StatusSuccess, @@ -842,15 +851,15 @@ func TestHandle(t *testing.T) { fmt.Sprintf("%s/%s", fakeOrg, fakeRepo): {"team-foo", "invalid-team-slug"}, }, }, - contexts: map[string]github.Status{ - "job": { + contexts: []github.Status{ + { Context: "job", Description: "failed", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "job": { + expected: []github.Status{ + { Context: "job", Description: description("user1"), State: github.StatusSuccess, @@ -861,14 +870,14 @@ func TestHandle(t *testing.T) { name: "override with empty branch protection", comment: "/override job", branchProtection: &github.BranchProtection{}, - expected: map[string]github.Status{}, + expected: []github.Status{}, checkComments: []string{}, }, { name: "override with branch protection empty status checks", comment: "/override job", branchProtection: &github.BranchProtection{RequiredStatusChecks: &github.RequiredStatusChecks{}}, - expected: map[string]github.Status{}, + expected: []github.Status{}, checkComments: []string{}, }, { @@ -877,8 +886,8 @@ func TestHandle(t *testing.T) { branchProtection: &github.BranchProtection{RequiredStatusChecks: &github.RequiredStatusChecks{ Contexts: []string{"job"}, }}, - expected: map[string]github.Status{ - "job": { + expected: []github.Status{ + { Context: "job", Description: description(adminUser), State: github.StatusSuccess, @@ -892,14 +901,14 @@ func TestHandle(t *testing.T) { branchProtection: &github.BranchProtection{RequiredStatusChecks: &github.RequiredStatusChecks{ Contexts: []string{"job"}, }}, - contexts: map[string]github.Status{ - "job": { + contexts: []github.Status{ + { Context: "job", State: github.StatusFailure, }, }, - expected: map[string]github.Status{ - "job": { + expected: []github.Status{ + { Context: "job", Description: description(adminUser), State: github.StatusSuccess, @@ -907,6 +916,50 @@ func TestHandle(t *testing.T) { }, checkComments: []string{"on behalf of " + adminUser}, }, + { + name: "handle only one status when multiple statuses have the same context", + comment: "/override problematic-test", + contexts: []github.Status{ + { + Context: "problematic-test", + State: github.StatusPending, + }, + { + Context: "problematic-test", + State: github.StatusFailure, + }, + { + Context: "problematic-test", + State: github.StatusPending, + }, + }, + presubmits: []config.Presubmit{ + { + JobBase: config.JobBase{ + Name: "problematic-test", + }, + Reporter: config.Reporter{ + Context: "problematic-test", + }, + }, + }, + jobs: sets.NewString("problematic-test"), + expected: []github.Status{ + { + Context: "problematic-test", + Description: description(adminUser), + State: github.StatusSuccess, + }, + { + Context: "problematic-test", + State: github.StatusFailure, + }, + { + Context: "problematic-test", + State: github.StatusPending, + }, + }, + }, } log := logrus.WithField("plugin", pluginName) @@ -925,7 +978,7 @@ func TestHandle(t *testing.T) { tc.action = github.GenericCommentActionCreated } if tc.contexts == nil { - tc.contexts = map[string]github.Status{} + tc.contexts = []github.Status{} } event := github.GenericCommentEvent{