remove duplicated code and fix minor bugs

Signed-off-by: Dejan Zele Pejchev <pejcev.dejan@gmail.com>
armadaproject · Sep 12, 2024 · e249792 · e249792
1 parent e3bcf44
commit e249792
Show file tree

Hide file tree

Showing 20 changed files with 695 additions and 543 deletions.
diff --git a/go.mod b/go.mod
@@ -23,6 +23,7 @@ require (
 	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
 	github.com/emicklei/go-restful/v3 v3.12.1 // indirect
+	github.com/evanphx/json-patch v4.12.0+incompatible // indirect
 	github.com/evanphx/json-patch/v5 v5.9.0 // indirect
 	github.com/fsnotify/fsnotify v1.7.0 // indirect
 	github.com/go-logr/zapr v1.3.0 // indirect

diff --git a/internal/controller/install/armadaserver_controller.go b/internal/controller/install/armadaserver_controller.go
@@ -63,68 +63,35 @@ type ArmadaServerReconciler struct {
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
 // move the current state of the cluster closer to the desired state.
 func (r *ArmadaServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	logger := log.FromContext(ctx).WithValues("namespace", req.Namespace, "name", req.Name)
+	logger := log.FromContext(ctx).WithValues("kind", "ArmadaServer", "namespace", req.Namespace, "name", req.Name)
+
 	started := time.Now()
-	logger.Info("Reconciling ArmadaServer object")
-
-	logger.Info("Fetching ArmadaServer object from cache")
-	var as installv1alpha1.ArmadaServer
-	if err := r.Client.Get(ctx, req.NamespacedName, &as); err != nil {
-		if k8serrors.IsNotFound(err) {
-			logger.Info("ArmadaServer not found in cache, ending reconcile...", "namespace", req.Namespace, "name", req.Name)
-			return ctrl.Result{}, nil
-		}
+
+	logger.Info("Reconciling object")
+
+	var server installv1alpha1.ArmadaServer
+	if miss, err := getObjectFromCache(ctx, r.Client, &server, req.NamespacedName, logger); err != nil || miss {
 		return ctrl.Result{}, err
 	}
 
-	pc, err := installv1alpha1.BuildPortConfig(as.Spec.ApplicationConfig)
+	pc, err := installv1alpha1.BuildPortConfig(server.Spec.ApplicationConfig)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-	as.Spec.PortConfig = pc
+	server.Spec.PortConfig = pc
 
 	var components *CommonComponents
-	components, err = generateArmadaServerInstallComponents(&as, r.Scheme)
+	components, err = generateArmadaServerInstallComponents(&server, r.Scheme)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	deletionTimestamp := as.ObjectMeta.DeletionTimestamp
-	// examine DeletionTimestamp to determine if object is under deletion
-	if deletionTimestamp.IsZero() {
-		// The object is not being deleted, so if it does not have our finalizer,
-		// then lets add the finalizer and update the object. This is equivalent
-		// registering our finalizer.
-		if !controllerutil.ContainsFinalizer(&as, operatorFinalizer) {
-			logger.Info("Attaching finalizer to As object", "finalizer", operatorFinalizer)
-			controllerutil.AddFinalizer(&as, operatorFinalizer)
-			if err := r.Update(ctx, &as); err != nil {
-				return ctrl.Result{}, err
-			}
-		}
-	} else {
-		logger.Info("ArmadaServer object is being deleted", "finalizer", operatorFinalizer)
-		logger.Info("Namespace-scoped resources will be deleted by Kubernetes based on their OwnerReference")
-		// The object is being deleted
-		if controllerutil.ContainsFinalizer(&as, operatorFinalizer) {
-			// our finalizer is present, so lets handle any external dependency
-			logger.Info("Running cleanup function for ArmadaServer cluster-scoped components", "finalizer", operatorFinalizer)
-			if err := r.deleteExternalResources(ctx, components, logger); err != nil {
-				// if fail to delete the external dependency here, return with error
-				// so that it can be retried
-				return ctrl.Result{}, err
-			}
-
-			// remove our finalizer from the list and update it.
-			logger.Info("Removing finalizer from ArmadaServer object", "finalizer", operatorFinalizer)
-			controllerutil.RemoveFinalizer(&as, operatorFinalizer)
-			if err := r.Update(ctx, &as); err != nil {
-				return ctrl.Result{}, err
-			}
-		}
-
-		// Stop reconciliation as the item is being deleted
-		return ctrl.Result{}, nil
+	cleanupF := func(ctx context.Context) error {
+		return r.deleteExternalResources(ctx, components, logger)
+	}
+	finish, err := checkAndHandleResourceDeletion(ctx, r.Client, &server, operatorFinalizer, cleanupF, logger)
+	if err != nil || finish {
+		return ctrl.Result{}, err
 	}
 
 	componentsCopy := components.DeepCopy()
@@ -134,21 +101,15 @@ func (r *ArmadaServerReconciler) Reconcile(ctx context.Context, req ctrl.Request
 		return nil
 	}
 
-	if components.ServiceAccount != nil {
-		logger.Info("Upserting ArmadaServer ServiceAccount object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.ServiceAccount, mutateFn); err != nil {
-			return ctrl.Result{}, err
-		}
+	if err := upsertObject(ctx, r.Client, components.ServiceAccount, server.Kind, mutateFn, logger); err != nil {
+		return ctrl.Result{}, err
 	}
 
-	if components.Secret != nil {
-		logger.Info("Upserting ArmadaServer Secret object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.Secret, mutateFn); err != nil {
-			return ctrl.Result{}, err
-		}
+	if err := upsertObject(ctx, r.Client, components.Secret, server.Kind, mutateFn, logger); err != nil {
+		return ctrl.Result{}, err
 	}
 
-	if as.Spec.PulsarInit {
+	if server.Spec.PulsarInit {
 		for idx := range components.Jobs {
 			err = func() error {
 				if components.Jobs[idx] != nil {
@@ -283,13 +244,17 @@ func generateArmadaServerInstallComponents(as *installv1alpha1.ArmadaServer, sch
 	}
 
 	var pr *monitoringv1.PrometheusRule
+	var sm *monitoringv1.ServiceMonitor
 	if as.Spec.Prometheus != nil && as.Spec.Prometheus.Enabled {
 		pr = createServerPrometheusRule(as.Name, as.Namespace, as.Spec.Prometheus.ScrapeInterval, as.Spec.Labels, as.Spec.Prometheus.Labels)
-	}
+		if err := controllerutil.SetOwnerReference(as, pr, scheme); err != nil {
+			return nil, err
+		}
 
-	sm := createServiceMonitor(as)
-	if err := controllerutil.SetOwnerReference(as, sm, scheme); err != nil {
-		return nil, err
+		sm = createServiceMonitor(as)
+		if err := controllerutil.SetOwnerReference(as, sm, scheme); err != nil {
+			return nil, err
+		}
 	}
 
 	jobs := []*batchv1.Job{{}}

diff --git a/internal/controller/install/binoculars_controller.go b/internal/controller/install/binoculars_controller.go
@@ -60,18 +60,14 @@ type BinocularsReconciler struct {
 // For more details, check Reconcile and its Result here:
 // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.0/pkg/reconcile
 func (r *BinocularsReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	logger := log.FromContext(ctx).WithValues("namespace", req.Namespace, "name", req.Name)
+	logger := log.FromContext(ctx).WithValues("kind", "Binoculars", "namespace", req.Namespace, "name", req.Name)
+
 	started := time.Now()
-	logger.Info("Reconciling Binoculars object")
 
-	logger.Info("Fetching Binoculars object from cache")
+	logger.Info("Reconciling Binoculars object")
 
 	var binoculars installv1alpha1.Binoculars
-	if err := r.Client.Get(ctx, req.NamespacedName, &binoculars); err != nil {
-		if k8serrors.IsNotFound(err) {
-			logger.Info("Binoculars not found in cache, ending reconcile...", "namespace", req.Namespace, "name", req.Name)
-			return ctrl.Result{}, nil
-		}
+	if miss, err := getObjectFromCache(ctx, r.Client, &binoculars, req.NamespacedName, logger); err != nil || miss {
 		return ctrl.Result{}, err
 	}
 
@@ -87,41 +83,12 @@ func (r *BinocularsReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, err
 	}
 
-	deletionTimestamp := binoculars.ObjectMeta.DeletionTimestamp
-	// examine DeletionTimestamp to determine if object is under deletion
-	if deletionTimestamp.IsZero() {
-		// The object is not being deleted, so if it does not have our finalizer,
-		// then lets add the finalizer and update the object. This is equivalent
-		// registering our finalizer.
-		if !controllerutil.ContainsFinalizer(&binoculars, operatorFinalizer) {
-			logger.Info("Attaching finalizer to Binoculars object", "finalizer", operatorFinalizer)
-			controllerutil.AddFinalizer(&binoculars, operatorFinalizer)
-			if err := r.Update(ctx, &binoculars); err != nil {
-				return ctrl.Result{}, err
-			}
-		}
-	} else {
-		logger.Info("Binoculars object is being deleted", "finalizer", operatorFinalizer)
-		// The object is being deleted
-		if controllerutil.ContainsFinalizer(&binoculars, operatorFinalizer) {
-			// our finalizer is present, so lets handle any external dependency
-			logger.Info("Running cleanup function for Binoculars object", "finalizer", operatorFinalizer)
-			if err := r.deleteExternalResources(ctx, components); err != nil {
-				// if fail to delete the external dependency here, return with error
-				// so that it can be retried
-				return ctrl.Result{}, err
-			}
-
-			// remove our finalizer from the list and update it.
-			logger.Info("Removing finalizer from Binoculars object", "finalizer", operatorFinalizer)
-			controllerutil.RemoveFinalizer(&binoculars, operatorFinalizer)
-			if err := r.Update(ctx, &binoculars); err != nil {
-				return ctrl.Result{}, err
-			}
-		}
-
-		// Stop reconciliation as the item is being deleted
-		return ctrl.Result{}, nil
+	cleanupF := func(ctx context.Context) error {
+		return r.deleteExternalResources(ctx, components)
+	}
+	finish, err := checkAndHandleResourceDeletion(ctx, r.Client, &binoculars, operatorFinalizer, cleanupF, logger)
+	if err != nil || finish {
+		return ctrl.Result{}, err
 	}
 
 	componentsCopy := components.DeepCopy()
@@ -131,58 +98,40 @@ func (r *BinocularsReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return nil
 	}
 
-	if components.ServiceAccount != nil {
-		logger.Info("Upserting Binoculars ServiceAccount object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.ServiceAccount, mutateFn); err != nil {
-			return ctrl.Result{}, err
-		}
+	if err := upsertObject(ctx, r.Client, components.ServiceAccount, binoculars.Kind, mutateFn, logger); err != nil {
+		return ctrl.Result{}, err
 	}
 
-	if components.ClusterRole != nil {
-		logger.Info("Upserting Binoculars ClusterRole object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.ClusterRole, mutateFn); err != nil {
-			return ctrl.Result{}, err
-		}
+	if err := upsertObject(ctx, r.Client, components.ClusterRole, binoculars.Kind, mutateFn, logger); err != nil {
+		return ctrl.Result{}, err
 	}
 
-	if components.ClusterRoleBindings != nil && len(components.ClusterRoleBindings) > 0 {
-		logger.Info("Upserting Binoculars ClusterRoleBinding object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.ClusterRoleBindings[0], mutateFn); err != nil {
-			return ctrl.Result{}, err
+	if len(components.ClusterRoleBindings) > 0 {
+		for _, crb := range components.ClusterRoleBindings {
+			if err := upsertObject(ctx, r.Client, crb, binoculars.Kind, mutateFn, logger); err != nil {
+				return ctrl.Result{}, err
+			}
 		}
 	}
 
-	if components.Secret != nil {
-		logger.Info("Upserting Binoculars Secret object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.Secret, mutateFn); err != nil {
-			return ctrl.Result{}, err
-		}
+	if err := upsertObject(ctx, r.Client, components.Secret, binoculars.Kind, mutateFn, logger); err != nil {
+		return ctrl.Result{}, err
 	}
 
-	if components.Deployment != nil {
-		logger.Info("Upserting Binoculars Deployment object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.Deployment, mutateFn); err != nil {
-			return ctrl.Result{}, err
-		}
+	if err := upsertObject(ctx, r.Client, components.Deployment, binoculars.Kind, mutateFn, logger); err != nil {
+		return ctrl.Result{}, err
 	}
 
-	if components.Service != nil {
-		logger.Info("Upserting Binoculars Service object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.Service, mutateFn); err != nil {
-			return ctrl.Result{}, err
-		}
+	if err := upsertObject(ctx, r.Client, components.Service, binoculars.Kind, mutateFn, logger); err != nil {
+		return ctrl.Result{}, err
 	}
-	if components.IngressGrpc != nil {
-		logger.Info("Upserting GRPC Ingress object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.IngressGrpc, mutateFn); err != nil {
-			return ctrl.Result{}, err
-		}
+
+	if err := upsertObject(ctx, r.Client, components.IngressGrpc, binoculars.Kind, mutateFn, logger); err != nil {
+		return ctrl.Result{}, err
 	}
-	if components.IngressHttp != nil {
-		logger.Info("Upserting REST Ingress object")
-		if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, components.IngressHttp, mutateFn); err != nil {
-			return ctrl.Result{}, err
-		}
+
+	if err := upsertObject(ctx, r.Client, components.IngressHttp, binoculars.Kind, mutateFn, logger); err != nil {
+		return ctrl.Result{}, err
 	}
 
 	logger.Info("Successfully reconciled Binoculars object", "durationMillis", time.Since(started).Milliseconds())

diff --git a/internal/controller/install/binoculars_controller_test.go b/internal/controller/install/binoculars_controller_test.go
@@ -161,14 +161,19 @@ func TestBinocularsReconciler_Reconcile(t *testing.T) {
 		t.Fatal("We should not fail on generating binoculars")
 	}
 
+	// Binoculars
 	mockK8sClient := k8sclient.NewMockClient(mockCtrl)
 	mockK8sClient.
 		EXPECT().
 		Get(gomock.Any(), expectedNamespacedName, gomock.AssignableToTypeOf(&v1alpha1.Binoculars{})).
 		Return(nil).
 		SetArg(2, expectedBinoculars)
-	// Binoculars finalizer
-	mockK8sClient.EXPECT().Update(gomock.Any(), gomock.AssignableToTypeOf(&installv1alpha1.Binoculars{})).Return(nil)
+
+	// Finalizer
+	mockK8sClient.
+		EXPECT().
+		Update(gomock.Any(), gomock.AssignableToTypeOf(&installv1alpha1.Binoculars{})).
+		Return(nil)
 
 	mockK8sClient.
 		EXPECT().