From 2d465fe57657b6ad04e409fa8c951f29c2e33c77 Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Mon, 19 Aug 2024 21:03:47 +0530 Subject: [PATCH] Fix PAC watcher panic when concurrency_limit is set This fixes a panic in PAC watcher when concurreny_limit is set via global repository and concurrency was not working as expected. E2E tests are also added. https://issues.redhat.com/browse/SRVKP-5560 Signed-off-by: Zaki Shaikh --- pkg/pipelineascode/pipelineascode.go | 7 ++ pkg/reconciler/queue_pipelineruns.go | 6 ++ test/github_pullrequest_concurrency_test.go | 76 ++++++++++++++++++++- 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index f5c4561e1..13bdee2fc 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -88,6 +88,13 @@ func (p *PacRun) Run(ctx context.Context) error { if match.Repo == nil { match.Repo = repo } + + // After matchRepo func fetched repo from k8s api repo is updated and + // need to merge global repo again + if p.globalRepo != nil { + match.Repo.Spec.Merge(p.globalRepo.Spec) + } + wg.Add(1) go func(match matcher.Match, i int) { diff --git a/pkg/reconciler/queue_pipelineruns.go b/pkg/reconciler/queue_pipelineruns.go index 83196c9fa..90252f18c 100644 --- a/pkg/reconciler/queue_pipelineruns.go +++ b/pkg/reconciler/queue_pipelineruns.go @@ -34,6 +34,12 @@ func (r *Reconciler) queuePipelineRun(ctx context.Context, logger *zap.SugaredLo return fmt.Errorf("updateError: %w", err) } + // merge local repo with global repo here in order to derive settings from global + // for further concurrency and other operations. + if r.globalRepo, err = r.repoLister.Repositories(r.run.Info.Kube.Namespace).Get(r.run.Info.Controller.GlobalRepository); err == nil && r.globalRepo != nil { + repo.Spec.Merge(r.globalRepo.Spec) + } + // if concurrency was set and later removed or changed to zero // then remove pipelineRun from Queue and update pending state to running if repo.Spec.ConcurrencyLimit != nil && *repo.Spec.ConcurrencyLimit == 0 { diff --git a/test/github_pullrequest_concurrency_test.go b/test/github_pullrequest_concurrency_test.go index e0de46ad9..c0077385a 100644 --- a/test/github_pullrequest_concurrency_test.go +++ b/test/github_pullrequest_concurrency_test.go @@ -7,15 +7,22 @@ import ( "context" "fmt" "net/http" + "os" "strings" "testing" "time" + "github.com/google/go-github/v61/github" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/pkg/sort" + "github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx" 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" + "github.com/openshift-pipelines/pipelines-as-code/test/pkg/repository" "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/pkg/names" @@ -49,6 +56,33 @@ func TestGithubSecondPullRequestConcurrency1by1WithError(t *testing.T) { }) } +func TestGithubGlobalRepoConcurrencyLimit(t *testing.T) { + label := "Github PullRequest Concurrent Two at a Time Set by Global Repo" + // in this test we don't set `concurrency_limit` in local repo as our goal + // here is to verify `concurrency_limit` for global repository. + localRepoMaxConcurrentRuns := -1 + testGlobalRepoConcurrency(t, label, localRepoMaxConcurrentRuns) +} + +func TestGithubGlobalAndLocalRepoConcurrencyLimit(t *testing.T) { + label := "Github PullRequest Concurrent Three at a Time Set by Local Repo" + testGlobalRepoConcurrency(t, label /* localRepoMaxConcurrentRuns */, 3) +} + +func testGlobalRepoConcurrency(t *testing.T, label string, localRepoMaxConcurrentRuns int) { + ctx := context.Background() + // create global repo + ctx, globalNS, runcnx, err := createGlobalRepo(ctx) + assert.NilError(t, err) + defer (func() { + err = cleanUpGlobalRepo(runcnx, globalNS) + assert.NilError(t, err) + })() + + numberOfPipelineRuns := 10 + testGithubConcurrency(ctx, t, localRepoMaxConcurrentRuns, numberOfPipelineRuns, label, false, map[string]string{}) +} + 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") @@ -65,7 +99,9 @@ func testGithubConcurrency(ctx context.Context, t *testing.T, maxNumberOfConcurr } // set concurrency - opts.Concurrency = maxNumberOfConcurrentPipelineRuns + if maxNumberOfConcurrentPipelineRuns >= 0 { + opts.Concurrency = maxNumberOfConcurrentPipelineRuns + } err = tgithub.CreateCRD(ctx, t, repoinfo, runcnx, opts, targetNS) assert.NilError(t, err) @@ -155,3 +191,41 @@ func testGithubConcurrency(ctx context.Context, t *testing.T, maxNumberOfConcurr } } } + +func createGlobalRepo(ctx context.Context) (context.Context, string, *params.Run, error) { + runcnx := params.New() + if err := runcnx.Clients.NewClients(ctx, &runcnx.Info); err != nil { + return ctx, "", nil, err + } + + ctx, err := cctx.GetControllerCtxInfo(ctx, runcnx) + if err != nil { + return ctx, "", nil, err + } + + globalNS := info.GetNS(ctx) + + repo := &v1alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: info.DefaultGlobalRepoName, + }, + Spec: v1alpha1.RepositorySpec{ + ConcurrencyLimit: github.Int(2), + }, + } + + if err := repository.CreateRepo(ctx, globalNS, runcnx, repo); err != nil { + return ctx, "", nil, err + } + + return ctx, globalNS, runcnx, nil +} + +func cleanUpGlobalRepo(runcnx *params.Run, globalNS string) error { + if os.Getenv("TEST_NOCLEANUP") != "true" { + runcnx.Clients.Log.Infof("Cleaning up global repo %s in %s", info.DefaultGlobalRepoName, globalNS) + return runcnx.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(globalNS).Delete( + context.Background(), info.DefaultGlobalRepoName, metav1.DeleteOptions{}) + } + return nil +}