diff --git a/internal/controller/hypervisor_maintenance_controller.go b/internal/controller/hypervisor_maintenance_controller.go index 5c824878..05ec3a27 100644 --- a/internal/controller/hypervisor_maintenance_controller.go +++ b/internal/controller/hypervisor_maintenance_controller.go @@ -24,20 +24,19 @@ 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" logger "sigs.k8s.io/controller-runtime/pkg/log" "github.com/gophercloud/gophercloud/v2" "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 +57,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 +67,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 @@ -218,33 +212,46 @@ 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) 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="" }