From bb578c5dfb28c7cb3b171c9974a970cded013491 Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Mon, 13 Jul 2026 14:46:32 +0200 Subject: [PATCH] Migrate EvictionReconciler to SSA status updates Replace MergeFromWithOptimisticLock + retry with Status().Apply() for all eviction status updates. The evictionStatusCfg helper seeds every apply with the full set of owned fields so SSA never prunes values between reconcile cycles. As the sole owner of every condition on the Eviction CR, the reconciler seeds all three condition types on each apply. The apply configuration constructor for the cluster-scoped Eviction resource takes only a name (no namespace). --- internal/controller/eviction_controller.go | 309 +++++++++++---------- 1 file changed, 160 insertions(+), 149 deletions(-) diff --git a/internal/controller/eviction_controller.go b/internal/controller/eviction_controller.go index b0fa33c9..b1ecd78e 100644 --- a/internal/controller/eviction_controller.go +++ b/internal/controller/eviction_controller.go @@ -32,12 +32,13 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/util/retry" + k8sacmetav1 "k8s.io/client-go/applyconfigurations/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" logger "sigs.k8s.io/controller-runtime/pkg/log" 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" ) @@ -85,6 +86,32 @@ func moveToBack(instances []string) []string { return instances } +// evictionStatusCfg builds an EvictionStatusApplyConfiguration pre-populated +// with the current scalar fields and the conditions this controller owns. +// Call sites upsert the single condition they are setting via +// utils.SetApplyConfigurationStatusCondition, then apply. +func evictionStatusCfg(eviction *kvmv1.Eviction) *apiv1.EvictionStatusApplyConfiguration { + cfg := apiv1.EvictionStatus(). + WithHypervisorServiceId(eviction.Status.HypervisorServiceId). + WithOutstandingRamMb(eviction.Status.OutstandingRamMb) + cfg.OutstandingInstances = eviction.Status.OutstandingInstances + // Seed all conditions this controller owns so they are not pruned on Apply. + for _, t := range []string{kvmv1.ConditionTypeEvicting, kvmv1.ConditionTypePreflight, kvmv1.ConditionTypeMigration} { + if c := meta.FindStatusCondition(eviction.Status.Conditions, t); c != nil { + cfg.WithConditions(utils.ConditionFromStatus(*c)) + } + } + return cfg +} + +// applyStatus applies the eviction status via SSA with this controller as the +// field owner. +func (r *EvictionReconciler) applyStatus(ctx context.Context, name string, cfg *apiv1.EvictionStatusApplyConfiguration) error { + return r.Status().Apply(ctx, + apiv1.Eviction(name).WithStatus(cfg), + client.ForceOwnership, client.FieldOwner(EvictionControllerName)) +} + // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions/status,verbs=get;update;patch // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions/finalizers,verbs=update @@ -118,14 +145,14 @@ func (r *EvictionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c statusCondition := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypeEvicting) if statusCondition == nil { - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeEvicting, - Status: metav1.ConditionTrue, - Message: "Running", - Reason: kvmv1.ConditionReasonRunning, - }) - - return ctrl.Result{}, r.updateStatus(ctx, eviction) + statusCfg := evictionStatusCfg(eviction) + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeEvicting). + WithStatus(metav1.ConditionTrue). + WithReason(kvmv1.ConditionReasonRunning). + WithMessage("Running")) + return ctrl.Result{}, r.applyStatus(ctx, eviction.Name, statusCfg) } switch statusCondition.Status { @@ -157,36 +184,16 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1. return r.evictNext(ctx, eviction) } - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeEvicting, - Status: metav1.ConditionFalse, - Message: "eviction completed successfully", - Reason: kvmv1.ConditionReasonSucceeded, - }) - - eviction.Status.OutstandingRamMb = 0 logger.FromContext(ctx).Info("succeeded") - return ctrl.Result{}, r.updateStatus(ctx, eviction) -} - -func (r *EvictionReconciler) updateStatus(ctx context.Context, eviction *kvmv1.Eviction) error { - desiredStatus := eviction.Status.DeepCopy() - return retry.RetryOnConflict(utils.StatusPatchBackoff, func() error { - freshEviction := &kvmv1.Eviction{} - if err := r.Get(ctx, client.ObjectKeyFromObject(eviction), freshEviction); err != nil { - return err - } - freshBase := freshEviction.DeepCopy() - // Apply desired conditions and scalar fields onto the fresh status - for _, c := range desiredStatus.Conditions { - meta.SetStatusCondition(&freshEviction.Status.Conditions, c) - } - freshEviction.Status.OutstandingInstances = desiredStatus.OutstandingInstances - freshEviction.Status.OutstandingRamMb = desiredStatus.OutstandingRamMb - freshEviction.Status.HypervisorServiceId = desiredStatus.HypervisorServiceId - return r.Status().Patch(ctx, freshEviction, client.MergeFromWithOptions(freshBase, - client.MergeFromWithOptimisticLock{}), client.FieldOwner(EvictionControllerName)) - }) + statusCfg := evictionStatusCfg(eviction) + statusCfg.WithOutstandingRamMb(0) + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeEvicting). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonSucceeded). + WithMessage("eviction completed successfully")) + return ctrl.Result{}, r.applyStatus(ctx, eviction.Name, statusCfg) } func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv1.Eviction, hv *kvmv1.Hypervisor) (ctrl.Result, error) { @@ -194,14 +201,20 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv // If the hypervisor should exist, then we need to ensure it is disabled before we start evicting if expectHypervisor && !meta.IsStatusConditionTrue(hv.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled) { - // Hypervisor is not disabled (yet?), reflect that as a failing preflight check - if meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypePreflight, - Status: metav1.ConditionFalse, - Message: "hypervisor not disabled", - Reason: kvmv1.ConditionReasonFailed, - }) { - return ctrl.Result{}, r.updateStatus(ctx, eviction) + // Hypervisor is not disabled (yet?), reflect that as a failing preflight check. + // Only apply if the condition is new or changed, to avoid unnecessary API calls on repeated requeues. + existing := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypePreflight) + if existing == nil || existing.Status != metav1.ConditionFalse { + statusCfg := evictionStatusCfg(eviction) + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypePreflight). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonFailed). + WithMessage("hypervisor not disabled")) + if err := r.applyStatus(ctx, eviction.Name, statusCfg); err != nil { + return ctrl.Result{}, err + } } return ctrl.Result{RequeueAfter: defaultPollTime}, nil // Wait for hypervisor to be disabled } @@ -216,47 +229,50 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv if expectHypervisor { // Abort eviction - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeEvicting, - Status: metav1.ConditionFalse, - Message: fmt.Sprintf("failed to get hypervisor %v", err), - Reason: kvmv1.ConditionReasonFailed, - }) - return ctrl.Result{}, r.updateStatus(ctx, eviction) - } else { - // That is (likely) an eviction for a node that never registered - // so we are good to go - msg := "eviction completed successfully due to expected case of no hypervisor" - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeEvicting, - Status: metav1.ConditionFalse, - Message: msg, - Reason: kvmv1.ConditionReasonSucceeded, - }) - eviction.Status.OutstandingRamMb = 0 - logger.FromContext(ctx).Info(msg) - return ctrl.Result{}, r.updateStatus(ctx, eviction) + statusCfg := evictionStatusCfg(eviction) + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeEvicting). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonFailed). + WithMessage(fmt.Sprintf("failed to get hypervisor %v", err))) + return ctrl.Result{}, r.applyStatus(ctx, eviction.Name, statusCfg) } + + // That is (likely) an eviction for a node that never registered + // so we are good to go + msg := "eviction completed successfully due to expected case of no hypervisor" + logger.FromContext(ctx).Info(msg) + statusCfg := evictionStatusCfg(eviction) + statusCfg.WithOutstandingRamMb(0) + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeEvicting). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonSucceeded). + WithMessage(msg)) + return ctrl.Result{}, r.applyStatus(ctx, eviction.Name, statusCfg) } + var instances []string if hypervisor.Servers != nil { - uuids := make([]string, len(*hypervisor.Servers)) + instances = make([]string, len(*hypervisor.Servers)) for i, server := range *hypervisor.Servers { - uuids[i] = server.UUID + instances[i] = server.UUID } - eviction.Status.OutstandingInstances = uuids } - // Update status - eviction.Status.HypervisorServiceId = hypervisor.ID - eviction.Status.OutstandingRamMb = int64(hypervisor.MemoryMBUsed) - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypePreflight, - Status: metav1.ConditionTrue, - Message: "Preflight checks passed, hypervisor is disabled and ready for eviction", - Reason: kvmv1.ConditionReasonSucceeded, - }) - return ctrl.Result{}, r.updateStatus(ctx, eviction) + statusCfg := evictionStatusCfg(eviction) + statusCfg.WithHypervisorServiceId(hypervisor.ID) + statusCfg.WithOutstandingRamMb(int64(hypervisor.MemoryMBUsed)) + statusCfg.OutstandingInstances = instances + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypePreflight). + WithStatus(metav1.ConditionTrue). + WithReason(kvmv1.ConditionReasonSucceeded). + WithMessage("Preflight checks passed, hypervisor is disabled and ready for eviction")) + return ctrl.Result{}, r.applyStatus(ctx, eviction.Name, statusCfg) } // Tries to handle the NotFound-error by updating the status @@ -265,18 +281,20 @@ func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction *kvmv1 return err } logger.FromContext(ctx).Info("Instance is gone") - var uuid string - eviction.Status.OutstandingInstances, uuid = popInstance(eviction.Status.OutstandingInstances) + remaining, uuid := popInstance(eviction.Status.OutstandingInstances) if uuid == "" { return nil } - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeMigration, - Status: metav1.ConditionFalse, - Message: fmt.Sprintf("Instance %s is gone", uuid), - Reason: kvmv1.ConditionReasonSucceeded, - }) - return r.updateStatus(ctx, eviction) + + statusCfg := evictionStatusCfg(eviction) + statusCfg.OutstandingInstances = remaining + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeMigration). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonSucceeded). + WithMessage(fmt.Sprintf("Instance %s is gone", uuid))) + return r.applyStatus(ctx, eviction.Name, statusCfg) } func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Eviction) (ctrl.Result, error) { @@ -309,41 +327,25 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic case "ERROR": // Needs manual intervention (or another operator fixes it) // put it at the end of the list (beginning of array) - eviction.Status.OutstandingInstances = moveToBack(eviction.Status.OutstandingInstances) + reordered := moveToBack(eviction.Status.OutstandingInstances) log.Info("error", "faultMessage", vm.Fault.Message) - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeMigration, - Status: metav1.ConditionFalse, - Message: fmt.Sprintf("Migration of instance %s failed: %s", vm.ID, vm.Fault.Message), - Reason: kvmv1.ConditionReasonFailed, - }) - - return ctrl.Result{}, errors.Join(fmt.Errorf("error migrating instance %v", uuid), - r.updateStatus(ctx, eviction)) + + statusCfg := evictionStatusCfg(eviction) + statusCfg.OutstandingInstances = reordered + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeMigration). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonFailed). + WithMessage(fmt.Sprintf("Migration of instance %s failed: %s", vm.ID, vm.Fault.Message))) + applyErr := r.applyStatus(ctx, eviction.Name, statusCfg) + return ctrl.Result{}, errors.Join(fmt.Errorf("error migrating instance %v", uuid), applyErr) } currentHypervisor, _, _ := strings.Cut(vm.HypervisorHostname, ".") if currentHypervisor != eviction.Spec.Hypervisor { log.Info("migrated") - // Don't overwrite a sticky Failed migration condition with Succeeded - // while there are still other outstanding VMs - an earlier ERROR VM - // has been moved to the back of the queue and the eviction is not - // actually clean. The condition is reset only when the whole - // eviction completes (OutstandingInstances becomes empty). - remaining, _ := popInstance(eviction.Status.OutstandingInstances) - prior := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypeMigration) - stickyFailure := len(remaining) > 0 && prior != nil && - prior.Status == metav1.ConditionFalse && - prior.Reason == kvmv1.ConditionReasonFailed - if !stickyFailure { - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeMigration, - Status: metav1.ConditionFalse, - Message: fmt.Sprintf("Migration of instance %s finished", vm.ID), - Reason: kvmv1.ConditionReasonSucceeded, - }) - } // So, it is already off this one, do we need to verify it? if vm.Status == "VERIFY_RESIZE" { @@ -358,27 +360,50 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic } // All done - eviction.Status.OutstandingInstances, _ = popInstance(eviction.Status.OutstandingInstances) - return ctrl.Result{}, r.updateStatus(ctx, eviction) + popped, _ := popInstance(eviction.Status.OutstandingInstances) + + // Don't overwrite a sticky Failed migration condition with Succeeded + // while there are still other outstanding VMs - an earlier ERROR VM + // has been moved to the back of the queue and the eviction is not + // actually clean. The condition is reset only when the whole + // eviction completes (OutstandingInstances becomes empty). + prior := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypeMigration) + stickyFailure := len(popped) > 0 && prior != nil && + prior.Status == metav1.ConditionFalse && + prior.Reason == kvmv1.ConditionReasonFailed + + statusCfg := evictionStatusCfg(eviction) + statusCfg.OutstandingInstances = popped + if !stickyFailure { + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeMigration). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonSucceeded). + WithMessage(fmt.Sprintf("Migration of instance %s finished", vm.ID))) + } + return ctrl.Result{}, r.applyStatus(ctx, eviction.Name, statusCfg) } if vm.TaskState == "deleting" { //nolint:gocritic // We just have to wait for it to be gone. Try the next one. - eviction.Status.OutstandingInstances = moveToBack(eviction.Status.OutstandingInstances) - - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeMigration, - Status: metav1.ConditionFalse, - Message: fmt.Sprintf("Live migration of terminating instance %s skipped", vm.ID), - Reason: kvmv1.ConditionReasonFailed, - }) - if err := r.updateStatus(ctx, eviction); err != nil { + reordered := moveToBack(eviction.Status.OutstandingInstances) + + statusCfg := evictionStatusCfg(eviction) + statusCfg.OutstandingInstances = reordered + utils.SetApplyConfigurationStatusCondition(&statusCfg.Conditions, + *k8sacmetav1.Condition(). + WithType(kvmv1.ConditionTypeMigration). + WithStatus(metav1.ConditionFalse). + WithReason(kvmv1.ConditionReasonFailed). + WithMessage(fmt.Sprintf("Live migration of terminating instance %s skipped", vm.ID))) + if err := r.applyStatus(ctx, eviction.Name, statusCfg); err != nil { return ctrl.Result{}, fmt.Errorf("could not update status due to %w", err) } return ctrl.Result{RequeueAfter: defaultPollTime}, nil } else if vm.Status == "ACTIVE" || vm.PowerState == 1 { log.Info("trigger live-migration") - if err := r.liveMigrate(ctx, vm.ID, eviction); err != nil { + if err := r.liveMigrate(ctx, vm.ID, eviction.Spec.Hypervisor); err != nil { if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { return ctrl.Result{}, err2 } @@ -386,7 +411,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic } } else { log.Info("trigger cold-migration") - if err := r.coldMigrate(ctx, vm.ID, eviction); err != nil { + if err := r.coldMigrate(ctx, vm.ID, eviction.Spec.Hypervisor); err != nil { if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil { return ctrl.Result{}, err2 } @@ -400,7 +425,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic return ctrl.Result{RequeueAfter: defaultPollTime}, nil } -func (r *EvictionReconciler) liveMigrate(ctx context.Context, uuid string, eviction *kvmv1.Eviction) error { +func (r *EvictionReconciler) liveMigrate(ctx context.Context, uuid, hypervisorName string) error { log := logger.FromContext(ctx) liveMigrateOpts := servers.LiveMigrateOpts{ @@ -409,36 +434,22 @@ func (r *EvictionReconciler) liveMigrate(ctx context.Context, uuid string, evict res := servers.LiveMigrate(ctx, r.computeClient, uuid, liveMigrateOpts) if res.Err != nil { - err := fmt.Errorf("failed to evict VM %s due to %w", uuid, res.Err) - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeMigration, - Status: metav1.ConditionFalse, - Message: err.Error(), - Reason: kvmv1.ConditionReasonFailed, - }) - return err + return fmt.Errorf("failed to evict VM %s due to %w", uuid, res.Err) } - log.Info("Live migrating server", "server", uuid, "source", eviction.Spec.Hypervisor, "X-Openstack-Request-Id", res.Header.Get("X-Openstack-Request-Id")) + log.Info("Live migrating server", "server", uuid, "source", hypervisorName, "X-Openstack-Request-Id", res.Header.Get("X-Openstack-Request-Id")) return nil } -func (r *EvictionReconciler) coldMigrate(ctx context.Context, uuid string, eviction *kvmv1.Eviction) error { +func (r *EvictionReconciler) coldMigrate(ctx context.Context, uuid, hypervisorName string) error { log := logger.FromContext(ctx) res := servers.Migrate(ctx, r.computeClient, uuid) if res.Err != nil { - err := fmt.Errorf("failed to evict stopped server %s due to %w", uuid, res.Err) - meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{ - Type: kvmv1.ConditionTypeMigration, - Status: metav1.ConditionFalse, - Message: err.Error(), - Reason: kvmv1.ConditionReasonFailed, - }) - return err + return fmt.Errorf("failed to evict stopped server %s due to %w", uuid, res.Err) } - log.Info("Cold-migrating server", "server", uuid, "source", eviction.Spec.Hypervisor, "X-Openstack-Request-Id", res.Header.Get("X-Openstack-Request-Id")) + log.Info("Cold-migrating server", "server", uuid, "source", hypervisorName, "X-Openstack-Request-Id", res.Header.Get("X-Openstack-Request-Id")) return nil }