remove duplicated code and fix minor bugs

armadaproject · Sep 11, 2024 · 878085e · 878085e
1 parent e3bcf44
commit 878085e
Show file tree

Hide file tree

Showing 18 changed files with 567 additions and 514 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 {

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().