From 365b061c62eaf157052b494cdec9ec8c200e26da Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Wed, 29 May 2024 14:26:38 +0200 Subject: [PATCH] Add test for when concurrency and startPR error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit when we cannot create the PipelineRun and we got an error from the controller, if we had concurrency set the controller would crash. This was fixed in e553256f9c1d51371ac2b59320a257164c8276bb but added test for this ``` 💡 11:25:51 pac-controller pipelinerun test-gh-kjtd8 has been created in namespace pac-e2e-ns-7bw6b for SHA: 4a82af8eb94e78576bc0f9ab3e1a7c5582825412 Target Branch: main 🚨 11:25:51 pac-controller There was an error starting the PipelineRun 00-bad-apple-tdza-, creating pipelinerun 00-bad-apple-tdza- in namespace pac-e2e-ns-7bw6b has failed. Tekton Controller has reported this error: ```admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid value: couldn't add link between noexist and donotexist: task noexist depends on donotexist but donotexist wasn't present in Pipeline: spec.pipelineSpec.tasks``` 💡 11:25:52 pac-controller patched pipelinerun with checkRunID and logURL: pac-e2e-ns-7bw6b/test-gh-1-6x879 💡 11:25:52 pac-controller patched pipelinerun with checkRunID and logURL: pac-e2e-ns-7bw6b/test-gh-kjtd8 💡 11:25:52 pac-controller patched pipelinerun with checkRunID and logURL: pac-e2e-ns-7bw6b/prlongrunnning-1-fjrs-qckbv 💡 11:25:52 pac-controller patched pipelinerun with checkRunID and logURL: pac-e2e-ns-7bw6b/test-gh-2-tjdwf 💡 11:25:52 pac-controller skipping event: check_run: unsupported action "created" 💡 11:25:52 pac-controller skipping event: check_run: unsupported action "created" 💡 11:25:52 pac-controller skipping event: check_run: unsupported action "created" 💡 11:25:52 pac-controller skipping event: check_run: unsupported action "created" 💡 11:25:52 pac-controller skipping event: check_run: unsupported action "created" pipelines-as-code/ghe-controller-6b68d9cfb-dc6lg[pac-controller]: panic: reflect: call of reflect.Value.Interface on zero Value pipelines-as-code/ghe-controller-6b68d9cfb-dc6lg[pac-controller]: pipelines-as-code/ghe-controller-6b68d9cfb-dc6lg[pac-controller]: goroutine 563 [running]: pipelines-as-code/ghe-controller-6b68d9cfb-dc6lg[pac-controller]: reflect.valueInterface({0x0?, 0x0?, 0x4000da0d40?}, 0xe0?) pipelines-as-code/ghe-controller-6b68d9cfb-dc6lg[pac-controller]: reflect/value.go:1501 +0xfc pipelines-as-code/ghe-controller-6b68d9cfb-dc6lg[pac-controller]: ``` fixing and add e2e tests for it. --- test/github_pullrequest_concurrency_test.go | 54 +++++++++++++++++---- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/test/github_pullrequest_concurrency_test.go b/test/github_pullrequest_concurrency_test.go index 83c45722d..904f6800b 100644 --- a/test/github_pullrequest_concurrency_test.go +++ b/test/github_pullrequest_concurrency_test.go @@ -7,9 +7,11 @@ import ( "context" "fmt" "net/http" + "strings" "testing" "time" + "github.com/openshift-pipelines/pipelines-as-code/pkg/sort" tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github" "github.com/openshift-pipelines/pipelines-as-code/test/pkg/options" "github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload" @@ -20,14 +22,37 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func TestGithubPullRequestConcurrency(t *testing.T) { +func TestGithubSecondPullRequestConcurrency1by1(t *testing.T) { ctx := context.Background() - label := "Github PullRequest Concurrent" + label := "Github PullRequest Concurrent, sequentially one by one" + numberOfPipelineRuns := 5 + maxNumberOfConcurrentPipelineRuns := 1 + testGithubConcurrency(ctx, t, maxNumberOfConcurrentPipelineRuns, numberOfPipelineRuns, label, true, map[string]string{}) +} + +func TestGithubSecondPullRequestConcurrency3by3(t *testing.T) { + ctx := context.Background() + label := "Github PullRequest Concurrent three at time" numberOfPipelineRuns := 10 maxNumberOfConcurrentPipelineRuns := 3 + testGithubConcurrency(ctx, t, maxNumberOfConcurrentPipelineRuns, numberOfPipelineRuns, label, false, map[string]string{}) +} + +// TestGithubSecondPullRequestConcurrency1by1WithError will test concurrency and an error, this used to crash for us previously. +func TestGithubSecondPullRequestConcurrency1by1WithError(t *testing.T) { + ctx := context.Background() + label := "Github PullRequest Concurrent, sequentially one by one with one bad apple" + numberOfPipelineRuns := 1 + maxNumberOfConcurrentPipelineRuns := 1 + testGithubConcurrency(ctx, t, maxNumberOfConcurrentPipelineRuns, numberOfPipelineRuns, label, false, map[string]string{ + ".tekton/00-bad-apple.yaml": "testdata/failures/bad-runafter-task.yaml", + }) +} +func testGithubConcurrency(ctx context.Context, t *testing.T, maxNumberOfConcurrentPipelineRuns, numberOfPipelineRuns int, label string, checkOrdering bool, yamlFiles map[string]string) { + pipelineRunFileNamePrefix := "prlongrunnning-" targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns") - _, runcnx, opts, ghcnx, err := tgithub.Setup(ctx, false, false) + _, runcnx, opts, ghcnx, err := tgithub.Setup(ctx, true, false) assert.NilError(t, err) logmsg := fmt.Sprintf("Testing %s with Github APPS integration on %s", label, targetNS) @@ -45,12 +70,11 @@ func TestGithubPullRequestConcurrency(t *testing.T) { err = tgithub.CreateCRD(ctx, t, repoinfo, runcnx, opts, targetNS) assert.NilError(t, err) - yamlFiles := map[string]string{} for i := 1; i <= numberOfPipelineRuns; i++ { - yamlFiles[fmt.Sprintf(".tekton/prlongrunnning-%d.yaml", i)] = "testdata/pipelinerun_long_running.yaml" + yamlFiles[fmt.Sprintf(".tekton/%s%d.yaml", pipelineRunFileNamePrefix, i)] = "testdata/pipelinerun_long_running.yaml" } - entries, err := payload.GetEntries(yamlFiles, targetNS, options.MainBranch, options.PullRequestEvent, map[string]string{}) + entries, err := payload.GetEntries(yamlFiles, targetNS, options.MainBranch, "pull_request", map[string]string{}) assert.NilError(t, err) targetRefName := fmt.Sprintf("refs/heads/%s", @@ -79,7 +103,7 @@ func TestGithubPullRequestConcurrency(t *testing.T) { assert.NilError(t, wait.UntilMinPRAppeared(ctx, runcnx.Clients, waitOpts, numberOfPipelineRuns)) finished := false - maxLoop := 15 + maxLoop := 30 for i := 0; i < maxLoop; i++ { unsuccessful := 0 prs, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{}) @@ -101,11 +125,23 @@ func TestGithubPullRequestConcurrency(t *testing.T) { finished = true break } - runcnx.Clients.Log.Infof("number of unsuccessful PR %d out of %d, waiting 10s more, %d/%d", unsuccessful, numberOfPipelineRuns*2, i, maxLoop) + runcnx.Clients.Log.Infof("number of unsuccessful PR %d out of %d, waiting 10s more in the waiting loop: %d/%d", unsuccessful, numberOfPipelineRuns, i, maxLoop) // it's high because it takes time to process on kind - time.Sleep(15 * time.Second) + time.Sleep(10 * time.Second) } if !finished { t.Errorf("the %d pipelineruns has not successfully finished, some of them are still pending or it's abnormally slow to process the Q", numberOfPipelineRuns) } + + // sort all the PR by when they have started + if checkOrdering { + prs, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{}) + assert.NilError(t, err) + sort.PipelineRunSortByStartTime(prs.Items) + for i := 0; i < numberOfPipelineRuns; i++ { + prExpectedName := fmt.Sprintf("%s%d", pipelineRunFileNamePrefix, len(prs.Items)-i) + prActualName := prs.Items[i].GetName() + assert.Assert(t, strings.HasPrefix(prActualName, prExpectedName), "prActualName: %s does not start with expected prefix %s, was is ordered properly at start time", prActualName, prExpectedName) + } + } }