fix: Delete cronjobs when pruning disabled and actually reconcile sch…

…eduler corncobs (#338) * Delete cronjobs when pruning disabled and actually reconcile scheduler cronjobs Signed-off-by: Jason Parraga <[email protected]> * Address comments Signed-off-by: Jason Parraga <[email protected]> --------- Signed-off-by: Jason Parraga <[email protected]>
armadaproject · Nov 21, 2024 · 9e41715 · 9e41715
1 parent 6633b3f
commit 9e41715
Show file tree

Hide file tree

Showing 5 changed files with 455 additions and 26 deletions.
diff --git a/internal/controller/install/common_helpers.go b/internal/controller/install/common_helpers.go
@@ -596,13 +596,13 @@ func upsertObjectIfNeeded(
 	mutateFn controllerutil.MutateFn,
 	logger logr.Logger,
 ) error {
-	if !isNil(object) {
-		logger.Info(fmt.Sprintf("Upserting %s %s object", componentName, object.GetObjectKind()))
-		if _, err := controllerutil.CreateOrUpdate(ctx, client, object, mutateFn); err != nil {
-			return err
-		}
+	if isNil(object) {
+		return nil
 	}
-	return nil
+
+	logger.Info(fmt.Sprintf("Upserting %s %s object", componentName, object.GetObjectKind()))
+	_, err := controllerutil.CreateOrUpdate(ctx, client, object, mutateFn)
+	return err
 }
 
 // Helper function to determine if the object is nil even if it's a pointer to a nil value
@@ -619,6 +619,27 @@ func isNil(i any) bool {
 	}
 }
 
+// deleteObjectIfNeeded will delete the object if it exists.
+func deleteObjectIfNeeded(
+	ctx context.Context,
+	k8sClient client.Client,
+	object client.Object,
+	componentName string,
+	logger logr.Logger,
+) error {
+	if isNil(object) {
+		return nil
+	}
+
+	if err := k8sClient.Delete(ctx, object); err != nil {
+		return client.IgnoreNotFound(err)
+	}
+
+	logger.Info("Successfully deleted %s %s", componentName, object.GetObjectKind())
+
+	return nil
+}
+
 // getObject will get the object from Kubernetes and return if it is missing or an error.
 func getObject(
 	ctx context.Context,

diff --git a/internal/controller/install/lookout_controller.go b/internal/controller/install/lookout_controller.go
@@ -124,8 +124,14 @@ func (r *LookoutReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		return ctrl.Result{}, err
 	}
 
-	if err := upsertObjectIfNeeded(ctx, r.Client, components.CronJob, lookout.Kind, mutateFn, logger); err != nil {
-		return ctrl.Result{}, err
+	if enabled := lookout.Spec.DbPruningEnabled; enabled != nil && *enabled {
+		if err := upsertObjectIfNeeded(ctx, r.Client, components.CronJob, lookout.Kind, mutateFn, logger); err != nil {
+			return ctrl.Result{}, err
+		}
+	} else {
+		if err := deleteObjectIfNeeded(ctx, r.Client, components.CronJob, lookout.Kind, logger); err != nil {
+			return ctrl.Result{}, err
+		}
 	}
 
 	if err := upsertObjectIfNeeded(ctx, r.Client, components.ServiceMonitor, lookout.Kind, mutateFn, logger); err != nil {
@@ -210,15 +216,12 @@ func generateLookoutInstallComponents(
 		return nil, err
 	}
 
-	var cronJob *batchv1.CronJob
-	if enabled := lookout.Spec.DbPruningEnabled; enabled != nil && *enabled {
-		cronJob, err = createLookoutCronJob(lookout, serviceAccountName)
-		if err != nil {
-			return nil, err
-		}
-		if err := controllerutil.SetOwnerReference(lookout, cronJob, scheme); err != nil {
-			return nil, err
-		}
+	cronJob, err := createLookoutCronJob(lookout, serviceAccountName)
+	if err != nil {
+		return nil, err
+	}
+	if err := controllerutil.SetOwnerReference(lookout, cronJob, scheme); err != nil {
+		return nil, err
 	}
 
 	ingressHTTP, err := createLookoutIngressHttp(lookout, config)

diff --git a/internal/controller/install/lookout_controller_test.go b/internal/controller/install/lookout_controller_test.go
@@ -218,6 +218,188 @@ func TestLookoutReconciler_Reconcile(t *testing.T) {
 	}
 }
 
+func TestLookoutReconciler_ReconcilePruningDisabled(t *testing.T) {
+	t.Parallel()
+
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	scheme, err := v1alpha1.SchemeBuilder.Build()
+	if err != nil {
+		t.Fatalf("should not return error when building schema")
+	}
+
+	expectedNamespacedName := types.NamespacedName{Namespace: "default", Name: "lookout"}
+	dbPruningEnabled := false
+	dbPruningSchedule := "1d"
+	terminationGracePeriod := int64(20)
+	expectedLookout := v1alpha1.Lookout{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Lookout",
+			APIVersion: "install.armadaproject.io/v1alpha1",
+		},
+		ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "lookout"},
+		Spec: v1alpha1.LookoutSpec{
+			Replicas: ptr.To[int32](2),
+			CommonSpecBase: installv1alpha1.CommonSpecBase{
+				Labels: nil,
+				Image: v1alpha1.Image{
+					Repository: "testrepo",
+					Tag:        "1.0.0",
+				},
+				ApplicationConfig:             runtime.RawExtension{},
+				Resources:                     &corev1.ResourceRequirements{},
+				Prometheus:                    &installv1alpha1.PrometheusConfig{Enabled: true, ScrapeInterval: &metav1.Duration{Duration: 1 * time.Second}},
+				TerminationGracePeriodSeconds: &terminationGracePeriod,
+			},
+			ClusterIssuer: "test",
+			HostNames:     []string{"localhost"},
+			Ingress: &installv1alpha1.IngressConfig{
+				IngressClass: "nginx",
+				Labels:       map[string]string{"test": "hello"},
+				Annotations:  map[string]string{"test": "hello"},
+			},
+			DbPruningEnabled:  &dbPruningEnabled,
+			DbPruningSchedule: &dbPruningSchedule,
+		},
+	}
+
+	mockK8sClient := k8sclient.NewMockClient(mockCtrl)
+	// Lookout
+	mockK8sClient.
+		EXPECT().
+		Get(gomock.Any(), expectedNamespacedName, gomock.AssignableToTypeOf(&v1alpha1.Lookout{})).
+		Return(nil).
+		SetArg(2, expectedLookout)
+
+	// Finalizer
+	mockK8sClient.
+		EXPECT().
+		Update(gomock.Any(), gomock.AssignableToTypeOf(&installv1alpha1.Lookout{})).
+		Return(nil)
+
+	// ServiceAccount
+	mockK8sClient.
+		EXPECT().
+		Get(gomock.Any(), expectedNamespacedName, gomock.AssignableToTypeOf(&corev1.ServiceAccount{})).
+		Return(errors.NewNotFound(schema.GroupResource{}, "lookout"))
+	mockK8sClient.
+		EXPECT().
+		Create(gomock.Any(), gomock.AssignableToTypeOf(&corev1.ServiceAccount{})).
+		Return(nil)
+
+	mockK8sClient.
+		EXPECT().
+		Get(gomock.Any(), expectedNamespacedName, gomock.AssignableToTypeOf(&corev1.Secret{})).
+		Return(errors.NewNotFound(schema.GroupResource{}, "lookout"))
+	mockK8sClient.
+		EXPECT().
+		Create(gomock.Any(), gomock.AssignableToTypeOf(&corev1.Secret{})).
+		Return(nil)
+
+	expectedJobName := types.NamespacedName{Namespace: "default", Name: "lookout-migration"}
+	expectedMigrationJob := &batchv1.Job{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "default",
+			Name:      "lookout-migration",
+		},
+		Spec: batchv1.JobSpec{
+			Template: corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{
+						{
+							Name:  "lookout-migration",
+							Image: "testrepo:1.0.0",
+						},
+					},
+				},
+			},
+		},
+		Status: batchv1.JobStatus{
+			Conditions: []batchv1.JobCondition{{
+				Type:   batchv1.JobComplete,
+				Status: corev1.ConditionTrue,
+			}},
+		},
+	}
+
+	mockK8sClient.
+		EXPECT().
+		Get(gomock.Any(), expectedJobName, gomock.AssignableToTypeOf(&batchv1.Job{})).
+		Return(errors.NewNotFound(schema.GroupResource{}, "lookout"))
+	mockK8sClient.
+		EXPECT().
+		Create(gomock.Any(), gomock.AssignableToTypeOf(&batchv1.Job{})).
+		Return(nil)
+	mockK8sClient.
+		EXPECT().
+		Get(gomock.Any(), expectedJobName, gomock.AssignableToTypeOf(&batchv1.Job{})).
+		Return(nil).
+		SetArg(2, *expectedMigrationJob)
+
+	mockK8sClient.
+		EXPECT().
+		Get(gomock.Any(), expectedNamespacedName, gomock.AssignableToTypeOf(&appsv1.Deployment{})).
+		Return(errors.NewNotFound(schema.GroupResource{}, "lookout"))
+	mockK8sClient.
+		EXPECT().
+		Create(gomock.Any(), gomock.AssignableToTypeOf(&appsv1.Deployment{})).
+		Return(nil)
+
+	mockK8sClient.
+		EXPECT().
+		Get(gomock.Any(), expectedNamespacedName, gomock.AssignableToTypeOf(&corev1.Service{})).
+		Return(errors.NewNotFound(schema.GroupResource{}, "lookout"))
+	mockK8sClient.
+		EXPECT().
+		Create(gomock.Any(), gomock.AssignableToTypeOf(&corev1.Service{})).
+		Return(nil)
+
+	// IngressHttp
+	expectedIngressName := expectedNamespacedName
+	expectedIngressName.Name = expectedIngressName.Name + "-rest"
+	mockK8sClient.
+		EXPECT().
+		Get(gomock.Any(), expectedIngressName, gomock.AssignableToTypeOf(&networkingv1.Ingress{})).
+		Return(errors.NewNotFound(schema.GroupResource{}, "lookout"))
+	mockK8sClient.
+		EXPECT().
+		Create(gomock.Any(), gomock.AssignableToTypeOf(&networkingv1.Ingress{})).
+		Return(nil)
+
+	// ServiceMonitor
+	mockK8sClient.
+		EXPECT().
+		Get(gomock.Any(), expectedNamespacedName, gomock.AssignableToTypeOf(&monitoringv1.ServiceMonitor{})).
+		Return(errors.NewNotFound(schema.GroupResource{}, "armadaserver"))
+	mockK8sClient.
+		EXPECT().
+		Create(gomock.Any(), gomock.AssignableToTypeOf(&monitoringv1.ServiceMonitor{})).
+		Return(nil)
+
+	// CronJob should be deleted
+	expectedCronJobName := expectedNamespacedName
+	expectedCronJobName.Name = expectedCronJobName.Name + "-db-pruner"
+	mockK8sClient.
+		EXPECT().
+		Delete(gomock.Any(), gomock.AssignableToTypeOf(&batchv1.CronJob{})).
+		Return(nil)
+
+	r := LookoutReconciler{
+		Client: mockK8sClient,
+		Scheme: scheme,
+	}
+
+	req := ctrl.Request{
+		NamespacedName: types.NamespacedName{Namespace: "default", Name: "lookout"},
+	}
+
+	_, err = r.Reconcile(context.Background(), req)
+	if err != nil {
+		t.Fatalf("reconcile should not return error")
+	}
+}
+
 func TestLookoutReconciler_ReconcileNoLookout(t *testing.T) {
 	t.Parallel()
 

diff --git a/internal/controller/install/scheduler_controller.go b/internal/controller/install/scheduler_controller.go
@@ -135,6 +135,16 @@ func (r *SchedulerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return ctrl.Result{}, err
 	}
 
+	if scheduler.Spec.Pruner != nil && scheduler.Spec.Pruner.Enabled {
+		if err := upsertObjectIfNeeded(ctx, r.Client, components.CronJob, scheduler.Kind, mutateFn, logger); err != nil {
+			return ctrl.Result{}, err
+		}
+	} else {
+		if err := deleteObjectIfNeeded(ctx, r.Client, components.CronJob, scheduler.Kind, logger); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
+
 	logger.Info("Successfully reconciled Scheduler object", "durationMillis", time.Since(started).Milliseconds())
 
 	return ctrl.Result{}, nil
@@ -218,15 +228,12 @@ func generateSchedulerInstallComponents(
 		return nil, err
 	}
 
-	var cronJob *batchv1.CronJob
-	if scheduler.Spec.Pruner != nil && scheduler.Spec.Pruner.Enabled {
-		cronJob, err = newSchedulerCronJob(scheduler, serviceAccountName)
-		if err != nil {
-			return nil, err
-		}
-		if err := controllerutil.SetOwnerReference(scheduler, cronJob, scheme); err != nil {
-			return nil, err
-		}
+	cronJob, err := newSchedulerCronJob(scheduler, serviceAccountName)
+	if err != nil {
+		return nil, err
+	}
+	if err := controllerutil.SetOwnerReference(scheduler, cronJob, scheme); err != nil {
+		return nil, err
 	}
 
 	ingressGRPC, err := newSchedulerIngressGRPC(scheduler, config)