From 6e8c27817bbea038434ffd4daf8026ed6bdc3708 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 21 Nov 2024 12:43:37 +0100 Subject: [PATCH] fix: Fix cancel in progress to act same pr/branch We need to ensure that we only cancel the pipeline run that is running on the same branch as the current PR or push event. This ensures that we don't cancel the wrong pipeline run. On PullRequest events, we only cancel the pipeline run that is running on the same Pull Request number. On Push events, we only cancel the pipeline run that is running on the same SourceBranch (which is the same as HeadBranch on push) This avoids the issue when you have multiple pull request running on a repo and we don't cancel the wrong ones that are running for a different PR. Signed-off-by: Chmouel Boudjnah --- docs/content/docs/guide/running.md | 13 +- pkg/pipelineascode/cancel_pipelineruns.go | 36 ++- .../cancel_pipelineruns_test.go | 212 ++++++++++++++++-- test/gitea_test.go | 64 +++++- 4 files changed, 278 insertions(+), 47 deletions(-) diff --git a/docs/content/docs/guide/running.md b/docs/content/docs/guide/running.md index 950b1e6c4..5649a852c 100644 --- a/docs/content/docs/guide/running.md +++ b/docs/content/docs/guide/running.md @@ -119,9 +119,16 @@ has already completed or has been cancelled, it will be skipped. (For persistent settings, refer to the [max-keep-run annotation]({{< relref "/docs/guide/cleanups.md" >}}) instead.) -The cancellation occurs after the latest PipelineRun has been successfully -created and started. This annotation cannot be used to ensure that only one -PipelineRun is active at any time. +The cancellation selection is filtered by the current `PullRequest` or a +by the targeted branch in the Push event. This means that if for example there +are two `PullRequests`, each associated with a `PipelineRun` of the same name +and marked with the `cancel-in-progress` annotation, the cancellation will +cancel only the `PipelineRun` currently in progress for the specific +`PullRequest`. This ensures no interference between the two `PullRequests`. + +The cancellation is not immediate and only occurs after the latest PipelineRun +has been successfully created and started. This annotation cannot be relied to +ensure that only one PipelineRun is active at any time. Currently, `cancel-in-progress` cannot be used in conjunction with [concurrency limit]({{< relref "/docs/guide/repositorycrd.md#concurrency" >}}). diff --git a/pkg/pipelineascode/cancel_pipelineruns.go b/pkg/pipelineascode/cancel_pipelineruns.go index de46d1e2d..9da6d67e7 100644 --- a/pkg/pipelineascode/cancel_pipelineruns.go +++ b/pkg/pipelineascode/cancel_pipelineruns.go @@ -6,16 +6,17 @@ import ( "strconv" "sync" - "github.com/openshift-pipelines/pipelines-as-code/pkg/action" - "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" - "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" - "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" - "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "go.uber.org/zap" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" + + "github.com/openshift-pipelines/pipelines-as-code/pkg/action" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" ) var cancelMergePatch = map[string]interface{}{ @@ -46,13 +47,19 @@ func (p *PacRun) cancelInProgress(ctx context.Context, matchPR *tektonv1.Pipelin if !ok { return nil } - labelSelector := getLabelSelector(map[string]string{ + + labelMap := map[string]string{ keys.URLRepository: formatting.CleanValueKubernetes(p.event.Repository), keys.OriginalPRName: prName, - }) - + keys.EventType: p.event.TriggerTarget.String(), + } + if p.event.TriggerTarget == triggertype.PullRequest { + labelMap[keys.PullRequest] = strconv.Itoa(p.event.PullRequestNumber) + } + labelMaps := getLabelSelector(labelMap) + p.run.Clients.Log.Infof("cancel-in-progress: selecting pipelineRuns to cancel with labels: %v", labelMaps) prs, err := p.run.Clients.Tekton.TektonV1().PipelineRuns(matchPR.GetNamespace()).List(ctx, metav1.ListOptions{ - LabelSelector: labelSelector, + LabelSelector: labelMaps, }) if err != nil { return fmt.Errorf("failed to list pipelineRuns : %w", err) @@ -62,6 +69,17 @@ func (p *PacRun) cancelInProgress(ctx context.Context, matchPR *tektonv1.Pipelin if pr.GetName() == matchPR.GetName() { continue } + if sourceBranch, ok := pr.GetAnnotations()[keys.SourceBranch]; ok { + // NOTE(chmouel): Every PR has their own branch and so is every push to different branch + // it means we only cancel pipelinerun of the same name that runs to + // the unique branch. Note: HeadBranch is the branch from where the PR + // comes from in git jargon. + if sourceBranch != p.event.HeadBranch { + p.logger.Infof("cancel-in-progress: skipping pipelinerun %v/%v as it is not from the same branch, annotation source-branch: %s event headbranch: %s", pr.GetNamespace(), pr.GetName(), sourceBranch, p.event.HeadBranch) + continue + } + } + if pr.IsDone() { continue } diff --git a/pkg/pipelineascode/cancel_pipelineruns_test.go b/pkg/pipelineascode/cancel_pipelineruns_test.go index db00e0500..4c69e65f9 100644 --- a/pkg/pipelineascode/cancel_pipelineruns_test.go +++ b/pkg/pipelineascode/cancel_pipelineruns_test.go @@ -6,13 +6,6 @@ import ( "testing" "github.com/google/go-github/v64/github" - "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" - "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" - "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" - "github.com/openshift-pipelines/pipelines-as-code/pkg/params" - "github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients" - "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" - testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "go.uber.org/zap" zapobserver "go.uber.org/zap/zaptest/observer" @@ -22,10 +15,20 @@ import ( "knative.dev/pkg/apis" knativeduckv1 "knative.dev/pkg/apis/duck/v1" rtesting "knative.dev/pkg/reconciler/testing" + + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" + testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" ) var ( - fooRepo = &v1alpha1.Repository{ + pullReqNumber = 11 + fooRepo = &v1alpha1.Repository{ ObjectMeta: metav1.ObjectMeta{ Namespace: "foo", Name: "foo", @@ -42,25 +45,26 @@ var ( keys.OriginalPRName: "pr-foo", keys.URLRepository: formatting.CleanValueKubernetes("foo"), keys.SHA: formatting.CleanValueKubernetes("foosha"), - keys.PullRequest: strconv.Itoa(11), + keys.PullRequest: strconv.Itoa(pullReqNumber), + keys.EventType: string(triggertype.PullRequest), } fooRepoAnnotations = map[string]string{ keys.URLRepository: "foo", keys.SHA: "foosha", - keys.PullRequest: strconv.Itoa(11), + keys.PullRequest: strconv.Itoa(pullReqNumber), keys.Repository: "foo", } fooRepoLabelsPrFooAbc = map[string]string{ keys.URLRepository: formatting.CleanValueKubernetes("foo"), keys.SHA: formatting.CleanValueKubernetes("foosha"), - keys.PullRequest: strconv.Itoa(11), + keys.PullRequest: strconv.Itoa(pullReqNumber), keys.OriginalPRName: "pr-foo-abc", keys.Repository: "foo", } fooRepoAnnotationsPrFooAbc = map[string]string{ keys.URLRepository: "foo", keys.SHA: "foosha", - keys.PullRequest: strconv.Itoa(11), + keys.PullRequest: strconv.Itoa(pullReqNumber), keys.OriginalPRName: "pr-foo-abc", keys.Repository: "foo", } @@ -82,7 +86,7 @@ func TestCancelPipelinerun(t *testing.T) { Repository: "foo", SHA: "foosha", TriggerTarget: "pull_request", - PullRequestNumber: 11, + PullRequestNumber: pullReqNumber, State: info.State{ CancelPipelineRuns: true, }, @@ -108,7 +112,7 @@ func TestCancelPipelinerun(t *testing.T) { Repository: "foo", SHA: "foosha", TriggerTarget: "pull_request", - PullRequestNumber: 11, + PullRequestNumber: pullReqNumber, State: info.State{ CancelPipelineRuns: true, }, @@ -123,7 +127,7 @@ func TestCancelPipelinerun(t *testing.T) { Repository: "foo", SHA: "foosha", TriggerTarget: "pull_request", - PullRequestNumber: 11, + PullRequestNumber: pullReqNumber, State: info.State{ CancelPipelineRuns: true, TargetCancelPipelineRun: "pr-foo-abc", @@ -169,7 +173,7 @@ func TestCancelPipelinerun(t *testing.T) { Repository: "foo", SHA: "foosha", TriggerTarget: "pull_request", - PullRequestNumber: 11, + PullRequestNumber: pullReqNumber, State: info.State{ CancelPipelineRuns: true, }, @@ -328,16 +332,25 @@ func TestCancelInProgress(t *testing.T) { { name: "cancel in progress", event: &info.Event{ - Repository: "foo", - SHA: "foosha", + Repository: "foo", + SHA: "foosha", + HeadBranch: "head", + EventType: string(triggertype.PullRequest), + TriggerTarget: triggertype.PullRequest, + PullRequestNumber: pullReqNumber, }, pipelineRuns: []*pipelinev1.PipelineRun{ { ObjectMeta: metav1.ObjectMeta{ - Name: "pr-foo", - Namespace: "foo", - Labels: fooRepoLabels, - Annotations: map[string]string{keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo", keys.Repository: "foo"}, + Name: "pr-foo", + Namespace: "foo", + Labels: fooRepoLabels, + Annotations: map[string]string{ + keys.CancelInProgress: "true", + keys.OriginalPRName: "pr-foo", + keys.Repository: "foo", + keys.SourceBranch: "head", + }, }, Spec: pipelinev1.PipelineRunSpec{}, }, @@ -346,7 +359,11 @@ func TestCancelInProgress(t *testing.T) { GenerateName: "pr-foo-", Namespace: "foo", Labels: fooRepoLabels, - Annotations: map[string]string{keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo", keys.Repository: "foo"}, + Annotations: map[string]string{ + keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo", + keys.Repository: "foo", + keys.SourceBranch: "head", + }, }, Spec: pipelinev1.PipelineRunSpec{}, }, @@ -357,6 +374,155 @@ func TestCancelInProgress(t *testing.T) { }, wantLog: "cancel-in-progress: cancelling pipelinerun foo/", }, + { + name: "cancel in progress exclude not belonging to same push branch", + event: &info.Event{ + Repository: "foo", + SHA: "foosha", + HeadBranch: "head", + EventType: string(triggertype.Push), + TriggerTarget: triggertype.Push, + }, + pipelineRuns: []*pipelinev1.PipelineRun{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pr-foo", + Namespace: "foo", + Labels: map[string]string{ + keys.OriginalPRName: "pr-foo", + keys.URLRepository: formatting.CleanValueKubernetes("foo"), + keys.SHA: formatting.CleanValueKubernetes("foosha"), + keys.EventType: string(triggertype.Push), + }, + Annotations: map[string]string{ + keys.CancelInProgress: "true", + keys.OriginalPRName: "pr-foo", + keys.Repository: "foo", + keys.SourceBranch: "head", + }, + }, + Spec: pipelinev1.PipelineRunSpec{}, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pr-foo-1", + Namespace: "foo", + Labels: map[string]string{ + keys.OriginalPRName: "pr-foo", + keys.URLRepository: formatting.CleanValueKubernetes("foo"), + keys.SHA: formatting.CleanValueKubernetes("foosha"), + keys.EventType: string(triggertype.Push), + }, + Annotations: map[string]string{ + keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo", + keys.Repository: "foo", + keys.SourceBranch: "anotherhead", + }, + }, + Spec: pipelinev1.PipelineRunSpec{}, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pr-foo-2", + Namespace: "foo", + Labels: map[string]string{ + keys.OriginalPRName: "pr-foo", + keys.URLRepository: formatting.CleanValueKubernetes("foo"), + keys.SHA: formatting.CleanValueKubernetes("foosha"), + keys.EventType: string(triggertype.Push), + }, + Annotations: map[string]string{ + keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo", + keys.Repository: "foo", + keys.SourceBranch: "head", + }, + }, + Spec: pipelinev1.PipelineRunSpec{}, + }, + }, + repo: fooRepo, + cancelledPipelineRuns: map[string]bool{ + "pr-foo-2": true, + }, + wantLog: "skipping pipelinerun foo/pr-foo-1 as it is not from the same branch", + }, + { + name: "cancel in progress exclude not belonging to same pr", + event: &info.Event{ + Repository: "foo", + SHA: "foosha", + HeadBranch: "head", + EventType: string(triggertype.PullRequest), + TriggerTarget: triggertype.PullRequest, + PullRequestNumber: pullReqNumber, + }, + pipelineRuns: []*pipelinev1.PipelineRun{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pr-foo", + Namespace: "foo", + Labels: map[string]string{ + keys.OriginalPRName: "pr-foo", + keys.URLRepository: formatting.CleanValueKubernetes("foo"), + keys.SHA: formatting.CleanValueKubernetes("foosha"), + keys.PullRequest: strconv.Itoa(pullReqNumber), + keys.EventType: string(triggertype.PullRequest), + }, + Annotations: map[string]string{ + keys.CancelInProgress: "true", + keys.OriginalPRName: "pr-foo", + keys.Repository: "foo", + keys.SourceBranch: "head", + }, + }, + Spec: pipelinev1.PipelineRunSpec{}, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pr-foo-1", + Namespace: "foo", + Labels: map[string]string{ + keys.OriginalPRName: "pr-foo", + keys.URLRepository: formatting.CleanValueKubernetes("foo"), + keys.SHA: formatting.CleanValueKubernetes("foosha"), + keys.PullRequest: strconv.Itoa(10), + keys.EventType: string(triggertype.PullRequest), + }, + Annotations: map[string]string{ + keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo", + keys.Repository: "foo", + keys.SourceBranch: "head", + }, + }, + Spec: pipelinev1.PipelineRunSpec{}, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pr-foo-2", + Namespace: "foo", + Labels: map[string]string{ + keys.OriginalPRName: "pr-foo", + keys.URLRepository: formatting.CleanValueKubernetes("foo"), + keys.SHA: formatting.CleanValueKubernetes("foosha"), + keys.PullRequest: strconv.Itoa(pullReqNumber), + keys.EventType: string(triggertype.PullRequest), + }, + Annotations: map[string]string{ + keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo", + keys.Repository: "foo", + keys.SourceBranch: "head", + }, + }, + Spec: pipelinev1.PipelineRunSpec{}, + }, + }, + repo: fooRepo, + cancelledPipelineRuns: map[string]bool{ + "pr-foo-2": true, + }, + wantLog: "cancel-in-progress: cancelling pipelinerun foo/", + }, + { name: "cancel in progress with concurrency limit", event: &info.Event{ diff --git a/test/gitea_test.go b/test/gitea_test.go index 5d4dd2ecc..97f7fd170 100644 --- a/test/gitea_test.go +++ b/test/gitea_test.go @@ -378,29 +378,69 @@ func TestGiteaConfigMaxKeepRun(t *testing.T) { assert.Equal(t, len(prs.Items), 1, "should have only one pipelinerun, but we have: %d", len(prs.Items)) } +// TestGiteaConfigCancelInProgress will test the pipelinerun annotation +// `pipelinesascode.tekton.dev/cancel-in-progress: "true", it will first start +// one Pull Request which will run a PipelineRun and then send a /retest which +// should cancel the in progress one. +// It will create a new branch and push a new Pull Request with a PipelineRun of +// the same name of the first PR and make sure PipelineRun of the same name only +// acts on the same Pull Request and not on the one of the others. func TestGiteaConfigCancelInProgress(t *testing.T) { + prmap := map[string]string{".tekton/pr.yaml": "testdata/pipelinerun-cancel-in-progress.yaml"} topts := &tgitea.TestOpts{ - TargetEvent: triggertype.PullRequest.String(), - YAMLFiles: map[string]string{ - ".tekton/pr.yaml": "testdata/pipelinerun-cancel-in-progress.yaml", - }, + TargetEvent: triggertype.PullRequest.String(), + YAMLFiles: prmap, CheckForStatus: "", ExpectEvents: false, Regexp: nil, } _, f := tgitea.TestPR(t, topts) defer f() + + time.Sleep(3 * time.Second) // “Evil does not sleep. It waits.” - Galadriel + // wait a bit that the pipelinerun had created tgitea.PostCommentOnPullRequest(t, topts, "/retest") - time.Sleep(5 * time.Second) // “Evil does not sleep. It waits.” - Galadriel + time.Sleep(2 * time.Second) // “Evil does not sleep. It waits.” - Galadriel + + targetRef := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("cancel-in-progress") + entries, err := payload.GetEntries(prmap, topts.TargetNS, topts.DefaultBranch, topts.TargetEvent, map[string]string{}) + assert.NilError(t, err) + topts.TargetRefName = topts.DefaultBranch + scmOpts := &scm.Opts{ + GitURL: topts.GitCloneURL, + Log: topts.ParamsRun.Clients.Log, + WebURL: topts.GitHTMLURL, + TargetRefName: targetRef, + BaseRefName: topts.DefaultBranch, + NoCheckOutFromBase: true, + } + scm.PushFilesToRefGit(t, scmOpts, entries) + + pr, _, err := topts.GiteaCNX.Client.CreatePullRequest(topts.Opts.Organization, topts.Opts.Repo, gitea.CreatePullRequestOption{ + Title: "Test Pull Request - " + targetRef, + Head: targetRef, + Base: topts.DefaultBranch, + }) + assert.NilError(t, err) + topts.PullRequest = pr + topts.ParamsRun.Clients.Log.Infof("PullRequest %s has been created", pr.HTMLURL) + topts.CheckForStatus = "success" + tgitea.WaitForStatus(t, topts, "heads/"+targetRef, "", false) prs, err := topts.ParamsRun.Clients.Tekton.TektonV1().PipelineRuns(topts.TargetNS).List(context.Background(), metav1.ListOptions{}) assert.NilError(t, err) sort.PipelineRunSortByStartTime(prs.Items) - assert.Equal(t, len(prs.Items), 2, "should have 2 pipelineruns, but we have: %d", len(prs.Items)) - assert.Equal(t, prs.Items[1].GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason(), "Cancelled", "oldest pr should have been cancelled") + assert.Equal(t, len(prs.Items), 3, "should have 2 pipelineruns, but we have: %d", len(prs.Items)) + cancelledPr := 0 + for _, pr := range prs.Items { + if pr.GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Cancelled" { + cancelledPr++ + } + } + assert.Equal(t, cancelledPr, 1, "only one pr should have been canceled") } func TestGiteaPush(t *testing.T) { @@ -781,7 +821,7 @@ func TestGiteaProvenanceForDefaultBranch(t *testing.T) { Settings: &v1alpha1.Settings{PipelineRunProvenance: "default_branch"}, NoPullRequestCreation: true, } - verifyProvinance(t, topts, "HELLOMOTO", "step-task", false) + verifyProvenance(t, topts, "HELLOMOTO", "step-task", false) } // TestGiteaProvenanceForSource tests the provenance feature of the PipelineRun. @@ -793,7 +833,7 @@ func TestGiteaProvenanceForSource(t *testing.T) { Settings: &v1alpha1.Settings{PipelineRunProvenance: "source"}, NoPullRequestCreation: true, } - verifyProvinance(t, topts, "testing provenance for source", "step-source-provenance-test", false) + verifyProvenance(t, topts, "testing provenance for source", "step-source-provenance-test", false) } // TestGiteaGlobalRepoProvenanceForDefaultBranch tests the provenance feature of the PipelineRun. @@ -808,7 +848,7 @@ func TestGiteaGlobalRepoProvenanceForDefaultBranch(t *testing.T) { Settings: &v1alpha1.Settings{}, } - verifyProvinance(t, topts, "HELLOMOTO", "step-task", true) + verifyProvenance(t, topts, "HELLOMOTO", "step-task", true) } // TestGiteaGlobalAndLocalRepoProvenance verifies the provenance feature of the PipelineRun, @@ -824,10 +864,10 @@ func TestGiteaGlobalAndLocalRepoProvenance(t *testing.T) { }, } - verifyProvinance(t, topts, "testing provenance for source", "step-source-provenance-test", true) + verifyProvenance(t, topts, "testing provenance for source", "step-source-provenance-test", true) } -func verifyProvinance(t *testing.T, topts *tgitea.TestOpts, expectedOutput, cName string, isGlobal bool) { +func verifyProvenance(t *testing.T, topts *tgitea.TestOpts, expectedOutput, cName string, isGlobal bool) { if isGlobal { ctx := context.Background() topts.ParamsRun, topts.Opts, topts.GiteaCNX, _ = tgitea.Setup(ctx)