Repository: kubesphere/ks-devops
Pull request #907 (2023-02-15 02:42:05)

[release-3.4] add retry for pipelinerun annotation update
This is an automated cherry-pick of #906

```release-note
Add retry when update pipelinerun annotation
```

ks-ci-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **zheng1** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @zheng1` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubesphere%2Fks-devops).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubesphere/ks-devops/blob/release-3.4/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["zheng1"]} -->

Patch:
diff --git a/controllers/jenkins/pipeline/json_converter.go b/controllers/jenkins/pipeline/json_converter.go
index 32f0294..e027334 100644
--- a/controllers/jenkins/pipeline/json_converter.go
+++ b/controllers/jenkins/pipeline/json_converter.go
@@ -19,16 +19,21 @@ package pipeline
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"time"
 
 	"github.com/go-logr/logr"
 	"github.com/jenkins-zh/jenkins-client/pkg/core"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/client-go/util/retry"
+
 	v1alpha3 "kubesphere.io/devops/pkg/api/devops/v1alpha3"
 	"kubesphere.io/devops/pkg/jwt/token"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 )
 
 // tokenExpireIn indicates that the temporary token issued by controller will be expired in some time.
@@ -73,10 +78,16 @@ func (r *JenkinsfileReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	editMode := pip.Annotations[v1alpha3.PipelineJenkinsfileEditModeAnnoKey]
 	switch editMode {
 	case v1alpha3.PipelineJenkinsfileEditModeRaw:
-		result, err = r.reconcileJenkinsfileEditMode(pip, coreClient)
+		result, err = r.reconcileJenkinsfileEditMode(pip, req.NamespacedName, coreClient)
 	case v1alpha3.PipelineJenkinsfileEditModeJSON:
 		result, err = r.reconcileJSONEditMode(pip, coreClient)
 	case "":
+		// Reconcile pipeline version <= v3.3.2
+		if _, ok := pip.Annotations[v1alpha3.PipelineJenkinsfileValueAnnoKey]; !ok {
+			if pip.Spec.Pipeline != nil && pip.Spec.Pipeline.Jenkinsfile != "" {
+				result, err = r.reconcileJenkinsfileEditMode(pip, req.NamespacedName, coreClient)
+			}
+		}
 	default:
 		r.log.Info(fmt.Sprintf("invalid edit mode: %s", editMode))
 		return
@@ -84,25 +95,53 @@ func (r *JenkinsfileReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	return
 }
 
-func (r *JenkinsfileReconciler) reconcileJenkinsfileEditMode(pip *v1alpha3.Pipeline, coreClient core.Client) (
+func (r *JenkinsfileReconciler) reconcileJenkinsfileEditMode(pip *v1alpha3.Pipeline, pipelineKey client.ObjectKey, coreClient core.Client) (
 	result ctrl.Result, err error) {
 	jenkinsfile := pip.Spec.Pipeline.Jenkinsfile
-
-	var toJSONResult core.GenericResult
-	if toJSONResult, err = coreClient.ToJSON(jenkinsfile); err != nil || toJSONResult.GetStatus() != "success" {
-		err = fmt.Errorf("failed to convert Jenkinsfile to JSON format, error: %v", err)
-		return
-	}
-
+	toJsonJenkinsfile := ""
 	if pip.Annotations == nil {
 		pip.Annotations = map[string]string{}
 	}
-	pip.Annotations[v1alpha3.PipelineJenkinsfileValueAnnoKey] = toJSONResult.GetResult()
+
+	// Users are able to clean jenkinsfile
+	if jenkinsfile != "" {
+		var toJSONResult core.GenericResult
+		if toJSONResult, err = coreClient.ToJSON(jenkinsfile); err != nil || toJSONResult.GetStatus() != "success" {
+			r.log.Error(err, "failed to convert jenkinsfile to json format")
+			pip.Annotations[v1alpha3.PipelineJenkinsfileValueAnnoKey] = ""
+			pip.Annotations[v1alpha3.PipelineJenkinsfileEditModeAnnoKey] = ""
+			pip.Annotations[v1alpha3.PipelineJenkinsfileValidateAnnoKey] = v1alpha3.PipelineJenkinsfileValidateFailure
+			err = r.updateAnnotations(pip.Annotations, pipelineKey)
+			return
+		}
+		toJsonJenkinsfile = toJSONResult.GetResult()
+	}
+
+	pip.Annotations[v1alpha3.PipelineJenkinsfileValueAnnoKey] = toJsonJenkinsfile
 	pip.Annotations[v1alpha3.PipelineJenkinsfileEditModeAnnoKey] = ""
-	err = r.Update(context.Background(), pip)
+	pip.Annotations[v1alpha3.PipelineJenkinsfileValidateAnnoKey] = v1alpha3.PipelineJenkinsfileValidateSuccess
+	err = r.updateAnnotations(pip.Annotations, pipelineKey)
 	return
 }
 
+func (r *JenkinsfileReconciler) updateAnnotations(annotations map[string]string, pipelineKey client.ObjectKey) error {
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		pipeline := &v1alpha3.Pipeline{}
+		if err := r.Get(context.Background(), pipelineKey, pipeline); err != nil {
+			return client.IgnoreNotFound(err)
+		}
+		if reflect.DeepEqual(pipeline.Annotations, annotations) {
+			return nil
+		}
+
+		// update annotations
+		pipeline.Annotations[v1alpha3.PipelineJenkinsfileValueAnnoKey] = annotations[v1alpha3.PipelineJenkinsfileValueAnnoKey]
+		pipeline.Annotations[v1alpha3.PipelineJenkinsfileEditModeAnnoKey] = annotations[v1alpha3.PipelineJenkinsfileEditModeAnnoKey]
+		pipeline.Annotations[v1alpha3.PipelineJenkinsfileValidateAnnoKey] = annotations[v1alpha3.PipelineJenkinsfileValidateAnnoKey]
+		return r.Update(context.Background(), pipeline)
+	})
+}
+
 func (r *JenkinsfileReconciler) reconcileJSONEditMode(pip *v1alpha3.Pipeline, coreClient core.Client) (
 	result ctrl.Result, err error) {
 	var jsonData string
@@ -148,11 +187,29 @@ func (r *JenkinsfileReconciler) getOrCreateJenkinsCore(annotations map[string]st
 	return jenkinsCore, nil
 }
 
+// jenkinsfilePredicate returns a predicate only care about pipeline update event..
+var jenkinsfilePredicate = predicate.Funcs{
+	UpdateFunc: func(ue event.UpdateEvent) bool {
+		oldPipeline, okOld := ue.ObjectOld.(*v1alpha3.Pipeline)
+		newPipeline, okNew := ue.ObjectNew.(*v1alpha3.Pipeline)
+		if okOld && okNew {
+			if !reflect.DeepEqual(oldPipeline.Spec.Pipeline, newPipeline.Spec.Pipeline) {
+				return true
+			}
+		}
+		return false
+	},
+	GenericFunc: func(ge event.GenericEvent) bool {
+		return false
+	},
+}
+
 // SetupWithManager setups the log and recorder
 func (r *JenkinsfileReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	r.log = ctrl.Log.WithName(r.GetName())
 	r.recorder = mgr.GetEventRecorderFor(r.GetName())
 	return ctrl.NewControllerManagedBy(mgr).
+		WithEventFilter(jenkinsfilePredicate).
 		For(&v1alpha3.Pipeline{}).
 		Complete(r)
 }
diff --git a/controllers/jenkins/pipeline/pipeline_controller.go b/controllers/jenkins/pipeline/pipeline_controller.go
index 69fef25..7fc48e3 100644
--- a/controllers/jenkins/pipeline/pipeline_controller.go
+++ b/controllers/jenkins/pipeline/pipeline_controller.go
@@ -39,6 +39,7 @@ import (
 	corev1lister "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/client-go/util/retry"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
 
@@ -312,11 +313,36 @@ func (c *Controller) syncHandler(key string) error {
 	}
 
 	if !reflect.DeepEqual(pipeline, copyPipeline) {
-		_, err = c.kubesphereClient.DevopsV1alpha3().Pipelines(nsName).Update(context.Background(), copyPipeline, metav1.UpdateOptions{})
+		err = c.updatePipeline(context.Background(), name, nsName, copyPipeline)
 		if err != nil {
-			klog.V(8).Info(err, fmt.Sprintf("failed to update pipeline %s ", key))
+			klog.Error(err, fmt.Sprintf("failed to update pipeline %s ", key))
 			return err
 		}
+		klog.Infof("update pipeline %s:%s successful", nsName, name)
 	}
 	return nil
 }
+
+// Update with retry, if update failed, get new version and update again
+func (c *Controller) updatePipeline(ctx context.Context, name string, nsName string, pipeline *devopsv1alpha3.Pipeline) (err error) {
+	return retry.RetryOnConflict(retry.DefaultRetry, func() (err error) {
+		newPipeline, err := c.kubesphereClient.DevopsV1alpha3().Pipelines(nsName).Get(ctx, name, metav1.GetOptions{})
+		if err != nil {
+			return
+		}
+
+		if newPipeline.Annotations[devopsv1alpha3.PipelineSyncStatusAnnoKey] == pipeline.Annotations[devopsv1alpha3.PipelineSyncStatusAnnoKey] &&
+			newPipeline.Annotations[devopsv1alpha3.PipelineSpecHash] == pipeline.Annotations[devopsv1alpha3.PipelineSpecHash] &&
+			reflect.DeepEqual(newPipeline.ObjectMeta.Finalizers, pipeline.ObjectMeta.Finalizers) {
+			return nil
+		}
+		if pipeline.Annotations != nil {
+			// update annotations
+			newPipeline.Annotations[devopsv1alpha3.PipelineSyncStatusAnnoKey] = pipeline.Annotations[devopsv1alpha3.PipelineSyncStatusAnnoKey]
+			newPipeline.Annotations[devopsv1alpha3.PipelineSpecHash] = pipeline.Annotations[devopsv1alpha3.PipelineSpecHash]
+		}
+		newPipeline.ObjectMeta.Finalizers = pipeline.ObjectMeta.Finalizers
+		_, err = c.kubesphereClient.DevopsV1alpha3().Pipelines(nsName).Update(ctx, newPipeline, metav1.UpdateOptions{})
+		return err
+	})
+}
diff --git a/controllers/jenkins/pipeline/pipeline_metadata_controller.go b/controllers/jenkins/pipeline/pipeline_metadata_controller.go
index d60adf6..43eac4a 100644
--- a/controllers/jenkins/pipeline/pipeline_metadata_controller.go
+++ b/controllers/jenkins/pipeline/pipeline_metadata_controller.go
@@ -162,7 +162,7 @@ func (r *Reconciler) updateAnnotations(annotations map[string]string, pipelineKe
 	})
 }
 
-// pipelineMetadataPredicate returns a predicate which only cares about CreateEvent.
+// pipelineMetadataPredicate returns a predicate.
 var pipelineMetadataPredicate = predicate.Funcs{
 	CreateFunc: func(ce event.CreateEvent) bool {
 		return true
diff --git a/controllers/jenkins/pipelinerun/pipelinerun_controller.go b/controllers/jenkins/pipelinerun/pipelinerun_controller.go
index 31d51e6..4b9fe84 100644
--- a/controllers/jenkins/pipelinerun/pipelinerun_controller.go
+++ b/controllers/jenkins/pipelinerun/pipelinerun_controller.go
@@ -133,7 +133,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		if err != nil {
 			if err.Error() == BuildNotExistMsg { // delete pipelinerun if build not exist in jenkins
 				runID, _ := pipelineRun.GetPipelineRunID()
-				log.Info(fmt.Sprintf("the build(ID: %s) not exist in jenkins, delete it..", runID) )
+				log.Info(fmt.Sprintf("the build(ID: %s) not exist in jenkins, delete it..", runID))
 				if err = r.Client.Delete(ctx, pipelineRun); err != nil {
 					log.Error(err, "failed to delete pipelinerun")
 					return ctrl.Result{RequeueAfter: 3 * time.Second}, err
@@ -311,19 +311,24 @@ func getSCMRefName(prSpec *v1alpha3.PipelineRunSpec) (string, error) {
 }
 
 func (r *Reconciler) updateLabelsAndAnnotations(ctx context.Context, pr *v1alpha3.PipelineRun) (err error) {
-	// get pipeline
-	prToUpdate := v1alpha3.PipelineRun{}
-	if err = r.Get(ctx, client.ObjectKey{Namespace: pr.Namespace, Name: pr.Name}, &prToUpdate); err == nil {
-		if !reflect.DeepEqual(pr.Labels, prToUpdate.Labels) || !reflect.DeepEqual(pr.Annotations, prToUpdate.Annotations) {
-			prToUpdate = *prToUpdate.DeepCopy()
-			prToUpdate.Labels = pr.Labels
-			prToUpdate.Annotations = pr.Annotations
-			// make sure all PipelineRuns have the finalizer
-			k8sutil.AddFinalizer(&prToUpdate.ObjectMeta, v1alpha3.PipelineRunFinalizerName)
-			err = r.Update(ctx, &prToUpdate)
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		// get pipeline
+		prToUpdate := v1alpha3.PipelineRun{}
+		err = r.Get(ctx, client.ObjectKey{Namespace: pr.Namespace, Name: pr.Name}, &prToUpdate)
+		if err != nil {
+			return err
 		}
-	}
-	return
+		if reflect.DeepEqual(pr.Labels, prToUpdate.Labels) && reflect.DeepEqual(pr.Annotations, prToUpdate.Annotations) {
+			return nil
+		}
+
+		prToUpdate = *prToUpdate.DeepCopy()
+		prToUpdate.Labels = pr.Labels
+		prToUpdate.Annotations = pr.Annotations
+		// make sure all PipelineRuns have the finalizer
+		k8sutil.AddFinalizer(&prToUpdate.ObjectMeta, v1alpha3.PipelineRunFinalizerName)
+		return r.Update(ctx, &prToUpdate)
+	})
 }
 
 func (r *Reconciler) updateStatus(ctx context.Context, desiredStatus *v1alpha3.PipelineRunStatus, prKey client.ObjectKey) error {
diff --git a/pkg/api/devops/v1alpha3/pipeline_types.go b/pkg/api/devops/v1alpha3/pipeline_types.go
index d9b4662..04a819b 100644
--- a/pkg/api/devops/v1alpha3/pipeline_types.go
+++ b/pkg/api/devops/v1alpha3/pipeline_types.go
@@ -45,11 +45,18 @@ const (
 	PipelineJenkinsfileValueAnnoKey = PipelinePrefix + "jenkinsfile"
 	// PipelineJenkinsfileEditModeAnnoKey is the annotation key of the Jenkinsfile edit mode
 	PipelineJenkinsfileEditModeAnnoKey = PipelinePrefix + "jenkinsfile.edit.mode"
+	// PipelineJenkinsfileValidateAnnoKey is the annotation key of the Jenkinsfile validate, success or failure
+	PipelineJenkinsfileValidateAnnoKey = PipelinePrefix + "jenkinsfile.validate"
 
 	// PipelineJenkinsfileEditModeJSON indicates the Jenkinsfile editing mode is JSON
 	PipelineJenkinsfileEditModeJSON = "json"
 	// PipelineJenkinsfileEditModeRaw indicates the Jenkinsfile editing mode is groovy
 	PipelineJenkinsfileEditModeRaw = "raw"
+
+	// PipelineJenkinsfileValidateSuccess indicates the Jenkinsfile validate is success
+	PipelineJenkinsfileValidateSuccess = "success"
+	// PipelineJenkinsfileValidateFailure indicates the Jenkinsfile validate is failure
+	PipelineJenkinsfileValidateFailure = "failure"
 )
 
 // PipelineSpec defines the desired state of Pipeline


Test patch:
diff --git a/controllers/jenkins/pipeline/json_converter_test.go b/controllers/jenkins/pipeline/json_converter_test.go
index 32f1abb..6dd6464 100644
--- a/controllers/jenkins/pipeline/json_converter_test.go
+++ b/controllers/jenkins/pipeline/json_converter_test.go
@@ -95,7 +95,7 @@ func TestJenkinsfileReconciler_Reconcile(t *testing.T) {
 	}{{
 		name: "not found",
 		fields: fields{
-			Client: fake.NewFakeClientWithScheme(schema),
+			Client: fake.NewClientBuilder().WithScheme(schema).Build(),
 		},
 		wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
 			assert.Nil(t, err)
@@ -105,7 +105,7 @@ func TestJenkinsfileReconciler_Reconcile(t *testing.T) {
 	}, {
 		name: "invalid edit mode",
 		fields: fields{
-			Client:      fake.NewFakeClientWithScheme(schema, invalidEditMode),
+			Client:      fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(invalidEditMode).Build(),
 			JenkinsCore: core.JenkinsCore{},
 			log:         logr.Logger{},
 			TokenIssuer: &token.FakeIssuer{},
@@ -120,7 +120,7 @@ func TestJenkinsfileReconciler_Reconcile(t *testing.T) {
 	}, {
 		name: "empty edit mode",
 		fields: fields{
-			Client:      fake.NewFakeClientWithScheme(schema, emptyEditMode),
+			Client:      fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(emptyEditMode).Build(),
 			JenkinsCore: core.JenkinsCore{},
 			log:         logr.Logger{},
 			TokenIssuer: &token.FakeIssuer{},
@@ -135,7 +135,7 @@ func TestJenkinsfileReconciler_Reconcile(t *testing.T) {
 	}, {
 		name: "irregular pipeline, and jenkinsfile edit mode",
 		fields: fields{
-			Client:      fake.NewFakeClientWithScheme(schema, irregularPip),
+			Client:      fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(irregularPip).Build(),
 			JenkinsCore: core.JenkinsCore{},
 			log:         logr.Logger{},
 		},
@@ -149,7 +149,7 @@ func TestJenkinsfileReconciler_Reconcile(t *testing.T) {
 	}, {
 		name: "a regular pipeline with jenkinsfile edit mode",
 		fields: fields{
-			Client: fake.NewFakeClientWithScheme(schema, pip),
+			Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(pip).Build(),
 			JenkinsCore: core.JenkinsCore{
 				URL: "http://localhost",
 			},
@@ -174,6 +174,8 @@ func TestJenkinsfileReconciler_Reconcile(t *testing.T) {
 			}, pip)
 			assert.Nil(t, err)
 			assert.Equal(t, `{"a":"b"}`, pip.Annotations[v1alpha3.PipelineJenkinsfileValueAnnoKey])
+			assert.Equal(t, "", pip.Annotations[v1alpha3.PipelineJenkinsfileEditModeAnnoKey])
+			assert.Equal(t, v1alpha3.PipelineJenkinsfileValidateSuccess, pip.Annotations[v1alpha3.PipelineJenkinsfileValidateAnnoKey])
 		},
 		wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
 			assert.Nil(t, err)
@@ -182,7 +184,7 @@ func TestJenkinsfileReconciler_Reconcile(t *testing.T) {
 	}, {
 		name: "a regular pipeline with JSON edit mode",
 		fields: fields{
-			Client: fake.NewFakeClientWithScheme(schema, jsonEditModePip),
+			Client: fake.NewClientBuilder().WithScheme(schema).WithRuntimeObjects(jsonEditModePip).Build(),
 			JenkinsCore: core.JenkinsCore{
 				URL: "http://localhost",
 			},