Patch and field owners

internal/controller/aggregates_controller.go

@@ -114,14 +114,16 @@ func (ac *AggregatesController) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, err
 	}
 
+	base := hv.DeepCopy()
 	hv.Status.Aggregates = hv.Spec.Aggregates
 	meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
 		Type:    kvmv1.ConditionTypeAggregatesUpdated,
 		Status:  metav1.ConditionTrue,
 		Reason:  kvmv1.ConditionReasonSucceeded,
 		Message: "Aggregates updated successfully",
 	})
-	return ctrl.Result{}, ac.Status().Update(ctx, hv)
+	return ctrl.Result{}, ac.Status().Patch(ctx, hv, k8sclient.MergeFromWithOptions(base,
+		k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(AggregatesControllerName))
 }
 
 // setErrorCondition sets the error condition on the Hypervisor status, returns error if update fails
@@ -133,8 +135,10 @@ func (ac *AggregatesController) setErrorCondition(ctx context.Context, hv *kvmv1
 		Message: msg,
 	}
 
+	base := hv.DeepCopy()
 	if meta.SetStatusCondition(&hv.Status.Conditions, condition) {
-		if err := ac.Status().Update(ctx, hv); err != nil {
+		if err := ac.Status().Patch(ctx, hv, k8sclient.MergeFromWithOptions(base,
+			k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(AggregatesControllerName)); err != nil {
 			return err
 		}
 	}

internal/controller/decomission_controller.go

@@ -179,18 +179,21 @@ func (r *NodeDecommissionReconciler) removeFinalizer(ctx context.Context, node *
 
 	nodeBase := node.DeepCopy()
 	controllerutil.RemoveFinalizer(node, decommissionFinalizerName)
-	err := r.Patch(ctx, node, k8sclient.MergeFromWithOptions(nodeBase, k8sclient.MergeFromWithOptimisticLock{}))
+	err := r.Patch(ctx, node, k8sclient.MergeFromWithOptions(nodeBase,
+		k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(DecommissionControllerName))
 	return ctrl.Result{}, err
 }
 
 func (r *NodeDecommissionReconciler) setDecommissioningCondition(ctx context.Context, hv *kvmv1.Hypervisor, message string) (ctrl.Result, error) {
+	base := hv.DeepCopy()
 	meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{
 		Type:    kvmv1.ConditionTypeReady,
 		Status:  metav1.ConditionFalse,
 		Reason:  "Decommissioning",
 		Message: message,
 	})
-	if err := r.Status().Update(ctx, hv); err != nil {
+	if err := r.Status().Patch(ctx, hv, k8sclient.MergeFromWithOptions(base,
+		k8sclient.MergeFromWithOptimisticLock{}), k8sclient.FieldOwner(DecommissionControllerName)); err != nil {
 		return ctrl.Result{}, fmt.Errorf("cannot update hypervisor status due to %w", err)
 	}
 	return ctrl.Result{}, nil

internal/controller/eviction_controller.go

@@ -100,14 +100,15 @@ func (r *EvictionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 
 	statusCondition := meta.FindStatusCondition(eviction.Status.Conditions, kvmv1.ConditionTypeEvicting)
 	if statusCondition == nil {
-		// No status condition, so we need to add it
-		if r.addCondition(ctx, eviction, metav1.ConditionTrue, "Running", kvmv1.ConditionReasonRunning) {
-			log.Info("running")
-			return ctrl.Result{}, nil
-		}
-		// We just checked if the condition is there, so this should never
-		// be reached, but let's cover our bass
-		return ctrl.Result{RequeueAfter: 1 * time.Second}, nil
+		base := eviction.DeepCopy()
+		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, base)
 	}
 
 	switch statusCondition.Status {
@@ -139,6 +140,7 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1.
 		return r.evictNext(ctx, eviction)
 	}
 
+	base := eviction.DeepCopy()
 	meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 		Type:    kvmv1.ConditionTypeEvicting,
 		Status:  metav1.ConditionFalse,
@@ -148,10 +150,16 @@ func (r *EvictionReconciler) handleRunning(ctx context.Context, eviction *kvmv1.
 
 	eviction.Status.OutstandingRamMb = 0
 	logger.FromContext(ctx).Info("succeeded")
-	return ctrl.Result{}, r.Status().Update(ctx, eviction)
+	return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
+}
+
+func (r *EvictionReconciler) updateStatus(ctx context.Context, eviction, base *kvmv1.Eviction) error {
+	return r.Status().Patch(ctx, eviction, client.MergeFromWithOptions(base,
+		client.MergeFromWithOptimisticLock{}), client.FieldOwner(EvictionControllerName))
 }
 
 func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv1.Eviction) (ctrl.Result, error) {
+	base := eviction.DeepCopy()
 	hypervisorName := eviction.Spec.Hypervisor
 
 	// Does the hypervisor even exist? Is it enabled/disabled?
@@ -176,7 +184,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv
 				Message: err.Error(),
 				Reason:  kvmv1.ConditionReasonFailed,
 			})
-			return ctrl.Result{}, r.Status().Update(ctx, eviction)
+			return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
 		} else {
 			// That is (likely) an eviction for a node that never registered
 			// so we are good to go
@@ -189,7 +197,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv
 			})
 			eviction.Status.OutstandingRamMb = 0
 			logger.FromContext(ctx).Info(msg)
-			return ctrl.Result{}, r.Status().Update(ctx, eviction)
+			return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
 		}
 	}
 
@@ -204,7 +212,7 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv
 			Message: err.Error(),
 			Reason:  kvmv1.ConditionReasonFailed,
 		})
-		return ctrl.Result{}, r.Status().Update(ctx, eviction)
+		return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
 	}
 
 	if !meta.IsStatusConditionTrue(eviction.Status.Conditions, kvmv1.ConditionTypeHypervisorDisabled) {
@@ -235,15 +243,18 @@ func (r *EvictionReconciler) handlePreflight(ctx context.Context, eviction *kvmv
 		Message: "Preflight checks passed, hypervisor is disabled and ready for eviction",
 		Reason:  kvmv1.ConditionReasonSucceeded,
 	})
-	return ctrl.Result{}, r.Status().Update(ctx, eviction)
+	return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
 }
 
 // Tries to handle the NotFound-error by updating the status
-func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction *kvmv1.Eviction, err error) error {
+func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction, base *kvmv1.Eviction, err error) error {
 	if !gophercloud.ResponseCodeIs(err, http.StatusNotFound) {
 		return err
 	}
 	logger.FromContext(ctx).Info("Instance is gone")
+	if base == nil {
+		base = eviction.DeepCopy()
+	}
 	instances := &eviction.Status.OutstandingInstances
 	uuid := (*instances)[len(*instances)-1]
 	*instances = (*instances)[:len(*instances)-1]
@@ -253,10 +264,11 @@ func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction *kvmv1
 		Message: fmt.Sprintf("Instance %s is gone", uuid),
 		Reason:  kvmv1.ConditionReasonSucceeded,
 	})
-	return r.Status().Update(ctx, eviction)
+	return r.updateStatus(ctx, eviction, base)
 }
 
 func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Eviction) (ctrl.Result, error) {
+	base := eviction.DeepCopy()
 	instances := &eviction.Status.OutstandingInstances
 	uuid := (*instances)[len(*instances)-1]
 	log := logger.FromContext(ctx).WithName("Evict").WithValues("server", uuid)
@@ -266,7 +278,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
 	vm, err := res.Extract()
 
 	if err != nil {
-		if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil {
+		if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil {
 			return ctrl.Result{}, err2
 		} else {
 			return ctrl.Result{RequeueAfter: shortRetryTime}, nil
@@ -293,7 +305,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
 			Message: fmt.Sprintf("Migration of instance %s failed: %s", vm.ID, vm.Fault.Message),
 			Reason:  kvmv1.ConditionReasonFailed,
 		})
-		if err := r.Status().Update(ctx, eviction); err != nil {
+		if err := r.updateStatus(ctx, eviction, base); err != nil {
 			return ctrl.Result{}, err
 		}
 
@@ -314,7 +326,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
 		// So, it is already off this one, do we need to verify it?
 		if vm.Status == "VERIFY_RESIZE" {
 			err := servers.ConfirmResize(ctx, r.computeClient, vm.ID).ExtractErr()
-			if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil {
+			if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil {
 				// Retry confirm in next reconciliation
 				return ctrl.Result{}, err2
 			} else {
@@ -325,7 +337,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
 
 		// All done
 		*instances = (*instances)[:len(*instances)-1]
-		return ctrl.Result{}, r.Status().Update(ctx, eviction)
+		return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
 	}
 
 	if vm.TaskState == "deleting" { //nolint:gocritic
@@ -339,14 +351,14 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
 			Message: fmt.Sprintf("Live migration of terminating instance %s skipped", vm.ID),
 			Reason:  kvmv1.ConditionReasonFailed,
 		})
-		if err2 := r.Status().Update(ctx, eviction); err2 != nil {
+		if err2 := r.updateStatus(ctx, eviction, base); err2 != nil {
 			return ctrl.Result{}, fmt.Errorf("could update status due to %w", err2)
 		}
 	} else if vm.Status == "ACTIVE" || vm.PowerState == 1 {
 		log.Info("trigger live-migration")
 		err := r.liveMigrate(ctx, vm.ID, eviction)
 		if err != nil {
-			if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil {
+			if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil {
 				return ctrl.Result{}, err2
 			} else {
 				return ctrl.Result{RequeueAfter: shortRetryTime}, nil
@@ -356,7 +368,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
 		log.Info("trigger cold-migration")
 		err := r.coldMigrate(ctx, vm.ID, eviction)
 		if err != nil {
-			if err2 := r.handleNotFound(ctx, eviction, err); err2 != nil {
+			if err2 := r.handleNotFound(ctx, eviction, base, err); err2 != nil {
 				return ctrl.Result{}, err2
 			} else {
 				return ctrl.Result{RequeueAfter: shortRetryTime}, nil
@@ -389,9 +401,9 @@ func (r *EvictionReconciler) handleFinalizer(ctx context.Context, eviction *kvmv
 		}
 	}
 
-	evictionBase := eviction.DeepCopy()
+	base := eviction.DeepCopy()
 	controllerutil.RemoveFinalizer(eviction, evictionFinalizerName)
-	return r.Patch(ctx, eviction, client.MergeFromWithOptions(evictionBase, client.MergeFromWithOptimisticLock{}))
+	return r.Patch(ctx, eviction, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{}))
 }
 
 func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, eviction *kvmv1.Eviction) error {
@@ -400,18 +412,20 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
 	hypervisor, err := openstack.GetHypervisorByName(ctx, r.computeClient, eviction.Spec.Hypervisor, false)
 	if err != nil {
 		if errors.Is(err, openstack.ErrNoHypervisor) {
+			base := eviction.DeepCopy()
 			changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 				Type:    kvmv1.ConditionTypeHypervisorReEnabled,
 				Status:  metav1.ConditionTrue,
 				Message: "Hypervisor is gone, no need to re-enable",
 				Reason:  kvmv1.ConditionReasonSucceeded,
 			})
 			if changed {
-				return r.Status().Update(ctx, eviction)
+				return r.updateStatus(ctx, eviction, base)
 			} else {
 				return nil
 			}
 		} else {
+			base := eviction.DeepCopy()
 			errorMessage := fmt.Sprintf("failed to get hypervisor due to %s", err)
 			// update the condition to reflect the error, and retry the reconciliation
 			changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
@@ -422,7 +436,7 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
 			})
 
 			if changed {
-				if err2 := r.Status().Update(ctx, eviction); err2 != nil {
+				if err2 := r.updateStatus(ctx, eviction, base); err2 != nil {
 					log.Error(err, "failed to store error message in condition", "message", errorMessage)
 					return err2
 				}
@@ -433,14 +447,15 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
 	}
 
 	if hypervisor.Service.DisabledReason != r.evictionReason(eviction) {
+		base := eviction.DeepCopy()
 		changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 			Type:    kvmv1.ConditionTypeHypervisorReEnabled,
 			Status:  metav1.ConditionTrue,
 			Message: "Hypervisor already re-enabled for reason:" + hypervisor.Service.DisabledReason,
 			Reason:  kvmv1.ConditionReasonSucceeded,
 		})
 		if changed {
-			return r.Status().Update(ctx, eviction)
+			return r.updateStatus(ctx, eviction, base)
 		} else {
 			return nil
 		}
@@ -451,6 +466,7 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
 	_, err = services.Update(ctx, r.computeClient, hypervisor.Service.ID, enableService).Extract()
 
 	if err != nil {
+		base := eviction.DeepCopy()
 		errorMessage := fmt.Sprintf("failed to enable hypervisor due to %s", err)
 		changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 			Type:    kvmv1.ConditionTypeHypervisorReEnabled,
@@ -459,20 +475,21 @@ func (r *EvictionReconciler) enableHypervisorService(ctx context.Context, evicti
 			Reason:  kvmv1.ConditionReasonFailed,
 		})
 		if changed {
-			if err2 := r.Status().Update(ctx, eviction); err2 != nil {
+			if err2 := r.updateStatus(ctx, eviction, base); err2 != nil {
 				log.Error(err, "failed to store error message in condition", "message", errorMessage)
 			}
 		}
 		return ErrRetry
 	} else {
+		base := eviction.DeepCopy()
 		changed := meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 			Type:    kvmv1.ConditionTypeHypervisorReEnabled,
 			Status:  metav1.ConditionTrue,
 			Message: "Hypervisor re-enabled successfully",
 			Reason:  kvmv1.ConditionReasonSucceeded,
 		})
 		if changed {
-			return r.Status().Update(ctx, eviction)
+			return r.updateStatus(ctx, eviction, base)
 		} else {
 			return nil
 		}
@@ -486,26 +503,28 @@ func (r *EvictionReconciler) disableHypervisor(ctx context.Context, hypervisor *
 	disabledReason := hypervisor.Service.DisabledReason
 
 	if disabledReason != "" && disabledReason != evictionReason {
+		base := eviction.DeepCopy()
 		// Disabled for another reason already
 		meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 			Type:    kvmv1.ConditionTypeHypervisorDisabled,
 			Status:  metav1.ConditionTrue,
 			Message: fmt.Sprintf("Hypervisor already disabled for reason %q", disabledReason),
 			Reason:  kvmv1.ConditionReasonSucceeded,
 		})
-		return r.Status().Update(ctx, eviction)
+		return r.updateStatus(ctx, eviction, base)
 	}
 
 	if !controllerutil.ContainsFinalizer(eviction, evictionFinalizerName) {
-		evictionBase := eviction.DeepCopy()
+		base := eviction.DeepCopy()
 		controllerutil.AddFinalizer(eviction, evictionFinalizerName)
-		return r.Patch(ctx, eviction, client.MergeFromWithOptions(evictionBase, client.MergeFromWithOptimisticLock{}))
+		return r.Patch(ctx, eviction, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{}), client.FieldOwner(EvictionControllerName))
 	}
 
 	disableService := services.UpdateOpts{Status: services.ServiceDisabled,
 		DisabledReason: r.evictionReason(eviction)}
 
 	_, err := services.Update(ctx, r.computeClient, hypervisor.Service.ID, disableService).Extract()
+	base := eviction.DeepCopy()
 	if err != nil {
 		// We expect OpenStack calls to be transient errors, so we retry for the next reconciliation
 		meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
@@ -514,7 +533,7 @@ func (r *EvictionReconciler) disableHypervisor(ctx context.Context, hypervisor *
 			Message: fmt.Sprintf("Failed to disable hypervisor: %v", err),
 			Reason:  kvmv1.ConditionReasonFailed,
 		})
-		return r.Status().Update(ctx, eviction)
+		return r.updateStatus(ctx, eviction, base)
 	}
 
 	meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
@@ -523,7 +542,7 @@ func (r *EvictionReconciler) disableHypervisor(ctx context.Context, hypervisor *
 		Message: "Hypervisor disabled successfully",
 		Reason:  kvmv1.ConditionReasonSucceeded,
 	})
-	return r.Status().Update(ctx, eviction)
+	return r.updateStatus(ctx, eviction, base)
 }
 
 func (r *EvictionReconciler) liveMigrate(ctx context.Context, uuid string, eviction *kvmv1.Eviction) error {
@@ -568,28 +587,6 @@ func (r *EvictionReconciler) coldMigrate(ctx context.Context, uuid string, evict
 	return nil
 }
 
-// addCondition adds a condition to the Eviction status and updates the status
-func (r *EvictionReconciler) addCondition(ctx context.Context, eviction *kvmv1.Eviction,
-	status metav1.ConditionStatus, message string, reason string) bool {
-
-	if !meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
-		Type:    kvmv1.ConditionTypeEvicting,
-		Status:  status,
-		Message: message,
-		Reason:  reason,
-	}) {
-		return false
-	}
-
-	if err := r.Status().Update(ctx, eviction); err != nil {
-		log := logger.FromContext(ctx)
-		log.Error(err, "Failed to update conditions")
-		return false
-	}
-
-	return true
-}
-
 // SetupWithManager sets up the controller with the Manager.
 func (r *EvictionReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	ctx := context.Background()