diff --git a/internal/controller/promise_controller.go b/internal/controller/promise_controller.go index 99be5002..375ee5b3 100644 --- a/internal/controller/promise_controller.go +++ b/internal/controller/promise_controller.go @@ -1255,7 +1255,20 @@ func (r *PromiseReconciler) deletePromise(o opts, promise *v1alpha1.Promise) (ct if controllerutil.ContainsFinalizer(promise, dynamicControllerDependantResourcesCleanupFinalizer) { logging.Debug(o.logger, "deleting dependent resources for finalizer", "finalizer", dynamicControllerDependantResourcesCleanupFinalizer) - err := r.deleteDynamicControllerAndWorkflowResources(o, promise) + if err := r.ensureHealthRecordsDeleted(o, promise); err != nil { + return ctrl.Result{}, err + } + // if health records still exist, we want to wait for them to be deleted + healthRecords, err := r.getHealthRecordsForPromise(o, promise) + if err != nil { + return ctrl.Result{}, err + } + if len(healthRecords) > 0 { + logging.Info(o.logger, "waiting for health records to be deleted", "count", len(healthRecords)) + return fastRequeue, nil + } + + err = r.deleteDynamicControllerAndWorkflowResources(o, promise) if err != nil { return defaultRequeue, nil //nolint:nilerr // requeue rather than exponential backoff } @@ -1283,6 +1296,31 @@ func (r *PromiseReconciler) deletePromise(o opts, promise *v1alpha1.Promise) (ct return fastRequeue, nil } +func (r *PromiseReconciler) ensureHealthRecordsDeleted(o opts, promise *v1alpha1.Promise) error { + // Logic to ensure they are deleted is handled by the controller's main reconcile loop + // implicitly because HealthRecords are owner-referenced by the RRs which are deleted + // in the previous step (resourceRequestCleanupFinalizer). + // This function mainly serves as a check, but if we wanted to be proactive we could + // list and delete them here too, though that might be redundant. + return nil +} + +func (r *PromiseReconciler) getHealthRecordsForPromise(o opts, promise *v1alpha1.Promise) ([]v1alpha1.HealthRecord, error) { + healthRecordList := &v1alpha1.HealthRecordList{} + err := r.Client.List(o.ctx, healthRecordList) // TODO: Optimise with label selector if possible, or filter in memory + if err != nil { + return nil, err + } + + var relevantHealthRecords []v1alpha1.HealthRecord + for _, hr := range healthRecordList.Items { + if hr.Data.PromiseRef.Name == promise.Name { + relevantHealthRecords = append(relevantHealthRecords, hr) + } + } + return relevantHealthRecords, nil +} + func (r *PromiseReconciler) deletePromiseWorkflowJobs(o opts, promise *v1alpha1.Promise, finalizer string) error { jobGVK := schema.GroupVersionKind{ Group: batchv1.SchemeGroupVersion.Group, diff --git a/internal/controller/promise_controller_test.go b/internal/controller/promise_controller_test.go index 2f3c5259..c88639f7 100644 --- a/internal/controller/promise_controller_test.go +++ b/internal/controller/promise_controller_test.go @@ -12,6 +12,7 @@ import ( "github.com/syntasso/kratix/internal/controller" "github.com/syntasso/kratix/internal/controller/controllerfakes" + "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" controllerConfig "sigs.k8s.io/controller-runtime/pkg/config" @@ -1743,6 +1744,72 @@ var _ = Describe("PromiseController", func() { }) }) }) + When("the promise is being deleted", func() { + When("it has active health records", func() { + var healthRecord *v1alpha1.HealthRecord + + BeforeEach(func() { + promise = createPromise(promisePath) + result, err := t.reconcileUntilCompletion(reconciler, promise, &opts{ + funcs: []func(client.Object) error{autoMarkCRDAsEstablished}, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(ctrl.Result{RequeueAfter: reconciler.ReconciliationInterval})) + + healthRecord = &v1alpha1.HealthRecord{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-health-record", + Namespace: v1alpha1.SystemNamespace, + }, + Data: v1alpha1.HealthRecordData{ + PromiseRef: v1alpha1.PromiseRef{ + Name: promise.GetName(), + }, + State: "active", + }, + } + Expect(fakeK8sClient.Create(ctx, healthRecord)).To(Succeed()) + + Expect(fakeK8sClient.Delete(ctx, promise)).To(Succeed()) + Expect(fakeK8sClient.Get(ctx, promiseName, promise)).To(Succeed()) + Expect(promise.DeletionTimestamp.IsZero()).To(BeFalse()) + }) + + It("does not delete the dependencies until health records are gone", func() { + // Reconcile multiple times to ensure we are stuck waiting + for i := 0; i < 5; i++ { + result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: promiseName}) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(ctrl.Result{RequeueAfter: 5 * time.Second})) + } + + // Check dependencies still exist + controllerResourceName := types.NamespacedName{ + Name: promise.GetControllerResourceName(), + } + clusterrole := &rbacv1.ClusterRole{} + Expect(fakeK8sClient.Get(ctx, controllerResourceName, clusterrole)).To(Succeed()) + + // Now delete the health record + Expect(fakeK8sClient.Delete(ctx, healthRecord)).To(Succeed()) + + // Reconcile again, should proceed + // Depending on how the fake client handles deletion, we might need to ensure it's actually gone from the list + // implied by subsequent reconciles. + // In a real env, GC would delete it. Here we just deleted it. + + result, err := t.reconcileUntilCompletion(reconciler, promise, &opts{ + funcs: []func(client.Object) error{autoMarkCRDAsEstablished}, + }) + Expect(err).NotTo(HaveOccurred()) + // Should eventually return empty result (or NotFound if fully deleted) + Expect(result).To(Equal(ctrl.Result{})) // Assuming completion returns zero result if successfully deleted + + err = fakeK8sClient.Get(ctx, promiseName, promise) + Expect(errors.IsNotFound(err)).To(BeTrue()) + }) + }) + }) }) func autoMarkCRDAsEstablished(obj client.Object) error {