Fix servermaintenance deletion logic

Signed-off-by: Nagadeesh Nagaraja <nagdeesh@gmail.com>

Author: nagadeesh-nagaraja

internal/controller/servermaintenance_controller.go

@@ -343,16 +343,25 @@ func (r *ServerMaintenanceReconciler) handleFailedState(ctx context.Context, _ *
 func (r *ServerMaintenanceReconciler) delete(ctx context.Context, maintenance *metalv1alpha1.ServerMaintenance) (ctrl.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
 	log.V(1).Info("Deleting ServerMaintenance")
+	if !controllerutil.ContainsFinalizer(maintenance, serverMaintenanceFinalizer) {
+		return ctrl.Result{}, nil
+	}
 	if maintenance.Spec.ServerRef == nil {
 		return ctrl.Result{}, nil
 	}
 	server, err := GetServerByName(ctx, r.Client, maintenance.Spec.ServerRef.Name)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-	if err := r.cleanup(ctx, server); err != nil {
+	if err == nil {
+		if err := r.cleanup(ctx, server); err != nil {
+			return ctrl.Result{}, err
+		}
+	} else if apierrors.IsNotFound(err) {
+		// note: if the server is already deleted, we can skip the cleanup and just remove the finalizer
+		// if its transent error of server not found, the servermaintenance will be deleted and server will free up the ref automatically once this is deleted
+		log.V(1).Info("Server not found, continue with cleanup", "Server", maintenance.Spec.ServerRef.Name)
+	} else {
 		return ctrl.Result{}, err
 	}
+
 	log.V(1).Info("Removed dependencies")
 
 	log.V(1).Info("Ensuring that the finalizer is removed")

internal/controller/servermaintenance_controller_test.go

@@ -541,4 +541,72 @@ var _ = Describe("ServerMaintenance Controller", func() {
 		Expect(k8sClient.Delete(ctx, unsetPriorityMaintenance)).To(Succeed())
 		Expect(k8sClient.Delete(ctx, serverClaim)).To(Succeed())
 	})
+
+	It("should complete deletion when the referenced Server is already gone", func(ctx SpecContext) {
+		By("Creating a ServerMaintenance object")
+		serverMaintenance := &metalv1alpha1.ServerMaintenance{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-server-maintenance-server-gone",
+				Namespace: ns.Name,
+			},
+			Spec: metalv1alpha1.ServerMaintenanceSpec{
+				ServerRef:   &corev1.LocalObjectReference{Name: server.Name},
+				Policy:      metalv1alpha1.ServerMaintenancePolicyEnforced,
+				ServerPower: metalv1alpha1.PowerOff,
+			},
+		}
+		Expect(k8sClient.Create(ctx, serverMaintenance)).To(Succeed())
+
+		By("Waiting for the ServerMaintenance to reach InMaintenance state")
+		Eventually(Object(serverMaintenance)).Should(
+			HaveField("Status.State", metalv1alpha1.ServerMaintenanceStateInMaintenance),
+		)
+
+		By("Deleting the Server before deleting the ServerMaintenance")
+		Expect(k8sClient.Delete(ctx, server)).To(Succeed())
+		Eventually(Get(server)).ShouldNot(Succeed())
+
+		By("Deleting the ServerMaintenance")
+		Expect(k8sClient.Delete(ctx, serverMaintenance)).To(Succeed())
+
+		By("Ensuring the ServerMaintenance is fully deleted despite the Server being gone")
+		Eventually(Get(serverMaintenance)).ShouldNot(Succeed())
+	})
+
+	It("should skip cleanup and remove finalizer when no finalizer is present on deletion", func(ctx SpecContext) {
+		By("Creating a ServerMaintenance object without going through reconciliation (no finalizer)")
+		serverMaintenance := &metalv1alpha1.ServerMaintenance{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test-server-maintenance-no-finalizer",
+				Namespace: ns.Name,
+			},
+			Spec: metalv1alpha1.ServerMaintenanceSpec{
+				ServerRef:   &corev1.LocalObjectReference{Name: server.Name},
+				Policy:      metalv1alpha1.ServerMaintenancePolicyEnforced,
+				ServerPower: metalv1alpha1.PowerOff,
+			},
+		}
+		Expect(k8sClient.Create(ctx, serverMaintenance)).To(Succeed())
+
+		By("Waiting for the finalizer to be added by the reconciler")
+		Eventually(Object(serverMaintenance)).Should(
+			HaveField("Finalizers", ContainElement(serverMaintenanceFinalizer)),
+		)
+
+		By("Manually removing the finalizer to simulate a no-finalizer state")
+		Eventually(Update(serverMaintenance, func() {
+			serverMaintenance.Finalizers = nil
+		})).Should(Succeed())
+
+		By("Deleting the ServerMaintenance")
+		Expect(k8sClient.Delete(ctx, serverMaintenance)).To(Succeed())
+
+		By("Ensuring the ServerMaintenance is deleted immediately without cleanup side-effects")
+		Eventually(Get(serverMaintenance)).ShouldNot(Succeed())
+
+		By("Ensuring the Server was not modified during deletion")
+		Consistently(Object(server)).Should(
+			HaveField("Spec.ServerMaintenanceRef", BeNil()),
+		)
+	})
 })