From 2a4e355daeb195aef7cf6a1eceb264dfc660bf24 Mon Sep 17 00:00:00 2001 From: John Niang Date: Sun, 5 Dec 2021 21:43:48 +0800 Subject: [PATCH] Replace label of SCM ref name with field indexer Signed-off-by: John Niang --- cmd/controller/app/server.go | 5 ++++ .../jenkins/pipeline/metadata_converter.go | 1 + .../pipelinerun/pipelinerun_controller.go | 16 +++--------- .../pipelinerun_controller_test.go | 5 ++-- .../pipelinerun_synchronizer_test.go | 1 - go.mod | 2 +- go.sum | 4 +-- pkg/api/devops/v1alpha3/groupversion_info.go | 4 +-- pkg/apiserver/apiserver.go | 5 +++- pkg/indexers/indexers.go | 26 +++++++++++++++++++ .../devops/v1alpha3/pipeline/branch_filter.go | 4 --- .../v1alpha3/pipeline/branch_filter_test.go | 12 +++++++-- .../devops/v1alpha3/pipelinerun/handler.go | 13 +++++++--- pkg/kapis/devops/v1alpha3/pipelinerun/util.go | 18 +------------ .../devops/v1alpha3/pipelinerun/util_test.go | 20 +++----------- pkg/models/pipeline/pipeline.go | 5 +++- 16 files changed, 75 insertions(+), 66 deletions(-) create mode 100644 pkg/indexers/indexers.go diff --git a/cmd/controller/app/server.go b/cmd/controller/app/server.go index ec48df3a..36e6d8d1 100644 --- a/cmd/controller/app/server.go +++ b/cmd/controller/app/server.go @@ -28,6 +28,7 @@ import ( "kubesphere.io/devops/pkg/client/k8s" "kubesphere.io/devops/pkg/client/s3" "kubesphere.io/devops/pkg/config" + "kubesphere.io/devops/pkg/indexers" "kubesphere.io/devops/pkg/informers" "sigs.k8s.io/controller-runtime/pkg/runtime/signals" @@ -189,6 +190,10 @@ func Run(s *options.DevOpsControllerManagerOptions, stopCh <-chan struct{}) erro return fmt.Errorf("unable to register controllers to the manager: %v", err) } + if err := indexers.CreatePipelineRunSCMRefNameIndexer(mgr.GetCache()); err != nil { + return err + } + // Start cache data after all informer is registered klog.V(0).Info("Starting cache resource from apiserver...") informerFactory.Start(stopCh) diff --git a/controllers/jenkins/pipeline/metadata_converter.go b/controllers/jenkins/pipeline/metadata_converter.go index 293f75a5..3a76cc68 100644 --- a/controllers/jenkins/pipeline/metadata_converter.go +++ b/controllers/jenkins/pipeline/metadata_converter.go @@ -75,6 +75,7 @@ func convertBranches(jobBranches []job.PipelineBranch) []pipeline.Branch { for _, jobBranch := range jobBranches { branches = append(branches, pipeline.Branch{ Name: jobBranch.Name, + RawName: jobBranch.DisplayName, WeatherScore: jobBranch.WeatherScore, Branch: jobBranch.Branch, PullRequest: jobBranch.PullRequest, diff --git a/controllers/jenkins/pipelinerun/pipelinerun_controller.go b/controllers/jenkins/pipelinerun/pipelinerun_controller.go index cabf7d6a..49973d18 100644 --- a/controllers/jenkins/pipelinerun/pipelinerun_controller.go +++ b/controllers/jenkins/pipelinerun/pipelinerun_controller.go @@ -21,7 +21,6 @@ import ( "encoding/json" "fmt" "reflect" - "strings" "time" "github.com/go-logr/logr" @@ -30,7 +29,6 @@ import ( corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/retry" @@ -119,9 +117,6 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { pipelineRunCopied.Labels = make(map[string]string) } pipelineRunCopied.Labels[v1alpha3.PipelineNameLabelKey] = pipelineName - if refName, err := getSCMRefName(&pipelineRunCopied.Spec); err == nil && refName != "" { - pipelineRunCopied.Labels[v1alpha3.SCMRefNameLabelKey] = refName - } log = log.WithValues("namespace", namespaceName, "Pipeline", pipelineName) @@ -141,7 +136,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { // See also: https://book-v1.book.kubebuilder.io/basics/status_subresource.html if err := r.updateStatus(ctx, status, req.NamespacedName); err != nil { log.Error(err, "unable to update PipelineRun status.") - return ctrl.Result{RequeueAfter: time.Second}, err + return ctrl.Result{}, err } } @@ -169,7 +164,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { // update labels and annotations if err := r.updateLabelsAndAnnotations(ctx, pipelineRunCopied); err != nil { log.Error(err, "unable to update PipelineRun labels and annotations.") - return ctrl.Result{RequeueAfter: time.Second}, err + return ctrl.Result{}, err } r.recorder.Eventf(pipelineRunCopied, corev1.EventTypeNormal, v1alpha3.Updated, "Updated running data for PipelineRun %s", req.NamespacedName) @@ -231,7 +226,7 @@ func (r *Reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { } r.recorder.Eventf(pipelineRunCopied, corev1.EventTypeNormal, v1alpha3.Started, "Started PipelineRun %s", req.NamespacedName) // requeue after 1 second - return ctrl.Result{RequeueAfter: 1 * time.Second}, nil + return ctrl.Result{}, nil } func (r *Reconciler) hasSamePipelineRun(jobRun *job.PipelineRun, pipeline *v1alpha3.Pipeline) (exists bool, err error) { @@ -243,7 +238,7 @@ func (r *Reconciler) hasSamePipelineRun(jobRun *job.PipelineRun, pipeline *v1alp } if pipeline.Spec.Type == v1alpha3.MultiBranchPipelineType { // add SCM reference name into list options for multi-branch Pipeline - listOptions = append(listOptions, client.MatchingLabels{v1alpha3.SCMRefNameLabelKey: jobRun.Pipeline}) + listOptions = append(listOptions, client.MatchingFields{v1alpha3.PipelineRunSCMRefNameField: jobRun.Pipeline}) } if err = r.Client.List(context.Background(), pipelineRuns, listOptions...); err == nil { isMultiBranch := pipeline.Spec.Type == v1alpha3.MultiBranchPipelineType @@ -259,9 +254,6 @@ func getSCMRefName(prSpec *v1alpha3.PipelineRunSpec) (string, error) { if prSpec.SCM == nil || prSpec.SCM.RefName == "" { return "", fmt.Errorf("failed to obtain SCM reference name for multi-branch Pipeline") } - if errs := validation.IsValidLabelValue(prSpec.SCM.RefName); len(errs) != 0 { - return "", fmt.Errorf(strings.Join(errs, "; ")) - } branch = prSpec.SCM.RefName } return branch, nil diff --git a/controllers/jenkins/pipelinerun/pipelinerun_controller_test.go b/controllers/jenkins/pipelinerun/pipelinerun_controller_test.go index 633fe079..0a0fe12d 100644 --- a/controllers/jenkins/pipelinerun/pipelinerun_controller_test.go +++ b/controllers/jenkins/pipelinerun/pipelinerun_controller_test.go @@ -82,7 +82,7 @@ func Test_getBranch(t *testing.T) { }, want: "main", }, { - name: "Multi-branch Pipeline and SCM set, but the name is invalid", + name: "Multi-branch Pipeline and SCM set, but the name is written in Chinese", args: args{ prSpec: &v1alpha3.PipelineRunSpec{ PipelineSpec: &v1alpha3.PipelineSpec{ @@ -94,7 +94,7 @@ func Test_getBranch(t *testing.T) { }, }, }, - wantErr: true, + want: "测试分支", }, } for _, tt := range tests { @@ -159,7 +159,6 @@ var _ = Describe("TestReconciler_hasSamePipelineRun", func() { v1alpha3.JenkinsPipelineRunIDAnnoKey: "123", }, Labels: map[string]string{ - v1alpha3.SCMRefNameLabelKey: "main", v1alpha3.PipelineNameLabelKey: "multi-branch-pipeline", }, }, diff --git a/controllers/jenkins/pipelinerun/pipelinerun_synchronizer_test.go b/controllers/jenkins/pipelinerun/pipelinerun_synchronizer_test.go index 17065c0f..5fa35c1e 100644 --- a/controllers/jenkins/pipelinerun/pipelinerun_synchronizer_test.go +++ b/controllers/jenkins/pipelinerun/pipelinerun_synchronizer_test.go @@ -143,7 +143,6 @@ func Test_createBarePipelineRun(t *testing.T) { }, Labels: map[string]string{ v1alpha3.PipelineNameLabelKey: "fake-pipeline", - v1alpha3.SCMRefNameLabelKey: "main", }, }, Spec: v1alpha3.PipelineRunSpec{ diff --git a/go.mod b/go.mod index a5f2361d..9729f338 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,7 @@ require ( github.com/golang/mock v1.6.0 github.com/google/go-cmp v0.5.5 github.com/google/go-querystring v1.1.0 // indirect - github.com/jenkins-zh/jenkins-client v0.0.6 + github.com/jenkins-zh/jenkins-client v0.0.7 github.com/kubesphere/sonargo v0.0.2 github.com/onsi/ginkgo v1.16.4 github.com/onsi/gomega v1.15.0 diff --git a/go.sum b/go.sum index 45c0f0ac..858bb9e2 100644 --- a/go.sum +++ b/go.sum @@ -317,8 +317,8 @@ github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NH github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8= github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99/go.mod h1:1lJo3i6rXxKeerYnT8Nvf0QmHCRC1n8sfWVwXF2Frvo= github.com/jenkins-zh/jenkins-cli v0.0.32/go.mod h1:uE1mH9PNITrg0sugv6HXuM/CSddg0zxXoYu3w57I3JY= -github.com/jenkins-zh/jenkins-client v0.0.6 h1:Rs10HIXBP6evnmW+zeASJye/VSwrtT1bgXj+IC+oXXk= -github.com/jenkins-zh/jenkins-client v0.0.6/go.mod h1:ICBk7OOoTafVP//f/VfKZ34c0ff8vJwVnOsF9btiMYU= +github.com/jenkins-zh/jenkins-client v0.0.7 h1:E0EUOx1G3RVEUrvj8WGvI5WjDTRD4tcLm8FAg0iqJJ8= +github.com/jenkins-zh/jenkins-client v0.0.7/go.mod h1:ICBk7OOoTafVP//f/VfKZ34c0ff8vJwVnOsF9btiMYU= github.com/jenkins-zh/jenkins-formulas v0.0.5/go.mod h1:zS8fm8u5L6FcjZM0QznXsLV9T2UtSVK+hT6Sm76iUZ4= github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI= github.com/jessevdk/go-flags v1.5.0/go.mod h1:Fw0T6WPc1dYxT4mKEZRfG5kJhaTDP9pj1c2EWnYs/m4= diff --git a/pkg/api/devops/v1alpha3/groupversion_info.go b/pkg/api/devops/v1alpha3/groupversion_info.go index 43361878..f8db9397 100644 --- a/pkg/api/devops/v1alpha3/groupversion_info.go +++ b/pkg/api/devops/v1alpha3/groupversion_info.go @@ -35,10 +35,10 @@ const ( PipelineRunOrphanLabelKey = GroupName + "/jenkins-pipelinerun-orphan" // PipelineNameLabelKey is label key of Pipeline name. PipelineNameLabelKey = GroupName + "/pipeline" - // SCMRefNameLabelKey is label key of SCM reference name. - SCMRefNameLabelKey = GroupName + "/scm-ref-name" // PipelineRunCreatorAnnoKey is annotation key of PipelineRun's creator PipelineRunCreatorAnnoKey = GroupName + "/creator" + // PipelineRunSCMRefNameField is the field name of SCM reference name in PipelineRun spec. + PipelineRunSCMRefNameField = "spec.scm.ref-name" ) var ( diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index 30bf800b..029eb117 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -34,6 +34,7 @@ import ( "kubesphere.io/devops/pkg/apiserver/authentication/request/anonymous" "kubesphere.io/devops/pkg/apiserver/filters" "kubesphere.io/devops/pkg/apiserver/request" + "kubesphere.io/devops/pkg/indexers" "kubesphere.io/devops/pkg/kapis/oauth" "kubesphere.io/devops/pkg/models/auth" "sigs.k8s.io/controller-runtime/pkg/client" @@ -154,7 +155,9 @@ func (s *APIServer) installKubeSphereAPIs() { } func (s *APIServer) Run(stopCh <-chan struct{}) (err error) { - + if err := indexers.CreatePipelineRunSCMRefNameIndexer(s.RuntimeCache); err != nil { + return err + } err = s.waitForResourceSync(stopCh) if err != nil { return err diff --git a/pkg/indexers/indexers.go b/pkg/indexers/indexers.go new file mode 100644 index 00000000..b1ff6e62 --- /dev/null +++ b/pkg/indexers/indexers.go @@ -0,0 +1,26 @@ +package indexers + +import ( + "context" + + "k8s.io/apimachinery/pkg/runtime" + "kubesphere.io/devops/pkg/api/devops/v1alpha3" + "sigs.k8s.io/controller-runtime/pkg/cache" +) + +// CreatePipelineRunSCMRefNameIndexer creates field indexer which could speed up listing PipelineRun by SCM reference name. +func CreatePipelineRunSCMRefNameIndexer(runtimeCache cache.Cache) error { + return runtimeCache.IndexField(context.Background(), + &v1alpha3.PipelineRun{}, + v1alpha3.PipelineRunSCMRefNameField, + func(o runtime.Object) []string { + pipelineRun, ok := o.(*v1alpha3.PipelineRun) + if !ok || pipelineRun == nil { + return []string{} + } + if pipelineRun.Spec.SCM == nil { + return []string{} + } + return []string{pipelineRun.Spec.SCM.RefName} + }) +} diff --git a/pkg/kapis/devops/v1alpha3/pipeline/branch_filter.go b/pkg/kapis/devops/v1alpha3/pipeline/branch_filter.go index ce70d426..50b3317a 100644 --- a/pkg/kapis/devops/v1alpha3/pipeline/branch_filter.go +++ b/pkg/kapis/devops/v1alpha3/pipeline/branch_filter.go @@ -2,7 +2,6 @@ package pipeline import ( "github.com/jenkins-zh/jenkins-client/pkg/job" - "k8s.io/apimachinery/pkg/util/validation" "kubesphere.io/devops/pkg/models/pipeline" ) @@ -13,9 +12,6 @@ type branchSlice []pipeline.Branch func (branches branchSlice) filter(predicate branchPredicate) []pipeline.Branch { var resultBranches []pipeline.Branch for _, branch := range branches { - if errors := validation.IsValidLabelValue(branch.Name); len(errors) != 0 { - continue - } if predicate != nil && predicate(branch) { resultBranches = append(resultBranches, branch) } diff --git a/pkg/kapis/devops/v1alpha3/pipeline/branch_filter_test.go b/pkg/kapis/devops/v1alpha3/pipeline/branch_filter_test.go index a5676b4b..8f2b6bc6 100644 --- a/pkg/kapis/devops/v1alpha3/pipeline/branch_filter_test.go +++ b/pkg/kapis/devops/v1alpha3/pipeline/branch_filter_test.go @@ -64,7 +64,7 @@ func Test_filterBranches(t *testing.T) { PullRequest: &job.PullRequest{}, }}, }, { - name: "With filter: origin, but name is invalid", + name: "With filter: origin, but name is written in Chinese", args: args{ branches: []pipeline.Branch{{ Name: "main1", @@ -83,6 +83,9 @@ func Test_filterBranches(t *testing.T) { want: []pipeline.Branch{{ Name: "main1", PullRequest: nil, + }, { + Name: "主分支2", + PullRequest: &job.PullRequest{}, }}, }, { name: "With filter: pull-requests", @@ -108,7 +111,7 @@ func Test_filterBranches(t *testing.T) { }, }}, }, { - name: "With filter: pull-requests， but name is invalid", + name: "With filter: pull-requests, but name is written in Chinese", args: args{ branches: []pipeline.Branch{{ Name: "main1", @@ -134,6 +137,11 @@ func Test_filterBranches(t *testing.T) { PullRequest: &job.PullRequest{ ID: "1", }, + }, { + Name: "分支2", + PullRequest: &job.PullRequest{ + ID: "2", + }, }}, }, { name: "With other filter", diff --git a/pkg/kapis/devops/v1alpha3/pipelinerun/handler.go b/pkg/kapis/devops/v1alpha3/pipelinerun/handler.go index 1cc72352..02c7275d 100644 --- a/pkg/kapis/devops/v1alpha3/pipelinerun/handler.go +++ b/pkg/kapis/devops/v1alpha3/pipelinerun/handler.go @@ -56,17 +56,22 @@ func (h *apiHandler) listPipelineRuns(request *restful.Request, response *restfu } // build label selector - labelSelector, err := buildLabelSelector(queryParam, pipeline.Name, branchName) + labelSelector, err := buildLabelSelector(queryParam, pipeline.Name) if err != nil { api.HandleError(request, response, err) return } + opts := make([]client.ListOption, 0, 3) + opts = append(opts, client.InNamespace(pipeline.Namespace)) + opts = append(opts, client.MatchingLabelsSelector{Selector: labelSelector}) + if branchName != "" { + opts = append(opts, client.MatchingFields{v1alpha3.PipelineRunSCMRefNameField: branchName}) + } + var prs v1alpha3.PipelineRunList // fetch PipelineRuns - if err := h.client.List(context.Background(), &prs, - client.InNamespace(pipeline.Namespace), - client.MatchingLabelsSelector{Selector: labelSelector}); err != nil { + if err := h.client.List(context.Background(), &prs, opts...); err != nil { api.HandleError(request, response, err) return } diff --git a/pkg/kapis/devops/v1alpha3/pipelinerun/util.go b/pkg/kapis/devops/v1alpha3/pipelinerun/util.go index 689432f8..6165fd6a 100644 --- a/pkg/kapis/devops/v1alpha3/pipelinerun/util.go +++ b/pkg/kapis/devops/v1alpha3/pipelinerun/util.go @@ -3,20 +3,18 @@ package pipelinerun import ( "errors" "fmt" - "strings" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/selection" - "k8s.io/apimachinery/pkg/util/validation" "kubesphere.io/devops/pkg/api/devops/v1alpha3" "kubesphere.io/devops/pkg/apiserver/query" "kubesphere.io/devops/pkg/client/devops" ) -func buildLabelSelector(queryParam *query.Query, pipelineName, branchName string) (labels.Selector, error) { +func buildLabelSelector(queryParam *query.Query, pipelineName string) (labels.Selector, error) { labelSelector := queryParam.Selector() rq, err := labels.NewRequirement(v1alpha3.PipelineNameLabelKey, selection.Equals, []string{pipelineName}) if err != nil { @@ -24,17 +22,6 @@ func buildLabelSelector(queryParam *query.Query, pipelineName, branchName string return nil, err } labelSelector = labelSelector.Add(*rq) - if branchName != "" { - if errs := validation.IsValidLabelValue(branchName); len(errs) != 0 { - return nil, fmt.Errorf(strings.Join(errs, "; ")) - } - rq, err = labels.NewRequirement(v1alpha3.SCMRefNameLabelKey, selection.Equals, []string{branchName}) - if err != nil { - // should never happen - return nil, err - } - labelSelector = labelSelector.Add(*rq) - } return labelSelector, nil } @@ -109,8 +96,5 @@ func CreatePipelineRun(pipeline *v1alpha3.Pipeline, payload *devops.RunPayload, SCM: scm, }, } - if scm != nil && scm.RefName != "" { - pipelineRun.Labels[v1alpha3.SCMRefNameLabelKey] = scm.RefName - } return pipelineRun } diff --git a/pkg/kapis/devops/v1alpha3/pipelinerun/util_test.go b/pkg/kapis/devops/v1alpha3/pipelinerun/util_test.go index 20b39e2e..2f6b5de5 100644 --- a/pkg/kapis/devops/v1alpha3/pipelinerun/util_test.go +++ b/pkg/kapis/devops/v1alpha3/pipelinerun/util_test.go @@ -25,7 +25,6 @@ func Test_buildLabelSelector(t *testing.T) { type args struct { queryParam *query.Query pipelineName string - branchName string } tests := []struct { name string @@ -37,9 +36,8 @@ func Test_buildLabelSelector(t *testing.T) { args: args{ queryParam: &query.Query{}, pipelineName: "pipelineA", - branchName: "branchA", }, - want: parseSelector(fmt.Sprintf("%s=pipelineA,%s=branchA", v1alpha3.PipelineNameLabelKey, v1alpha3.SCMRefNameLabelKey)), + want: parseSelector(fmt.Sprintf("%s=pipelineA", v1alpha3.PipelineNameLabelKey)), }, { name: "Label selector was provided", args: args{ @@ -47,22 +45,12 @@ func Test_buildLabelSelector(t *testing.T) { LabelSelector: "a=b", }, pipelineName: "pipelineA", - branchName: "branchA", }, - want: parseSelector(fmt.Sprintf("%s=pipelineA,%s=branchA,a=b", v1alpha3.PipelineNameLabelKey, v1alpha3.SCMRefNameLabelKey)), - }, { - name: "No label selector was provided", - args: args{ - queryParam: &query.Query{}, - pipelineName: "pipelineA", - branchName: "分支A", - }, - wantErr: true, - }, - } + want: parseSelector(fmt.Sprintf("%s=pipelineA,a=b", v1alpha3.PipelineNameLabelKey)), + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := buildLabelSelector(tt.args.queryParam, tt.args.pipelineName, tt.args.branchName) + got, err := buildLabelSelector(tt.args.queryParam, tt.args.pipelineName) if (err != nil) != tt.wantErr { t.Errorf("buildLabelSelector() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/pkg/models/pipeline/pipeline.go b/pkg/models/pipeline/pipeline.go index a6d1d5cb..7e347382 100644 --- a/pkg/models/pipeline/pipeline.go +++ b/pkg/models/pipeline/pipeline.go @@ -25,7 +25,10 @@ type Metadata struct { // Branch contains branch metadata, like branch and pull request, and latest PipelineRun. type Branch struct { - Name string `json:"name,omitempty"` + // Name is branch name, like "feat%2FfeatureA" + Name string `json:"name,omitempty"` + // RawName is the display branch name, like "feat/featureA" + RawName string `json:"rawName,omitempty"` WeatherScore int `json:"weatherScore"` Disabled bool `json:"disabled,omitempty"` LatestRun *LatestRun `json:"latestRun,omitempty"`