From 46ad384889af2833aafdb063f790f5728a615bb3 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Mon, 13 Jul 2026 14:56:25 +0200 Subject: [PATCH 1/2] Migrate HypervisorMaintenanceController to SSA status updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace PatchHypervisorStatusWithRetry with Status().Apply() for ConditionTypeHypervisorDisabled, ConditionTypeEvicting, and the Evicted scalar. ConditionTypeEvicting is removed by omitting it from the apply — SSA prunes map-list entries no longer claimed by the sole owner. The status config is built once in Reconcile and passed to reconcileComputeService and reconcileEviction which mutate it directly. This eliminates intermediate condition return values and makes the single-apply-per-reconcile explicit. The early-return guard for already-enabled/disabled state is inlined into each branch. Seed only the conditions owned by this controller so SSA does not claim ownership over conditions written by others. For the delete-by-omission case, owned conditions are seeded selectively so the target condition is simply absent from the payload. Also clear the forced_down flag when enabling a compute service so an earlier HA event that set forced_down=true does not leave the service reachable but treated as down. SetupWithManager now delegates to registerWithManager instead of inlining the same ctrl.NewControllerManagedBy call, which had left registerWithManager unreachable. --- .../hypervisor_maintenance_controller.go | 177 +++++++++--------- .../hypervisor_maintenance_controller_test.go | 23 +++ 2 files changed, 109 insertions(+), 91 deletions(-) diff --git a/internal/controller/hypervisor_maintenance_controller.go b/internal/controller/hypervisor_maintenance_controller.go index 5c824878..9eefa887 100644 --- a/internal/controller/hypervisor_maintenance_controller.go +++ b/internal/controller/hypervisor_maintenance_controller.go @@ -24,11 +24,11 @@ import ( "context" "fmt" - "k8s.io/apimachinery/pkg/api/equality" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + k8sacmetav1 "k8s.io/client-go/applyconfigurations/meta/v1" ctrl "sigs.k8s.io/controller-runtime" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -38,6 +38,7 @@ import ( "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/services" kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" + apiv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/applyconfigurations/api/v1" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/openstack" "github.com/cobaltcore-dev/openstack-hypervisor-operator/internal/utils" ) @@ -58,7 +59,6 @@ type HypervisorMaintenanceController struct { func (hec *HypervisorMaintenanceController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { hv := &kvmv1.Hypervisor{} if err := hec.Get(ctx, req.NamespacedName, hv); err != nil { - // OnboardingReconciler not found errors, could be deleted return ctrl.Result{}, k8sclient.IgnoreNotFound(err) } @@ -69,148 +69,144 @@ func (hec *HypervisorMaintenanceController) Reconcile(ctx context.Context, req c return ctrl.Result{}, nil } - old := hv.DeepCopy() - - if err := hec.reconcileComputeService(ctx, hv); err != nil { - return ctrl.Result{}, err + // Build status apply config upfront; sub-functions mutate it directly. + // Seed only the HypervisorDisabled condition (the only one always retained). + // reconcileEviction conditionally seeds ConditionTypeEvicting: it is included + // when maintenance is active, and intentionally omitted when MaintenanceUnset + // so that SSA prunes it from the field manager's managed fields. + statusCfg := apiv1.HypervisorStatus().WithEvicted(hv.Status.Evicted) + if c := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled); c != nil { + statusCfg.WithConditions(utils.ConditionFromStatus(*c)) } - if err := hec.reconcileEviction(ctx, hv); err != nil { + if err := hec.reconcileComputeService(ctx, hv, statusCfg); err != nil { return ctrl.Result{}, err } - if equality.Semantic.DeepEqual(hv, old) { - return ctrl.Result{}, nil + if err := hec.reconcileEviction(ctx, hv, statusCfg); err != nil { + return ctrl.Result{}, err } - // Capture only the fields this controller owns - disabledCondition := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled) - evictingCondition := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting) - evicted := hv.Status.Evicted - - return ctrl.Result{}, utils.PatchHypervisorStatusWithRetry(ctx, hec.Client, hv.Name, HypervisorMaintenanceControllerName, func(h *kvmv1.Hypervisor) { - if disabledCondition != nil { - meta.SetStatusCondition(&h.Status.Conditions, *disabledCondition) - } - if evictingCondition != nil { - meta.SetStatusCondition(&h.Status.Conditions, *evictingCondition) - } else { - meta.RemoveStatusCondition(&h.Status.Conditions, kvmv1.ConditionTypeEvicting) - } - h.Status.Evicted = evicted - }) + return ctrl.Result{}, hec.Status().Apply(ctx, + apiv1.Hypervisor(hv.Name).WithStatus(statusCfg), + k8sclient.ForceOwnership, k8sclient.FieldOwner(HypervisorMaintenanceControllerName)) } -func (hec *HypervisorMaintenanceController) reconcileComputeService(ctx context.Context, hv *kvmv1.Hypervisor) error { +// reconcileComputeService enables/disables the nova-compute service based on +// hv.Spec.Maintenance and sets the HypervisorDisabled condition on statusCfg. +func (hec *HypervisorMaintenanceController) reconcileComputeService(ctx context.Context, hv *kvmv1.Hypervisor, statusCfg *apiv1.HypervisorStatusApplyConfiguration) error { log := logger.FromContext(ctx) serviceId := hv.Status.ServiceID - // We can only do something here, if there is a service to begin with. - // The onboarding should take care of that if serviceId == "" { + // We can only do something here, if there is a service to begin with. + // The onboarding should take care of that. return nil } switch hv.Spec.Maintenance { case kvmv1.MaintenanceUnset: - // Enable the compute service (in case we haven't done so already) - if !meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeHypervisorDisabled, - Status: metav1.ConditionFalse, - Message: "Hypervisor is enabled", - Reason: kvmv1.ConditionReasonSucceeded, - }) { - // Spec matches status - return nil + existing := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled) + if existing == nil || existing.Status != metav1.ConditionFalse { + // We need to enable the host as per spec. + // Also clear forced_down in case a previous HA event set it. + falseVal := false + enableService := openstack.UpdateServiceOpts{ + Status: services.ServiceEnabled, + ForcedDown: &falseVal, + } + log.Info("Enabling hypervisor", "id", serviceId) + if _, err := services.Update(ctx, hec.computeClient, serviceId, enableService).Extract(); err != nil { + return fmt.Errorf("failed to enable hypervisor due to %w", err) + } } + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeHypervisorDisabled). + WithStatus(metav1.ConditionFalse). + WithMessage("Hypervisor is enabled"). + WithReason(kvmv1.ConditionReasonSucceeded)) - // We need to enable the host as per spec and clear forced_down - // in case it was set by the HA service during maintenance. - falseVal := false - enableService := openstack.UpdateServiceOpts{ - Status: services.ServiceEnabled, - ForcedDown: &falseVal, - } - log.Info("Enabling hypervisor", "id", serviceId) - _, err := services.Update(ctx, hec.computeClient, serviceId, enableService).Extract() - if err != nil { - return fmt.Errorf("failed to enable hypervisor due to %w", err) - } case kvmv1.MaintenanceManual, kvmv1.MaintenanceAuto, kvmv1.MaintenanceTermination: // Disable the compute service. - if !meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeHypervisorDisabled, - Status: metav1.ConditionTrue, - Message: "Hypervisor is disabled", - Reason: kvmv1.ConditionReasonSucceeded, - }) { - // Spec matches status - return nil - } - - // We need to disable the host as per spec - enableService := services.UpdateOpts{ - Status: services.ServiceDisabled, - DisabledReason: "Hypervisor CRD: spec.maintenance=" + hv.Spec.Maintenance, - } - log.Info("Disabling hypervisor", "id", serviceId) - _, err := services.Update(ctx, hec.computeClient, serviceId, enableService).Extract() - if err != nil { - return fmt.Errorf("failed to disable hypervisor due to %w", err) + existing := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled) + if existing == nil || existing.Status != metav1.ConditionTrue { + disableService := services.UpdateOpts{ + Status: services.ServiceDisabled, + DisabledReason: "Hypervisor CRD: spec.maintenance=" + hv.Spec.Maintenance, + } + // We need to disable the host as per spec + log.Info("Disabling hypervisor", "id", serviceId) + if _, err := services.Update(ctx, hec.computeClient, serviceId, disableService).Extract(); err != nil { + return fmt.Errorf("failed to disable hypervisor due to %w", err) + } } + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeHypervisorDisabled). + WithStatus(metav1.ConditionTrue). + WithMessage("Hypervisor is disabled"). + WithReason(kvmv1.ConditionReasonSucceeded)) } return nil } -func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Context, hv *kvmv1.Hypervisor) error { +// reconcileEviction creates/deletes the Eviction CR and sets the ConditionTypeEvicting +// condition and Evicted scalar on statusCfg. When eviction should be removed, the +// condition entry is filtered out so SSA prunes it. +func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Context, hv *kvmv1.Hypervisor, statusCfg *apiv1.HypervisorStatusApplyConfiguration) error { eviction := &kvmv1.Eviction{ - ObjectMeta: metav1.ObjectMeta{ - Name: hv.Name, - }, + ObjectMeta: metav1.ObjectMeta{Name: hv.Name}, } switch hv.Spec.Maintenance { case kvmv1.MaintenanceUnset: // Avoid deleting the eviction over and over. - if hv.Status.Evicted || meta.RemoveStatusCondition(&hv.Status.Conditions, kvmv1.ConditionTypeEvicting) { - err := k8sclient.IgnoreNotFound(hec.Delete(ctx, eviction)) - hv.Status.Evicted = false + if !hv.Status.Evicted && meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting) == nil { + return nil + } + if err := k8sclient.IgnoreNotFound(hec.Delete(ctx, eviction)); err != nil { return err } - return nil + // ConditionTypeEvicting is intentionally absent from statusCfg — SSA + // will prune it from this field manager's managed fields on Apply. + statusCfg.WithEvicted(false) + case kvmv1.MaintenanceManual, kvmv1.MaintenanceAuto, kvmv1.MaintenanceTermination: - // In case of "ha", the host gets emptied from the HA service + // In case of "ha", the host gets emptied from the HA service. + // Seed the existing evicting condition so SSA does not prune it, + // regardless of whether we take the short-circuit below. if cond := meta.FindStatusCondition(hv.Status.Conditions, kvmv1.ConditionTypeEvicting); cond != nil { + statusCfg.WithConditions(utils.ConditionFromStatus(*cond)) if cond.Reason == kvmv1.ConditionReasonSucceeded { - // We are done here, no need to look at the eviction any more + // We are done here, no need to look at the eviction any more. return nil } } + status, err := hec.ensureEviction(ctx, eviction, hv) if err != nil { return err } - var reason, message string + var reason, message string if status == metav1.ConditionFalse { message = "Evicted" reason = kvmv1.ConditionReasonSucceeded - hv.Status.Evicted = true + statusCfg.WithEvicted(true) } else { message = "Evicting" reason = kvmv1.ConditionReasonRunning - hv.Status.Evicted = false + statusCfg.WithEvicted(false) } - meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeEvicting, - Status: status, - Reason: reason, - Message: message, - }) - - return nil + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeEvicting). + WithStatus(status). + WithReason(reason). + WithMessage(message)) } return nil @@ -242,9 +238,8 @@ func (hec *HypervisorMaintenanceController) ensureEviction(ctx context.Context, // check if we are still evicting (defaulting to yes) if meta.IsStatusConditionFalse(eviction.Status.Conditions, kvmv1.ConditionTypeEvicting) { return metav1.ConditionFalse, nil - } else { - return metav1.ConditionTrue, nil } + return metav1.ConditionTrue, nil } // registerWithManager registers the controller with the Manager without acquiring OpenStack clients. diff --git a/internal/controller/hypervisor_maintenance_controller_test.go b/internal/controller/hypervisor_maintenance_controller_test.go index bdbe6159..4af542b8 100644 --- a/internal/controller/hypervisor_maintenance_controller_test.go +++ b/internal/controller/hypervisor_maintenance_controller_test.go @@ -396,6 +396,29 @@ var _ = Describe("HypervisorMaintenanceController", func() { HaveField("Status", metav1.ConditionFalse), ))) }) + + It("should keep the evicting condition on a repeated reconcile (must not be pruned by SSA)", func(ctx SpecContext) { + // Reconciling again with the succeeded state already + // recorded on the Hypervisor takes the early-return + // branch in reconcileEviction. The apply must still + // seed the succeeded evicting condition — otherwise + // SSA prunes it because this controller is its sole + // owner. + req := ctrl.Request{NamespacedName: hypervisorName} + _, err := controller.Reconcile(ctx, req) + Expect(err).NotTo(HaveOccurred()) + + updated := &kvmv1.Hypervisor{} + Expect(k8sClient.Get(ctx, hypervisorName, updated)).To(Succeed()) + Expect(updated.Status.Conditions).To(ContainElement( + SatisfyAll( + HaveField("Type", kvmv1.ConditionTypeEvicting), + HaveField("Status", metav1.ConditionFalse), + HaveField("Reason", kvmv1.ConditionReasonSucceeded), + ), + )) + Expect(updated.Status.Evicted).To(BeTrue()) + }) }) }) // Spec.Maintenance="" } From 1adbd01b41a7b6ce0df1c0ade40794379112c272 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Mon, 13 Jul 2026 14:57:32 +0200 Subject: [PATCH 2/2] Migrate HypervisorMaintenanceController Eviction creation to SSA Replace Create + SetControllerReference with Apply for the Eviction CR. The owner reference and labels are set in the apply configuration metadata, and SSA handles the upsert. A Get is still performed after the apply to read the current eviction status conditions. The apply configuration constructor for the cluster-scoped Eviction resource takes only a name (no namespace). --- .../hypervisor_maintenance_controller.go | 48 ++++++++++++------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/internal/controller/hypervisor_maintenance_controller.go b/internal/controller/hypervisor_maintenance_controller.go index 9eefa887..05ec3a27 100644 --- a/internal/controller/hypervisor_maintenance_controller.go +++ b/internal/controller/hypervisor_maintenance_controller.go @@ -24,14 +24,12 @@ import ( "context" "fmt" - k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" k8sacmetav1 "k8s.io/client-go/applyconfigurations/meta/v1" ctrl "sigs.k8s.io/controller-runtime" k8sclient "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" logger "sigs.k8s.io/controller-runtime/pkg/log" "github.com/gophercloud/gophercloud/v2" @@ -214,25 +212,39 @@ func (hec *HypervisorMaintenanceController) reconcileEviction(ctx context.Contex func (hec *HypervisorMaintenanceController) ensureEviction(ctx context.Context, eviction *kvmv1.Eviction, hypervisor *kvmv1.Hypervisor) (metav1.ConditionStatus, error) { log := logger.FromContext(ctx) - if err := hec.Get(ctx, k8sclient.ObjectKeyFromObject(eviction), eviction); err != nil { - if !k8serrors.IsNotFound(err) { - return metav1.ConditionUnknown, fmt.Errorf("failed to get eviction due to %w", err) - } - if err := controllerutil.SetControllerReference(hypervisor, eviction, hec.Scheme); err != nil { - return metav1.ConditionUnknown, err - } - log.Info("Creating new eviction", "name", eviction.Name) - eviction.Spec = kvmv1.EvictionSpec{ - Hypervisor: hypervisor.Name, - Reason: "openstack-hypervisor-operator maintenance", + + // Build labels to transport from hypervisor (e.g. label-selector, if set) + evictionLabels := make(map[string]string) + for _, label := range transferLabels { + if v, ok := hypervisor.Labels[label]; ok { + evictionLabels[label] = v } + } - // This also transports the label-selector, if set - transportLabels(&hypervisor.ObjectMeta, &eviction.ObjectMeta) + ownerRef := k8sacmetav1.OwnerReference(). + WithAPIVersion(kvmv1.GroupVersion.String()). + WithKind("Hypervisor"). + WithName(hypervisor.Name). + WithUID(hypervisor.UID). + WithController(true). + WithBlockOwnerDeletion(true) + + evictionApplyCfg := apiv1.Eviction(eviction.Name). + WithLabels(evictionLabels). + WithOwnerReferences(ownerRef). + WithSpec(apiv1.EvictionSpec(). + WithHypervisor(hypervisor.Name). + WithReason("openstack-hypervisor-operator maintenance")) + + log.Info("Applying eviction", "name", eviction.Name) + if err := hec.Apply(ctx, evictionApplyCfg, + k8sclient.ForceOwnership, k8sclient.FieldOwner(HypervisorMaintenanceControllerName)); err != nil { + return metav1.ConditionUnknown, fmt.Errorf("failed to apply eviction due to %w", err) + } - if err = hec.Create(ctx, eviction); err != nil { - return metav1.ConditionUnknown, fmt.Errorf("failed to create eviction due to %w", err) - } + // Re-fetch to read current eviction status + if err := hec.Get(ctx, k8sclient.ObjectKeyFromObject(eviction), eviction); err != nil { + return metav1.ConditionUnknown, fmt.Errorf("failed to get eviction status due to %w", err) } // check if we are still evicting (defaulting to yes)