Add HypervisorReadyController to centralize Ready condition logic

cmd/main.go

@@ -49,6 +49,7 @@ import (
 
 	kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1"
 	"github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller"
+	"github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/controller/ready"
 	"github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/global"
 	"github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/logger"
 
@@ -316,6 +317,14 @@ func main() {
 		os.Exit(1)
 	}
 
+	if err = (&ready.Controller{
+		Client: mgr.GetClient(),
+		Scheme: mgr.GetScheme(),
+	}).SetupWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to create controller", "controller", ready.ControllerName)
+		os.Exit(1)
+	}
+
 	// +kubebuilder:scaffold:builder
 
 	if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil {

internal/controller/hypervisor_controller.go

@@ -112,18 +112,6 @@ func (hv *HypervisorController) Reconcile(ctx context.Context, req ctrl.Request)
 		nodeTerminationCondition := FindNodeStatusCondition(node.Status.Conditions, "Terminating")
 		if nodeTerminationCondition != nil && nodeTerminationCondition.Status == corev1.ConditionTrue {
 			// Node might be terminating, propagate condition to hypervisor
-
-			if readyCondition := meta.FindStatusCondition(hypervisor.Status.Conditions, kvmv1.ConditionTypeReady); readyCondition == nil || readyCondition.Status == metav1.ConditionTrue {
-				// Only set Terminating condition if Ready is still True, otherwise we might overwrite other controllers that already set Ready to False
-				// In particular if the hypervisor is evicting
-				meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
-					Type:    kvmv1.ConditionTypeReady,
-					Status:  metav1.ConditionFalse,
-					Reason:  nodeTerminationCondition.Reason,
-					Message: nodeTerminationCondition.Message,
-				})
-			}
-
 			meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
 				Type:    kvmv1.ConditionTypeTerminating,
 				Status:  metav1.ConditionStatus(nodeTerminationCondition.Status),

internal/controller/hypervisor_controller_test.go

@@ -21,7 +21,6 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -311,12 +310,7 @@ var _ = Describe("Hypervisor Controller", func() {
 				// Get the Hypervisor resource
 				updatedHypervisor := &kvmv1.Hypervisor{}
 				Expect(hypervisorController.Get(ctx, hypervisorName, updatedHypervisor)).To(Succeed())
-				Expect(updatedHypervisor.Status.Conditions).To(ContainElements(
-					SatisfyAll(
-						HaveField("Type", kvmv1.ConditionTypeReady),
-						HaveField("Reason", terminatingReason),
-						HaveField("Status", metav1.ConditionFalse),
-					),
+				Expect(updatedHypervisor.Status.Conditions).To(ContainElement(
 					SatisfyAll(
 						HaveField("Type", kvmv1.ConditionTypeTerminating),
 						HaveField("Reason", terminatingReason),
@@ -342,39 +336,6 @@ var _ = Describe("Hypervisor Controller", func() {
 				})
 			})
 
-			Context("and the Hypervisor resource already has a Ready Condition set to false", func() {
-				BeforeEach(func(ctx SpecContext) {
-					hypervisor := &kvmv1.Hypervisor{}
-					Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
-					meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
-						Type:   kvmv1.ConditionTypeReady,
-						Status: metav1.ConditionFalse,
-						Reason: "SomeOtherReason",
-					})
-					Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
-				})
-
-				It("should not update the existing Ready Condition with the new reason", func(ctx SpecContext) {
-					for range 2 {
-						_, err := hypervisorController.Reconcile(ctx, ctrl.Request{
-							NamespacedName: types.NamespacedName{Name: resource.Name},
-						})
-						Expect(err).NotTo(HaveOccurred())
-					}
-
-					// Get the Hypervisor resource again
-					updatedHypervisor := &kvmv1.Hypervisor{}
-					Expect(k8sClient.Get(ctx, hypervisorName, updatedHypervisor)).To(Succeed())
-					Expect(updatedHypervisor.Status.Conditions).To(ContainElement(
-						SatisfyAll(
-							HaveField("Type", kvmv1.ConditionTypeReady),
-							HaveField("Reason", "SomeOtherReason"),
-							HaveField("Status", metav1.ConditionFalse),
-						),
-					))
-				})
-			})
-
 			It("should successfully reconcile the terminating node", func(ctx SpecContext) {
 				By("Reconciling the created resource")
 				for range 2 {
@@ -391,12 +352,7 @@ var _ = Describe("Hypervisor Controller", func() {
 				// Not sure, if that is a good idea, but that is the current behaviour
 				// We expect another operator to set the Maintenance field to Termination
 				Expect(updatedHypervisor.Spec.Maintenance).NotTo(Equal(kvmv1.MaintenanceTermination))
-				Expect(updatedHypervisor.Status.Conditions).To(ContainElements(
-					SatisfyAll(
-						HaveField("Type", kvmv1.ConditionTypeReady),
-						HaveField("Reason", terminatingReason),
-						HaveField("Status", metav1.ConditionFalse),
-					),
+				Expect(updatedHypervisor.Status.Conditions).To(ContainElement(
 					SatisfyAll(
 						HaveField("Type", kvmv1.ConditionTypeTerminating),
 						HaveField("Reason", terminatingReason),

internal/controller/hypervisor_maintenance_controller.go

@@ -109,13 +109,6 @@ func (hec *HypervisorMaintenanceController) reconcileComputeService(ctx context.
 			return nil
 		}
 
-		meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
-			Type:    kvmv1.ConditionTypeReady,
-			Status:  metav1.ConditionTrue,
-			Reason:  kvmv1.ConditionReasonReadyReady,
-			Message: "Hypervisor is ready",
-		})
-
 		// We need to enable the host as per spec
 		enableService := services.UpdateOpts{Status: services.ServiceEnabled}
 		log.Info("Enabling hypervisor", "id", serviceId)
@@ -138,13 +131,6 @@ func (hec *HypervisorMaintenanceController) reconcileComputeService(ctx context.
 			return nil
 		}
 
-		meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
-			Type:    kvmv1.ConditionTypeReady,
-			Status:  metav1.ConditionFalse,
-			Reason:  kvmv1.ConditionReasonReadyMaintenance,
-			Message: "Hypervisor is disabled",
-		})
-
 		// We need to disable the host as per spec
 		enableService := services.UpdateOpts{
 			Status:         services.ServiceDisabled,
@@ -194,22 +180,10 @@ func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Contex
 			message = "Evicted"
 			reason = kvmv1.ConditionReasonSucceeded
 			hv.Status.Evicted = true
-			meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
-				Type:    kvmv1.ConditionTypeReady,
-				Status:  metav1.ConditionFalse,
-				Reason:  kvmv1.ConditionReasonReadyEvicted,
-				Message: "Hypervisor is disabled and evicted",
-			})
 		} else {
 			message = "Evicting"
 			reason = kvmv1.ConditionReasonRunning
 			hv.Status.Evicted = false
-			meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
-				Type:    kvmv1.ConditionTypeReady,
-				Status:  metav1.ConditionFalse,
-				Reason:  kvmv1.ConditionReasonReadyEvicting,
-				Message: "Hypervisor is disabled and evicting",
-			})
 		}
 
 		meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{

internal/controller/hypervisor_maintenance_controller_test.go

@@ -157,16 +157,6 @@ var _ = Describe("HypervisorMaintenanceController", func() {
 					Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
 					Expect(meta.IsStatusConditionFalse(updated.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled)).To(BeTrue())
 				})
-
-				It("should set the ConditionTypeReady to true", func(ctx SpecContext) {
-					updated := &kvmv1.Hypervisor{}
-					Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
-					Expect(updated.Status.Conditions).To(ContainElement(
-						SatisfyAll(
-							HaveField("Type", kvmv1.ConditionTypeReady),
-							HaveField("Status", metav1.ConditionTrue),
-						)))
-				})
 			}) // Spec.Maintenance=""
 		})
 
@@ -189,16 +179,6 @@ var _ = Describe("HypervisorMaintenanceController", func() {
 					Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
 					Expect(meta.IsStatusConditionTrue(updated.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled)).To(BeTrue())
 				})
-
-				It("should set the ConditionTypeReady to false", func(ctx SpecContext) {
-					updated := &kvmv1.Hypervisor{}
-					Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
-					Expect(updated.Status.Conditions).To(ContainElement(
-						SatisfyAll(
-							HaveField("Type", kvmv1.ConditionTypeReady),
-							HaveField("Status", metav1.ConditionFalse),
-						)))
-				})
 			}) // Spec.Maintenance="<mode>"
 		}
 
@@ -339,14 +319,13 @@ var _ = Describe("HypervisorMaintenanceController", func() {
 							Expect(hypervisor.Status.Evicted).To(BeFalse())
 						})
 
-						It("should set the ConditionTypeReady to false and reason to evicting", func(ctx SpecContext) {
+						It("should set the ConditionTypeEvicting to true", func(ctx SpecContext) {
 							updated := &kvmv1.Hypervisor{}
 							Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
 							Expect(updated.Status.Conditions).To(ContainElement(
 								SatisfyAll(
-									HaveField("Type", kvmv1.ConditionTypeReady),
-									HaveField("Status", metav1.ConditionFalse),
-									HaveField("Reason", kvmv1.ConditionReasonReadyEvicting),
+									HaveField("Type", kvmv1.ConditionTypeEvicting),
+									HaveField("Status", metav1.ConditionTrue),
 								)))
 						})
 					})
@@ -393,14 +372,13 @@ var _ = Describe("HypervisorMaintenanceController", func() {
 							Expect(hypervisor.Status.Evicted).To(BeTrue())
 						})
 
-						It("should set the ConditionTypeReady to false and reason to evicted", func(ctx SpecContext) {
+						It("should set the ConditionTypeEvicting to false when evicted", func(ctx SpecContext) {
 							updated := &kvmv1.Hypervisor{}
 							Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed())
 							Expect(updated.Status.Conditions).To(ContainElement(
 								SatisfyAll(
-									HaveField("Type", kvmv1.ConditionTypeReady),
+									HaveField("Type", kvmv1.ConditionTypeEvicting),
 									HaveField("Status", metav1.ConditionFalse),
-									HaveField("Reason", kvmv1.ConditionReasonReadyEvicted),
 								)))
 						})
 					})

internal/controller/offboarding_controller.go

@@ -68,11 +68,14 @@ func (r *HypervisorOffboardingReconciler) Reconcile(ctx context.Context, req ctr
 		return ctrl.Result{}, nil
 	}
 
-	if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeReady) {
+	// Check if offboarding has already started or completed
+	offboardedCondition := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeOffboarded)
+	if offboardedCondition == nil {
+		// Start offboarding
 		return r.setOffboardingCondition(ctx, hv, "Hypervisor is being offboarded, removing host from nova")
 	}
 
-	if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeOffboarded) {
+	if offboardedCondition.Status == metav1.ConditionTrue {
 		return ctrl.Result{}, nil
 	}
 
@@ -145,7 +148,7 @@ func (r *HypervisorOffboardingReconciler) Reconcile(ctx context.Context, req ctr
 func (r *HypervisorOffboardingReconciler) setOffboardingCondition(ctx context.Context, hv *kvmv1.Hypervisor, message string) (ctrl.Result, error) {
 	base := hv.DeepCopy()
 	meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
-		Type:    kvmv1.ConditionTypeReady,
+		Type:    kvmv1.ConditionTypeOffboarded,
 		Status:  metav1.ConditionFalse,
 		Reason:  "Offboarding",
 		Message: message,
@@ -165,12 +168,6 @@ func (r *HypervisorOffboardingReconciler) markOffboarded(ctx context.Context, hv
 		Reason:  "Offboarded",
 		Message: "Offboarding successful",
 	})
-	meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
-		Type:    kvmv1.ConditionTypeReady,
-		Status:  metav1.ConditionFalse,
-		Reason:  "Offboarded",
-		Message: "Offboarding successful",
-	})
 	if err := r.Status().Patch(ctx, hv, k8sclient.MergeFromWithOptions(base,
 		k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(OffboardingControllerName)); err != nil {
 		return fmt.Errorf("cannot update hypervisor status due to %w", err)

internal/controller/offboarding_controller_test.go

@@ -175,15 +175,15 @@ var _ = Describe("Offboarding Controller", func() {
 					})
 				})
 
-				It("should set the hypervisor ready condition", func(ctx SpecContext) {
+				It("should set the hypervisor offboarding condition", func(ctx SpecContext) {
 					_, err := offboardingReconciler.Reconcile(ctx, reconcileReq)
 					Expect(err).NotTo(HaveOccurred())
 
 					hypervisor := &kvmv1.Hypervisor{}
 					Expect(k8sClient.Get(ctx, resourceName, hypervisor)).To(Succeed())
 					Expect(hypervisor.Status.Conditions).To(ContainElement(
 						SatisfyAll(
-							HaveField("Type", kvmv1.ConditionTypeReady),
+							HaveField("Type", kvmv1.ConditionTypeOffboarded),
 							HaveField("Status", metav1.ConditionFalse),
 							HaveField("Reason", "Offboarding"),
 						),
@@ -199,19 +199,13 @@ var _ = Describe("Offboarding Controller", func() {
 
 					hypervisor := &kvmv1.Hypervisor{}
 					Expect(k8sClient.Get(ctx, resourceName, hypervisor)).To(Succeed())
-					Expect(hypervisor.Status.Conditions).To(ContainElements(
+					Expect(hypervisor.Status.Conditions).To(ContainElement(
 						SatisfyAll(
 							HaveField("Type", kvmv1.ConditionTypeOffboarded),
 							HaveField("Status", metav1.ConditionTrue),
 							HaveField("Reason", "Offboarded"),
 							HaveField("Message", "Offboarding successful"),
 						),
-						SatisfyAll(
-							HaveField("Type", kvmv1.ConditionTypeReady),
-							HaveField("Status", metav1.ConditionFalse),
-							HaveField("Reason", "Offboarded"),
-							HaveField("Message", "Offboarding successful"),
-						),
 					))
 				})
 
@@ -296,7 +290,7 @@ var _ = Describe("Offboarding Controller", func() {
 			Expect(k8sClient.Get(ctx, resourceName, hypervisor)).To(Succeed())
 			Expect(hypervisor.Status.Conditions).To(ContainElement(
 				SatisfyAll(
-					HaveField("Type", kvmv1.ConditionTypeReady),
+					HaveField("Type", kvmv1.ConditionTypeOffboarded),
 					HaveField("Status", metav1.ConditionFalse),
 					HaveField("Reason", "Offboarding"),
 					HaveField("Message", ContainSubstring(expectedMessageSubstring)),
@@ -310,6 +304,15 @@ var _ = Describe("Offboarding Controller", func() {
 			Expect(k8sClient.Get(ctx, resourceName, hypervisor)).To(Succeed())
 			hypervisor.Spec.Maintenance = kvmv1.MaintenanceTermination
 			Expect(k8sClient.Update(ctx, hypervisor)).To(Succeed())
+
+			By("Setting initial offboarding condition")
+			meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
+				Type:    kvmv1.ConditionTypeOffboarded,
+				Status:  metav1.ConditionFalse,
+				Reason:  "Offboarding",
+				Message: "Hypervisor is being offboarded, removing host from nova",
+			})
+			Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
 		})
 
 		Context("When getting hypervisor by name fails", func() {

internal/controller/onboarding_controller.go

@@ -110,12 +110,6 @@ func (r *OnboardingController) Reconcile(ctx context.Context, req ctrl.Request)
 	status := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding)
 	if status == nil {
 		base := hv.DeepCopy()
-		meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
-			Type:    kvmv1.ConditionTypeReady,
-			Status:  metav1.ConditionFalse,
-			Reason:  kvmv1.ConditionReasonOnboarding,
-			Message: "Onboarding in progress",
-		})
 
 		meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
 			Type:    kvmv1.ConditionTypeOnboarding,
@@ -151,18 +145,6 @@ func (r *OnboardingController) abortOnboarding(ctx context.Context, hv *kvmv1.Hy
 	}
 
 	base := hv.DeepCopy()
-	ready := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeReady)
-	if ready != nil {
-		// Only undo ones own readiness status reporting
-		if ready.Reason == kvmv1.ConditionReasonOnboarding {
-			meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
-				Type:    kvmv1.ConditionTypeReady,
-				Status:  metav1.ConditionFalse,
-				Reason:  kvmv1.ConditionReasonOnboarding,
-				Message: "Onboarding aborted",
-			})
-		}
-	}
 
 	meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
 		Type:    kvmv1.ConditionTypeOnboarding,
@@ -354,13 +336,6 @@ func (r *OnboardingController) completeOnboarding(ctx context.Context, host stri
 			Message: "Onboarding completed",
 		})
 
-		meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
-			Type:    kvmv1.ConditionTypeReady,
-			Status:  metav1.ConditionTrue,
-			Reason:  kvmv1.ConditionReasonReadyReady,
-			Message: "Hypervisor is ready",
-		})
-
 		return ctrl.Result{}, r.patchStatus(ctx, hv, base)
 	}