optimize resourceconsist controller events (#38)

style, resourceconsist controller events
KusionStack · Apr 7, 2024 · 2b5eba5 · 2b5eba5
1 parent 7ba8e75
commit 2b5eba5
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 81 deletions.
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -85,6 +85,7 @@ jobs:
   SecretScan:
     name: Secret Scan
     runs-on: ubuntu-latest
+    if: ${{ github.ref != 'refs/heads/main' }}
     steps:
       - name: Checkout
         uses: actions/checkout@v3

diff --git a/Makefile b/Makefile
@@ -192,7 +192,7 @@ $(CONTROLLER_GEN): $(LOCALBIN)
 .PHONY: envtest
 envtest: $(ENVTEST) ## Download envtest-setup locally if necessary.
 $(ENVTEST): $(LOCALBIN)
-	test -s $(LOCALBIN)/setup-envtest || GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
+	test -s $(LOCALBIN)/setup-envtest || GOBIN=$(LOCALBIN) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.16
 
 .PHONY: ginkgo
 ginkgo: $(GINKGO) ## Download ginkgo locally if necessary. If wrong version is installed, it will be overwritten.

diff --git a/pkg/frame/controller/consts.go b/pkg/frame/controller/consts.go
@@ -22,3 +22,14 @@ const (
 	lifecycleFinalizerRecordedAnnoKey = "resource-consist.kusionstack.io/employees-lifecycle-finalizer-recorded"
 	cleanFinalizerPrefix              = "resource-consist.kusionstack.io/clean-"
 )
+
+// Event reason list
+const (
+	EnsureEmployerCleanFinalizerFailed  = "EnsureEmployerCleanFinalizerFailed"
+	EnsureEmployerCleanFinalizerSucceed = "EnsureEmployerCleanFinalizerSucceed"
+	EnsureExpectedFinalizerFailed       = "EnsureExpectedFinalizerFailed"
+	SyncEmployerFailed                  = "SyncEmployerFailed"
+	SyncEmployeesFailed                 = "SyncEmployeesFailed"
+	CleanEmployerCleanFinalizerFailed   = "CleanEmployerCleanFinalizerFailed"
+	CleanEmployerCleanFinalizerSucceed  = "CleanEmployerCleanFinalizerSucceed"
+)
diff --git a/pkg/frame/controller/resourceconsist_controller.go b/pkg/frame/controller/resourceconsist_controller.go
@@ -136,6 +136,9 @@ func (r *Consist) Reconcile(ctx context.Context, request reconcile.Request) (rec
 		employer = &corev1.Service{}
 	}
 
+	logger := r.logger.WithValues("resourceconsist", request.String(), "kind", employer.GetObjectKind().GroupVersionKind().Kind)
+	defer logger.Info("reconcile finished")
+
 	if _, ok := r.adapter.(MultiClusterOptions); ok {
 		err = r.Client.Get(clusterinfo.WithCluster(ctx, clusterinfo.Fed), types.NamespacedName{
 			Namespace: request.Namespace,
@@ -152,100 +155,85 @@ func (r *Consist) Reconcile(ctx context.Context, request reconcile.Request) (rec
 		if errors.IsNotFound(err) {
 			return reconcile.Result{}, nil
 		}
-		r.logger.Error(err, fmt.Sprintf("get %s: %s/%s failed", employer.GetObjectKind().GroupVersionKind().Kind,
-			request.Namespace, request.Name))
+		logger.Error(err, "get employer failed")
 		return reconcile.Result{}, err
 	}
 
-	// ensure employer-clean finalizer firstly, employer-clean finalizer should be cleaned at the end
+	// Ensure employer-clean finalizer firstly, employer-clean finalizer should be cleaned at the end
 	updated, err := r.ensureEmployerCleanFlz(ctx, employer)
 	if err != nil {
-		r.logger.Error(err, fmt.Sprintf("add employer-clean finalizer for %s: %s/%s failed",
-			employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-		r.recorder.Eventf(employer, corev1.EventTypeWarning, "ensureEmployerCleanFlzFailed",
-			"add employer-clean finalizer failed: %s", err.Error())
+		logger.Error(err, "add employer clean finalizer failed")
+		r.recorder.Eventf(employer, corev1.EventTypeWarning, EnsureEmployerCleanFinalizerFailed,
+			"add employer clean finalizer failed: %s", err.Error())
 		return reconcile.Result{}, err
 	}
 	if updated {
-		r.logger.Info(fmt.Sprintf("add employer-clean finalizer for %s: %s/%s",
-			employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-		r.recorder.Eventf(employer, corev1.EventTypeNormal, "ensureEmployerCleanFlzSucceed",
-			"add employer-clean finalizer")
+		logger.Info("add employer clean finalizer succeed")
+		r.recorder.Event(employer, corev1.EventTypeNormal, EnsureEmployerCleanFinalizerSucceed,
+			"add employer clean finalizer succeed")
 		return reconcile.Result{}, nil
 	}
 
 	isExpectedClean, err := r.ensureExpectedFinalizer(ctx, employer)
 	if err != nil {
-		r.logger.Error(err, fmt.Sprintf("ensure employees' expected finalizer failed for %s: %s/%s",
-			employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-		r.recorder.Eventf(employer, corev1.EventTypeWarning, "ensureExpectedFinalizerFailed",
-			"ensure employees' expected finalizer failed: %s", err.Error())
+		logger.Error(err, "ensure employees expected finalizer failed")
+		r.recorder.Eventf(employer, corev1.EventTypeWarning, EnsureExpectedFinalizerFailed,
+			"ensure employees expected finalizer failed: %s", err.Error())
 		return reconcile.Result{}, err
 	}
 
+	// Sync employer
 	expectedEmployer, err := r.adapter.GetExpectedEmployer(ctx, employer)
 	if err != nil {
-		r.logger.Error(err, fmt.Sprintf("get expected employer for %s: %s/%s failed",
-			employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-		r.recorder.Eventf(employer, corev1.EventTypeWarning, "GetExpectEmployerFailed",
-			"get expect employer failed: %s", err.Error())
+		logger.Error(err, "get expect employer failed")
 		return reconcile.Result{}, err
 	}
 	currentEmployer, err := r.adapter.GetCurrentEmployer(ctx, employer)
 	if err != nil {
-		r.logger.Error(err, fmt.Sprintf("get current employer for %s: %s/%s failed",
-			employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-		r.recorder.Eventf(employer, corev1.EventTypeWarning, "GetCurrentEmployerFailed",
-			"get current employer failed: %s", err.Error())
+		logger.Error(err, "get current employer failed")
 		return reconcile.Result{}, err
 	}
 	isCleanEmployer, syncEmployerFailedExist, cudEmployerResults, err := r.syncEmployer(ctx, employer, expectedEmployer, currentEmployer)
 	if err != nil {
-		r.logger.Error(err, fmt.Sprintf("sync employer for %s: %s/%s failed",
-			employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-		r.recorder.Eventf(employer, corev1.EventTypeWarning, "syncEmployerFailed",
+		logger.Error(err, "sync employer failed")
+		r.recorder.Eventf(employer, corev1.EventTypeWarning, SyncEmployerFailed,
 			"sync employer failed: %s", err.Error())
 		return reconcile.Result{}, err
 	}
 
-	expectEmployees, err := r.adapter.GetExpectedEmployee(ctx, employer)
+	// Sync employees
+	expectedEmployees, err := r.adapter.GetExpectedEmployee(ctx, employer)
 	if err != nil {
-		r.logger.Error(err, fmt.Sprintf("get expect employees for %s: %s/%s failed",
-			employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-		r.recorder.Eventf(employer, corev1.EventTypeWarning, "GetExpectEmployeeFailed",
-			"get expect employees failed: %s", err.Error())
+		logger.Error(err, "get expect employees failed")
 		return reconcile.Result{}, err
 	}
 	currentEmployees, err := r.adapter.GetCurrentEmployee(ctx, employer)
 	if err != nil {
-		r.logger.Error(err, fmt.Sprintf("get current employees for %s: %s/%s failed",
-			employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-		r.recorder.Eventf(employer, corev1.EventTypeWarning, "GetCurrentEmployeeFailed",
-			"get current employees failed: %s", err.Error())
+		logger.Error(err, "get current employees failed")
 		return reconcile.Result{}, err
 	}
-	isCleanEmployee, syncEmployeeFailedExist, cudEmployeeResults, err := r.syncEmployees(ctx, employer, expectEmployees, currentEmployees)
+	isCleanEmployee, syncEmployeeFailedExist, cudEmployeeResults, err := r.syncEmployees(ctx, employer, expectedEmployees, currentEmployees)
 	if err != nil {
-		r.logger.Error(err, fmt.Sprintf("sync employees for %s: %s/%s failed",
-			employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-		r.recorder.Eventf(employer, corev1.EventTypeWarning, "syncEmployeesFailed",
+		logger.Error(err, "sync employees failed")
+		r.recorder.Eventf(employer, corev1.EventTypeWarning, SyncEmployeesFailed,
 			"sync employees failed: %s", err.Error())
 		return reconcile.Result{}, err
 	}
 
 	if isCleanEmployer && isCleanEmployee && isExpectedClean && !employer.GetDeletionTimestamp().IsZero() {
 		err = r.cleanEmployerCleanFinalizer(ctx, employer)
 		if err != nil {
-			r.logger.Error(err, fmt.Sprintf("clean employer clean-finalizer for %s: %s/%s failed",
-				employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-			r.recorder.Eventf(employer, corev1.EventTypeWarning, "cleanEmployerCleanFinalizerFailed",
+			logger.Error(err, "clean employer clean-finalizer failed")
+			r.recorder.Eventf(employer, corev1.EventTypeWarning, CleanEmployerCleanFinalizerFailed,
 				"clean employer clean-finalizer failed: %s", err.Error())
 			return reconcile.Result{}, err
+		} else {
+			r.recorder.Event(employer, corev1.EventTypeNormal, CleanEmployerCleanFinalizerSucceed,
+				"clean employer clean finalizer succeed")
 		}
 	}
 
 	if syncEmployerFailedExist || syncEmployeeFailedExist {
-		r.recorder.Eventf(employer, corev1.EventTypeNormal, "ReconcileFailed", "employer or employees synced failed exist")
 		requeueOptions, requeueOptionsImplemented := r.adapter.(ReconcileRequeueOptions)
 		if requeueOptionsImplemented {
 			return reconcile.Result{RequeueAfter: requeueOptions.EmployeeSyncRequeueInterval()}, nil
@@ -256,14 +244,10 @@ func (r *Consist) Reconcile(ctx context.Context, request reconcile.Request) (rec
 	if recordOptions, ok := r.adapter.(StatusRecordOptions); ok {
 		err = recordOptions.RecordStatuses(ctx, employer, cudEmployerResults, cudEmployeeResults)
 		if err != nil {
-			r.logger.Error(err, fmt.Sprintf("record status for %s: %s/%s failed",
-				employer.GetObjectKind().GroupVersionKind().Kind, employer.GetNamespace(), employer.GetName()))
-			r.recorder.Eventf(employer, corev1.EventTypeWarning, "recordStatusesFailed",
-				"record status for employer failed: %s", err.Error())
+			logger.Error(err, "record status failed")
 			return reconcile.Result{}, err
 		}
 	}
 
-	r.recorder.Eventf(employer, corev1.EventTypeNormal, "ReconcileSucceed", "")
 	return reconcile.Result{}, nil
 }
diff --git a/pkg/frame/controller/resourceconsit_controller_test.go b/pkg/frame/controller/resourceconsit_controller_test.go
@@ -479,7 +479,7 @@ var _ = Describe("resource-consist-controller", func() {
 					return false
 				}
 				for _, evt := range events.Items {
-					if evt.Reason == "syncEmployerFailed" &&
+					if evt.Reason == SyncEmployerFailed &&
 						evt.Message == "sync employer failed: syncCreate failed, err: fake err" {
 						return true
 					}
@@ -492,21 +492,6 @@ var _ = Describe("resource-consist-controller", func() {
 				RemoteVIPQPS: 100,
 			})
 
-			Eventually(func() bool {
-				events, err := clientSet.CoreV1().Events("default").List(context.TODO(), v1.ListOptions{
-					FieldSelector: "involvedObject.name=resource-consist-ut-svc-2",
-					TypeMeta:      v1.TypeMeta{Kind: "Service"}})
-				if err != nil {
-					return false
-				}
-				for _, evt := range events.Items {
-					if evt.Reason == "ReconcileSucceed" && evt.Message == "" {
-						return true
-					}
-				}
-				return false
-			}, 3*time.Second, 100*time.Millisecond).Should(BeTrue())
-
 			Eventually(func() bool {
 				details, exist := demoResourceVipStatusInProvider.Load(svc2.Name)
 				return exist && details.(DemoServiceDetails).RemoteVIP == "demo-remote-VIP" && details.(DemoServiceDetails).RemoteVIPQPS == 100
@@ -528,7 +513,7 @@ var _ = Describe("resource-consist-controller", func() {
 					return false
 				}
 				for _, evt := range events.Items {
-					if evt.Reason == "syncEmployeesFailed" &&
+					if evt.Reason == SyncEmployeesFailed &&
 						evt.Message == "sync employees failed: syncCreate failed, err: fake err" {
 						return true
 					}
@@ -550,21 +535,6 @@ var _ = Describe("resource-consist-controller", func() {
 				},
 			})
 
-			Eventually(func() bool {
-				events, err := clientSet.CoreV1().Events("default").List(context.TODO(), v1.ListOptions{
-					FieldSelector: "involvedObject.name=resource-consist-ut-svc-2",
-					TypeMeta:      v1.TypeMeta{Kind: "Service"}})
-				if err != nil {
-					return false
-				}
-				for _, evt := range events.Items {
-					if evt.Reason == "ReconcileSucceed" && evt.Message == "" {
-						return true
-					}
-				}
-				return false
-			}, 3*time.Second, 100*time.Millisecond).Should(BeTrue())
-
 			Eventually(func() bool {
 				details, exist := demoResourceRsStatusInProvider.Load(pod2.Name)
 				return exist && details.(DemoPodStatus).GetEmployeeName() == pod2.Name &&