From b5addc7e7aaa0bf9243cfe6b3ef4eb90c4a4c54c Mon Sep 17 00:00:00 2001 From: Andrew Karpow Date: Wed, 8 Apr 2026 15:33:18 -0400 Subject: [PATCH 1/2] Add HypervisorReadyController to centralize Ready condition logic Introduces a new ReadyController that computes the Ready condition based on other conditions (Onboarding, Offboarded, Terminating, Evicting) and the maintenance spec field. Changes: - Add internal/controller/ready package with controller and tests - Remove Ready condition setting from OnboardingController - Remove Ready condition setting from MaintenanceController - Remove Ready condition setting from OffboardingController - Remove Ready condition setting from HypervisorController - Update OffboardingController to use Offboarded condition for state tracking The ReadyController uses a predicate that triggers on: - Status changes - Maintenance field changes (since it affects Ready condition) Ready condition priority: 1. Offboarded=True -> Ready=False 2. Terminating=True -> Ready=False 3. Onboarding in progress (Status=True) -> Ready=False 4. Maintenance mode (with eviction state) -> Ready=False 5. Onboarding not started/aborted/not succeeded -> Ready=False 6. All checks pass -> Ready=True --- cmd/main.go | 9 + internal/controller/hypervisor_controller.go | 12 - .../hypervisor_maintenance_controller.go | 26 -- internal/controller/offboarding_controller.go | 15 +- internal/controller/onboarding_controller.go | 25 -- internal/controller/ready/controller.go | 225 ++++++++++++ internal/controller/ready/controller_test.go | 336 ++++++++++++++++++ 7 files changed, 576 insertions(+), 72 deletions(-) create mode 100644 internal/controller/ready/controller.go create mode 100644 internal/controller/ready/controller_test.go diff --git a/cmd/main.go b/cmd/main.go index a9fe3a3..fecb731 100644 --- a/cmd/main.go +++ b/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 { diff --git a/internal/controller/hypervisor_controller.go b/internal/controller/hypervisor_controller.go index e9f62b0..0456959 100644 --- a/internal/controller/hypervisor_controller.go +++ b/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), diff --git a/internal/controller/hypervisor_maintenance_controller.go b/internal/controller/hypervisor_maintenance_controller.go index eeaee0a..f4662ee 100644 --- a/internal/controller/hypervisor_maintenance_controller.go +++ b/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{ diff --git a/internal/controller/offboarding_controller.go b/internal/controller/offboarding_controller.go index 8790dfd..14336b4 100644 --- a/internal/controller/offboarding_controller.go +++ b/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) diff --git a/internal/controller/onboarding_controller.go b/internal/controller/onboarding_controller.go index faac00a..cc6bad6 100644 --- a/internal/controller/onboarding_controller.go +++ b/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) } diff --git a/internal/controller/ready/controller.go b/internal/controller/ready/controller.go new file mode 100644 index 0000000..878e8ca --- /dev/null +++ b/internal/controller/ready/controller.go @@ -0,0 +1,225 @@ +/* +SPDX-FileCopyrightText: Copyright 2024 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package ready + +import ( + "context" + + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + k8sclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + logger "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" + + kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" +) + +const ( + ControllerName = "ready" +) + +// StatusChangedPredicate triggers reconciliation only when the status subresource changes. +// This is the inverse of GenerationChangedPredicate which triggers on spec changes. +type StatusChangedPredicate struct{} + +var _ predicate.Predicate = StatusChangedPredicate{} + +func (StatusChangedPredicate) Create(_ event.CreateEvent) bool { + // Reconcile on create to compute initial Ready condition + return true +} + +func (StatusChangedPredicate) Delete(_ event.DeleteEvent) bool { + // No need to reconcile on delete + return false +} + +func (StatusChangedPredicate) Update(e event.UpdateEvent) bool { + if e.ObjectOld == nil || e.ObjectNew == nil { + return false + } + + oldHv, ok := e.ObjectOld.(*kvmv1.Hypervisor) + if !ok { + return false + } + newHv, ok := e.ObjectNew.(*kvmv1.Hypervisor) + if !ok { + return false + } + + // Trigger if status changed + if !equality.Semantic.DeepEqual(oldHv.Status, newHv.Status) { + return true + } + + // Also trigger if Maintenance field changed (affects Ready condition) + if oldHv.Spec.Maintenance != newHv.Spec.Maintenance { + return true + } + + return false +} + +func (StatusChangedPredicate) Generic(_ event.GenericEvent) bool { + return true +} + +// Controller reconciles the Ready condition based on other conditions +type Controller struct { + k8sclient.Client + Scheme *runtime.Scheme +} + +// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors,verbs=get;list;watch +// +kubebuilder:rbac:groups=kvm.cloud.sap,resources=hypervisors/status,verbs=get;list;watch;patch + +func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + log := logger.FromContext(ctx).WithName(req.Name) + ctx = logger.IntoContext(ctx, log) + + hv := &kvmv1.Hypervisor{} + if err := r.Get(ctx, req.NamespacedName, hv); err != nil { + return ctrl.Result{}, k8sclient.IgnoreNotFound(err) + } + + base := hv.DeepCopy() + + // Compute Ready condition based on other conditions + readyCondition := ComputeReadyCondition(hv) + meta.SetStatusCondition(&hv.Status.Conditions, readyCondition) + + if equality.Semantic.DeepEqual(hv.Status, base.Status) { + return ctrl.Result{}, nil + } + + log.Info("Updating Ready condition", "status", readyCondition.Status, "reason", readyCondition.Reason) + return ctrl.Result{}, r.Status().Patch(ctx, hv, k8sclient.MergeFromWithOptions(base, + k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(ControllerName)) +} + +// ComputeReadyCondition determines the Ready condition based on other conditions +func ComputeReadyCondition(hv *kvmv1.Hypervisor) metav1.Condition { + // Priority 1: Offboarded + if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeOffboarded) { + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: "Offboarded", + Message: "Hypervisor has been offboarded", + } + } + + // Priority 2: Terminating + if meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeTerminating) { + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonTerminating, + Message: "Hypervisor is terminating", + } + } + + // Priority 3: Active onboarding (Status=True means onboarding is in progress) + onboardingCondition := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeOnboarding) + if onboardingCondition != nil && onboardingCondition.Status == metav1.ConditionTrue { + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonOnboarding, + Message: "Onboarding in progress: " + onboardingCondition.Message, + } + } + + // Priority 4: Maintenance mode + if hv.Spec.Maintenance != kvmv1.MaintenanceUnset { + evictingCondition := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting) + if evictingCondition != nil { + if evictingCondition.Status == metav1.ConditionTrue { + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonReadyEvicting, + Message: "Hypervisor is disabled and evicting", + } + } + if evictingCondition.Status == metav1.ConditionFalse { + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonReadyEvicted, + Message: "Hypervisor is disabled and evicted", + } + } + } + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonReadyMaintenance, + Message: "Hypervisor is in maintenance mode", + } + } + + // Priority 5: Onboarding not started, aborted, or not succeeded + if onboardingCondition == nil { + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonOnboarding, + Message: "Onboarding not started", + } + } + + if onboardingCondition.Reason == kvmv1.ConditionReasonAborted { + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonOnboarding, + Message: "Onboarding was aborted", + } + } + + if onboardingCondition.Reason != kvmv1.ConditionReasonSucceeded { + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonOnboarding, + Message: "Onboarding not yet completed", + } + } + + // Priority 6: All checks passed - Ready + return metav1.Condition{ + Type: kvmv1.ConditionTypeReady, + Status: metav1.ConditionTrue, + Reason: kvmv1.ConditionReasonReadyReady, + Message: "Hypervisor is ready", + } +} + +func (r *Controller) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + Named(ControllerName). + For(&kvmv1.Hypervisor{}, builder.WithPredicates(StatusChangedPredicate{})). + Complete(r) +} diff --git a/internal/controller/ready/controller_test.go b/internal/controller/ready/controller_test.go new file mode 100644 index 0000000..2a00f87 --- /dev/null +++ b/internal/controller/ready/controller_test.go @@ -0,0 +1,336 @@ +/* +SPDX-FileCopyrightText: Copyright 2024 SAP SE or an SAP affiliate company and cobaltcore-dev contributors +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package ready + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" + + kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" +) + +func TestReadyController(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Ready Controller Suite") +} + +var _ = Describe("StatusChangedPredicate", func() { + var predicate StatusChangedPredicate + + BeforeEach(func() { + predicate = StatusChangedPredicate{} + }) + + Describe("Create", func() { + It("should return true for create events", func() { + e := event.CreateEvent{Object: &kvmv1.Hypervisor{}} + Expect(predicate.Create(e)).To(BeTrue()) + }) + }) + + Describe("Delete", func() { + It("should return false for delete events", func() { + e := event.DeleteEvent{Object: &kvmv1.Hypervisor{}} + Expect(predicate.Delete(e)).To(BeFalse()) + }) + }) + + Describe("Update", func() { + It("should return false when only non-maintenance spec changes", func() { + oldHv := &kvmv1.Hypervisor{ + Spec: kvmv1.HypervisorSpec{SkipTests: false}, + } + newHv := &kvmv1.Hypervisor{ + Spec: kvmv1.HypervisorSpec{SkipTests: true}, + } + e := event.UpdateEvent{ObjectOld: oldHv, ObjectNew: newHv} + Expect(predicate.Update(e)).To(BeFalse()) + }) + + It("should return true when status changes", func() { + oldHv := &kvmv1.Hypervisor{} + newHv := &kvmv1.Hypervisor{ + Status: kvmv1.HypervisorStatus{ + Conditions: []metav1.Condition{ + {Type: kvmv1.ConditionTypeOnboarding, Status: metav1.ConditionTrue}, + }, + }, + } + e := event.UpdateEvent{ObjectOld: oldHv, ObjectNew: newHv} + Expect(predicate.Update(e)).To(BeTrue()) + }) + + It("should return true when maintenance field changes", func() { + oldHv := &kvmv1.Hypervisor{ + Spec: kvmv1.HypervisorSpec{Maintenance: kvmv1.MaintenanceUnset}, + } + newHv := &kvmv1.Hypervisor{ + Spec: kvmv1.HypervisorSpec{Maintenance: kvmv1.MaintenanceManual}, + } + e := event.UpdateEvent{ObjectOld: oldHv, ObjectNew: newHv} + Expect(predicate.Update(e)).To(BeTrue()) + }) + + It("should return false when nothing changes", func() { + hv := &kvmv1.Hypervisor{ + Spec: kvmv1.HypervisorSpec{SkipTests: true}, + } + e := event.UpdateEvent{ObjectOld: hv, ObjectNew: hv} + Expect(predicate.Update(e)).To(BeFalse()) + }) + + It("should return false when ObjectOld is nil", func() { + e := event.UpdateEvent{ObjectOld: nil, ObjectNew: &kvmv1.Hypervisor{}} + Expect(predicate.Update(e)).To(BeFalse()) + }) + + It("should return false when ObjectNew is nil", func() { + e := event.UpdateEvent{ObjectOld: &kvmv1.Hypervisor{}, ObjectNew: nil} + Expect(predicate.Update(e)).To(BeFalse()) + }) + }) + + Describe("Generic", func() { + It("should return true for generic events", func() { + e := event.GenericEvent{Object: &kvmv1.Hypervisor{}} + Expect(predicate.Generic(e)).To(BeTrue()) + }) + }) +}) + +var _ = Describe("ComputeReadyCondition", func() { + var hv *kvmv1.Hypervisor + + BeforeEach(func() { + hv = &kvmv1.Hypervisor{} + }) + + Context("Priority 1: Offboarded", func() { + It("should return Ready=False with Reason=Offboarded when offboarded", func() { + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOffboarded, + Status: metav1.ConditionTrue, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionFalse)) + Expect(result.Reason).To(Equal("Offboarded")) + }) + }) + + Context("Priority 2: Terminating", func() { + It("should return Ready=False with Reason=Terminating when terminating", func() { + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeTerminating, + Status: metav1.ConditionTrue, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionFalse)) + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonTerminating)) + }) + + It("should prioritize Offboarded over Terminating", func() { + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOffboarded, + Status: metav1.ConditionTrue, + }) + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeTerminating, + Status: metav1.ConditionTrue, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Reason).To(Equal("Offboarded")) + }) + }) + + Context("Priority 3: Onboarding in progress", func() { + It("should return Ready=False when onboarding is in progress", func() { + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionTrue, + Reason: kvmv1.ConditionReasonTesting, + Message: "Running smoke tests", + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionFalse)) + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonOnboarding)) + Expect(result.Message).To(ContainSubstring("in progress")) + }) + + It("should prioritize onboarding in progress over maintenance", func() { + hv.Spec.Maintenance = kvmv1.MaintenanceManual + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionTrue, + Reason: kvmv1.ConditionReasonTesting, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonOnboarding)) + Expect(result.Message).To(ContainSubstring("in progress")) + }) + }) + + Context("Priority 4: Maintenance mode", func() { + It("should return Ready=False with Reason=Evicting when evicting", func() { + hv.Spec.Maintenance = kvmv1.MaintenanceAuto + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeEvicting, + Status: metav1.ConditionTrue, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionFalse)) + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonReadyEvicting)) + }) + + It("should return Ready=False with Reason=Evicted when evicted", func() { + hv.Spec.Maintenance = kvmv1.MaintenanceAuto + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeEvicting, + Status: metav1.ConditionFalse, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionFalse)) + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonReadyEvicted)) + }) + + It("should return Ready=False with Reason=Maintenance when in maintenance without eviction", func() { + hv.Spec.Maintenance = kvmv1.MaintenanceManual + + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionFalse)) + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonReadyMaintenance)) + }) + + It("should prioritize maintenance over onboarding not started", func() { + hv.Spec.Maintenance = kvmv1.MaintenanceManual + // No onboarding condition set + + result := ComputeReadyCondition(hv) + + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonReadyMaintenance)) + }) + + It("should prioritize maintenance over onboarding aborted", func() { + hv.Spec.Maintenance = kvmv1.MaintenanceManual + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonAborted, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonReadyMaintenance)) + }) + + It("should prioritize maintenance over onboarding not succeeded", func() { + hv.Spec.Maintenance = kvmv1.MaintenanceManual + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonFailed, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonReadyMaintenance)) + }) + }) + + Context("Priority 5: Onboarding not started/aborted/not succeeded", func() { + It("should return Ready=False when no onboarding condition exists", func() { + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionFalse)) + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonOnboarding)) + Expect(result.Message).To(ContainSubstring("not started")) + }) + + It("should return Ready=False when onboarding was aborted", func() { + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonAborted, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionFalse)) + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonOnboarding)) + Expect(result.Message).To(ContainSubstring("aborted")) + }) + + It("should return Ready=False when onboarding has non-succeeded reason", func() { + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonFailed, + }) + + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionFalse)) + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonOnboarding)) + }) + }) + + Context("Priority 6: All checks pass - Ready", func() { + It("should return Ready=True when all conditions are satisfied", func() { + meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + Type: kvmv1.ConditionTypeOnboarding, + Status: metav1.ConditionFalse, + Reason: kvmv1.ConditionReasonSucceeded, + }) + hv.Spec.Maintenance = kvmv1.MaintenanceUnset + + result := ComputeReadyCondition(hv) + + Expect(result.Type).To(Equal(kvmv1.ConditionTypeReady)) + Expect(result.Status).To(Equal(metav1.ConditionTrue)) + Expect(result.Reason).To(Equal(kvmv1.ConditionReasonReadyReady)) + }) + }) +}) From 69f6395362f11f3a0073ee9148bbd08e2ff5abf7 Mon Sep 17 00:00:00 2001 From: Andrew Karpow Date: Wed, 8 Apr 2026 16:32:39 -0400 Subject: [PATCH 2/2] Fix tests to not expect Ready condition from individual controllers Since the ReadyController now owns the Ready condition, update tests for maintenance, offboarding, onboarding, and hypervisor controllers to only check for the conditions those controllers actually set (Evicting, Offboarded, Onboarding, Terminating) rather than the computed Ready condition. - Remove tests checking Ready condition from maintenance controller - Update offboarding tests to check Offboarded condition instead - Remove Ready condition checks from onboarding controller tests - Remove test that checked Ready condition preservation in hypervisor controller (now handled by ReadyController) - Set initial Offboarded condition in failure mode tests so reconcile hits the error paths --- .../controller/hypervisor_controller_test.go | 48 +------------------ .../hypervisor_maintenance_controller_test.go | 32 ++----------- .../controller/offboarding_controller_test.go | 23 +++++---- .../controller/onboarding_controller_test.go | 31 ++---------- 4 files changed, 24 insertions(+), 110 deletions(-) diff --git a/internal/controller/hypervisor_controller_test.go b/internal/controller/hypervisor_controller_test.go index 44a0f49..9e1657b 100644 --- a/internal/controller/hypervisor_controller_test.go +++ b/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), diff --git a/internal/controller/hypervisor_maintenance_controller_test.go b/internal/controller/hypervisor_maintenance_controller_test.go index da8f947..8609217 100644 --- a/internal/controller/hypervisor_maintenance_controller_test.go +++ b/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="" } @@ -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), ))) }) }) diff --git a/internal/controller/offboarding_controller_test.go b/internal/controller/offboarding_controller_test.go index 49107e0..06c38c4 100644 --- a/internal/controller/offboarding_controller_test.go +++ b/internal/controller/offboarding_controller_test.go @@ -175,7 +175,7 @@ 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()) @@ -183,7 +183,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"), ), @@ -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() { diff --git a/internal/controller/onboarding_controller_test.go b/internal/controller/onboarding_controller_test.go index 962d66e..8c0003d 100644 --- a/internal/controller/onboarding_controller_test.go +++ b/internal/controller/onboarding_controller_test.go @@ -282,12 +282,7 @@ var _ = Describe("Onboarding Controller", func() { Expect(err).NotTo(HaveOccurred()) Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) - Expect(hv.Status.Conditions).To(ContainElements( - SatisfyAll( - HaveField("Type", kvmv1.ConditionTypeReady), - HaveField("Status", metav1.ConditionFalse), - HaveField("Reason", kvmv1.ConditionReasonOnboarding), - ), + Expect(hv.Status.Conditions).To(ContainElement( SatisfyAll( HaveField("Type", kvmv1.ConditionTypeOnboarding), HaveField("Status", metav1.ConditionTrue), @@ -327,12 +322,7 @@ var _ = Describe("Onboarding Controller", func() { Expect(err).NotTo(HaveOccurred()) Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) - Expect(hv.Status.Conditions).To(ContainElements( - SatisfyAll( - HaveField("Type", kvmv1.ConditionTypeReady), - HaveField("Status", metav1.ConditionFalse), - HaveField("Reason", kvmv1.ConditionReasonOnboarding), - ), + Expect(hv.Status.Conditions).To(ContainElement( SatisfyAll( HaveField("Type", kvmv1.ConditionTypeOnboarding), HaveField("Status", metav1.ConditionTrue), @@ -355,11 +345,6 @@ var _ = Describe("Onboarding Controller", func() { Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) hv.Status.HypervisorID = hypervisorId hv.Status.ServiceID = serviceId - meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeReady, - Status: metav1.ConditionFalse, - Reason: kvmv1.ConditionReasonOnboarding, - }) meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ Type: kvmv1.ConditionTypeOnboarding, Status: metav1.ConditionTrue, @@ -503,11 +488,7 @@ var _ = Describe("Onboarding Controller", func() { By("Verifying final state") Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) - Expect(hv.Status.Conditions).To(ContainElements( - SatisfyAll( - HaveField("Type", kvmv1.ConditionTypeReady), - HaveField("Status", metav1.ConditionTrue), - ), + Expect(hv.Status.Conditions).To(ContainElement( SatisfyAll( HaveField("Type", kvmv1.ConditionTypeOnboarding), HaveField("Status", metav1.ConditionFalse), @@ -576,11 +557,7 @@ var _ = Describe("Onboarding Controller", func() { Expect(err).NotTo(HaveOccurred()) Expect(k8sClient.Get(ctx, namespacedName, hv)).To(Succeed()) - Expect(hv.Status.Conditions).To(ContainElements( - SatisfyAll( - HaveField("Type", kvmv1.ConditionTypeReady), - HaveField("Status", metav1.ConditionTrue), - ), + Expect(hv.Status.Conditions).To(ContainElement( SatisfyAll( HaveField("Type", kvmv1.ConditionTypeOnboarding), HaveField("Status", metav1.ConditionFalse),