Skip to content

Commit 8e4dc4c

Browse files
committed
Don't try to cleanup Pending PipelineRuns
When using max-keep-runs annotations with concurrency, the cleanups process on the watcher was trying to cleanup Pending PipelineRuns. This would cause the watcher to get stuck and not able to process its queue. We are now avoiding it the same way we do for Running PipelineRuns. Improved TestGithubSecondPullRequestConcurrencyMultiplePR to include max-keep-runs and /retest over concurrency Increase the timeout for running the e2e tests since that test is getting a bit slower. Jira: https://issues.redhat.com/browse/SRVKP-4266 Signed-off-by: Chmouel Boudjnah <[email protected]>
1 parent be55a24 commit 8e4dc4c

File tree

6 files changed

+193
-10
lines changed

6 files changed

+193
-10
lines changed

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ LDFLAGS=
77
OUTPUT_DIR=bin
88
GO = go
99
TIMEOUT_UNIT = 20m
10-
TIMEOUT_E2E = 30m
10+
TIMEOUT_E2E = 45m
1111
GO_TEST_FLAGS +=
1212
SHELL := bash
1313

docs/content/docs/guide/cleanups.md

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,23 @@ weight: 8
44
---
55
# PipelineRuns Cleanups
66

7-
There can be many PipelineRuns into a user namespace and Pipelines-as-Code
8-
has the ability to only keep several PipelineRuns that matches an event.
7+
There can be many PipelineRuns into a user namespace and Pipelines-as-Code has
8+
the ability to only keep a certain amount of PipelineRuns and cleaning the old
9+
ones.
910

10-
For example if the PipelineRun has this annotation :
11+
When your PipelineRun has this annotation :
1112

1213
```yaml
1314
pipelinesascode.tekton.dev/max-keep-runs: "maxNumber"
1415
```
1516
16-
Pipelines-as-Code sees this and will start cleaning up right after it finishes a
17-
successful execution keeping only the maxNumber of PipelineRuns.
17+
Pipelines-as-Code sees this and will start cleaning up right after one of the
18+
PipelineRun finishes to a successful execution keeping only the last `maxNumber` of
19+
PipelineRuns.
1820

19-
It will skip the `Running` PipelineRuns but will not skip the PipelineRuns with
20-
`Unknown` status.
21+
It will skip the `Running` or `Pending` PipelineRuns but will not skip the
22+
PipelineRuns with `Unknown` status.
23+
24+
{{< hint info >}}
25+
The setting can be as well configured globally for a cluster via the [Pipelines-as-Code ConfigMap]({{< relref "/docs/install/settings.md" >}})
26+
{{< /hint >}}

pkg/kubeinteraction/cleanups.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,9 @@ func (k Interaction) CleanupPipelines(ctx context.Context, logger *zap.SugaredLo
3131
}
3232

3333
for c, prun := range psort.PipelineRunSortByCompletionTime(pruns.Items) {
34-
if prun.GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Running" {
35-
logger.Infof("skipping %s since currently running", prun.GetName())
34+
prReason := prun.GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason()
35+
if prReason == tektonv1.PipelineRunReasonRunning.String() || prReason == tektonv1.PipelineRunReasonPending.String() {
36+
logger.Infof("skipping cleaning PipelineRun %s since the conditions.reason is %s", prReason, prun.GetName())
3637
continue
3738
}
3839

pkg/kubeinteraction/cleanups_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,22 @@ func TestCleanupPipelines(t *testing.T) {
8383
prunLatestInList: "pipeline-running",
8484
},
8585
},
86+
{
87+
name: "cleanup-skip-pending",
88+
args: args{
89+
namespace: ns,
90+
repositoryName: cleanupRepoName,
91+
maxKeep: 1,
92+
kept: 1, // see my comment in code why only 1 is kept.
93+
prunCurrent: &tektonv1.PipelineRun{ObjectMeta: metav1.ObjectMeta{Labels: cleanupLabels, Annotations: cleanupAnnotations}},
94+
pruns: []*tektonv1.PipelineRun{
95+
tektontest.MakePRCompletion(clock, "pipeline-pending", ns, tektonv1.PipelineRunReasonPending.String(), nil, cleanupLabels, 10),
96+
tektontest.MakePRCompletion(clock, "pipeline-toclean", ns, tektonv1.PipelineRunReasonSuccessful.String(), nil, cleanupLabels, 30),
97+
tektontest.MakePRCompletion(clock, "pipeline-tokeep", ns, tektonv1.PipelineRunReasonSuccessful.String(), nil, cleanupLabels, 20),
98+
},
99+
prunLatestInList: "pipeline-pending",
100+
},
101+
},
86102
{
87103
name: "cleanup with secrets",
88104
args: args{
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
//go:build e2e
2+
// +build e2e
3+
4+
package test
5+
6+
import (
7+
"context"
8+
"fmt"
9+
"net/http"
10+
"os"
11+
"strings"
12+
"testing"
13+
"time"
14+
15+
"github.com/google/go-github/v56/github"
16+
"github.com/openshift-pipelines/pipelines-as-code/pkg/random"
17+
tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github"
18+
"github.com/openshift-pipelines/pipelines-as-code/test/pkg/options"
19+
"github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload"
20+
"github.com/openshift-pipelines/pipelines-as-code/test/pkg/repository"
21+
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
22+
"github.com/tektoncd/pipeline/pkg/names"
23+
"gotest.tools/v3/assert"
24+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
25+
)
26+
27+
// TestGithubSecondPullRequestConcurrencyMultiplePR concurrency for the same Repository over multiples PR including a /retest
28+
// and a max-keep-run, may be a bit slow (180s at least) but it's worth it.
29+
func TestGithubSecondPullRequestConcurrencyMultiplePR(t *testing.T) {
30+
ctx := context.Background()
31+
label := "Github Multiple PullRequest Concurrency-1 MaxKeepRun-1 Multiple"
32+
numberOfPullRequest := 3
33+
numberOfPipelineRuns := 3
34+
numberOfRetests := 1
35+
maxNumberOfConcurrentPipelineRuns := 1
36+
maxKeepRun := 1
37+
allPipelinesRunsCnt := (numberOfPullRequest * numberOfPipelineRuns) + (numberOfPullRequest * numberOfRetests * numberOfPipelineRuns)
38+
allPipelinesRunAfterCleanUp := allPipelinesRunsCnt / (maxKeepRun + 1)
39+
loopMax := 35
40+
41+
targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
42+
_, runcnx, opts, ghcnx, err := tgithub.Setup(ctx, true, false)
43+
assert.NilError(t, err)
44+
45+
runcnx.Clients.Log.Infof("Starting %d pipelineruns, (numberOfPullRequest=%d*numberOfPipelineRuns=%d) + (numberOfPullRequest=%d*numberOfRetests=%d*numberOfPipelineRuns=%d) Should end after clean up (maxKeepRun=%d) with %d",
46+
allPipelinesRunsCnt, numberOfPullRequest, numberOfPipelineRuns, numberOfPullRequest, numberOfRetests, numberOfPipelineRuns, maxKeepRun, allPipelinesRunAfterCleanUp)
47+
48+
repoinfo, resp, err := ghcnx.Client.Repositories.Get(ctx, opts.Organization, opts.Repo)
49+
assert.NilError(t, err)
50+
if resp != nil && resp.StatusCode == http.StatusNotFound {
51+
t.Errorf("Repository %s not found in %s", opts.Organization, opts.Repo)
52+
}
53+
// set concurrency
54+
opts.Concurrency = maxNumberOfConcurrentPipelineRuns
55+
err = tgithub.CreateCRD(ctx, t, repoinfo, runcnx, opts, targetNS)
56+
assert.NilError(t, err)
57+
58+
allPullRequests := []int{}
59+
for prc := 0; prc < numberOfPullRequest; prc++ {
60+
branchName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("branch")
61+
logmsg := fmt.Sprintf("Testing %s with Github APPS integration branch %s namespace %s", label, branchName, targetNS)
62+
yamlFiles := map[string]string{}
63+
randomAlphaString := strings.ToLower(random.AlphaString(4))
64+
for i := 1; i <= numberOfPipelineRuns; i++ {
65+
yamlFiles[fmt.Sprintf(".tekton/prlongrunnning-%s-%d.yaml", randomAlphaString, i)] = "testdata/pipelinerun_long_running_maxkeep_run.yaml"
66+
}
67+
68+
entries, err := payload.GetEntries(yamlFiles, targetNS, options.MainBranch, "pull_request", map[string]string{
69+
"MaxKeepRun": fmt.Sprint(maxKeepRun),
70+
})
71+
assert.NilError(t, err)
72+
73+
targetRefName := fmt.Sprintf("refs/heads/%s",
74+
names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test"))
75+
76+
sha, vref, err := tgithub.PushFilesToRef(ctx, ghcnx.Client, logmsg, repoinfo.GetDefaultBranch(), targetRefName,
77+
opts.Organization, opts.Repo, entries)
78+
assert.NilError(t, err)
79+
runcnx.Clients.Log.Infof("Commit %s has been created and pushed to %s", sha, vref.GetURL())
80+
81+
prNumber, err := tgithub.PRCreate(ctx, runcnx, ghcnx, opts.Organization, opts.Repo, targetRefName, repoinfo.GetDefaultBranch(), logmsg)
82+
assert.NilError(t, err)
83+
84+
defer tgithub.TearDown(ctx, t, runcnx, ghcnx, prNumber, targetRefName, targetNS, opts)
85+
allPullRequests = append(allPullRequests, prNumber)
86+
}
87+
88+
// send some retest to spice things up on concurrency and test the maxKeepRun
89+
for i := 0; i < numberOfRetests; i++ {
90+
for _, prNumber := range allPullRequests {
91+
_, _, err := ghcnx.Client.Issues.CreateComment(ctx,
92+
opts.Organization,
93+
opts.Repo, prNumber,
94+
&github.IssueComment{Body: github.String("/retest")})
95+
assert.NilError(t, err)
96+
}
97+
}
98+
99+
finished := false
100+
for i := 0; i < loopMax; i++ {
101+
unsuccessful := 0
102+
prs, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{})
103+
assert.NilError(t, err)
104+
for _, pr := range prs.Items {
105+
if pr.Status.GetConditions() == nil {
106+
unsuccessful++
107+
continue
108+
}
109+
for _, condition := range pr.Status.GetConditions() {
110+
if condition.Status == "Unknown" || condition.GetReason() == tektonv1.PipelineRunSpecStatusPending {
111+
unsuccessful++
112+
continue
113+
}
114+
}
115+
}
116+
if unsuccessful == 0 {
117+
finished = true
118+
break
119+
}
120+
runcnx.Clients.Log.Infof("number of unsuccessful PR %d out of %d, waiting 10s more, %d/%d", unsuccessful, allPipelinesRunsCnt, i, loopMax)
121+
// it's high because it takes time to process on kind
122+
time.Sleep(10 * time.Second)
123+
}
124+
if !finished {
125+
t.Errorf("we didn't get %d pipelineruns as successful, some of them are still pending or it's abnormally slow to process the Q", allPipelinesRunsCnt)
126+
}
127+
128+
prs, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{})
129+
assert.NilError(t, err)
130+
assert.Equal(t, len(prs.Items), allPipelinesRunAfterCleanUp, "we should have had %d kept after cleanup, we got %d", allPipelinesRunAfterCleanUp, len(prs.Items))
131+
132+
runcnx.Clients.Log.Infof("success: number of cleaned PR is %d we expected to have %d after the cleanup", len(prs.Items), allPipelinesRunAfterCleanUp)
133+
134+
if os.Getenv("TEST_NOCLEANUP") != "true" {
135+
repository.NSTearDown(ctx, t, runcnx, targetNS)
136+
return
137+
}
138+
}
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
apiVersion: tekton.dev/v1beta1
3+
kind: PipelineRun
4+
metadata:
5+
name: "\\ .PipelineName //"
6+
annotations:
7+
pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //"
8+
pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]"
9+
pipelinesascode.tekton.dev/on-event: "[\\ .TargetEvent //]"
10+
pipelinesascode.tekton.dev/max-keep-runs: "\\ .MaxKeepRun //"
11+
spec:
12+
pipelineSpec:
13+
tasks:
14+
- name: task
15+
taskSpec:
16+
steps:
17+
- name: task
18+
image: registry.access.redhat.com/ubi9/ubi-micro
19+
script: |
20+
echo "hello pipeline"
21+
sleep 10
22+
exit 0

0 commit comments

Comments
 (0)