Merge pull request #1386 from damdo/fix-controllers-guard-on-empty-status-authoritativeAPI-take2

OCPCLOUD-2986,OCPCLOUD-2985,OCPBUGS-56849: Fix controllers guard on empty status authoritative api + defaulting

Author: openshift-merge-bot[bot]

pkg/controller/machine/controller.go

@@ -177,43 +177,60 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
 	originalConditions := conditions.DeepCopyConditions(m.Status.Conditions)
 
 	if r.gate.Enabled(featuregate.Feature(openshiftfeatures.FeatureGateMachineAPIMigration)) {
-		// Check Status.AuthoritativeAPI
-		// If not MachineAPI. Set the paused condition true and return early.
-		//
-		// Once we have a webhook, we want to remove the check that the AuthoritativeAPI
-		// field is populated.
-		if m.Status.AuthoritativeAPI != "" &&
-			m.Status.AuthoritativeAPI != machinev1.MachineAuthorityMachineAPI {
-			conditions.Set(m, conditions.TrueConditionWithReason(
-				PausedCondition,
-				PausedConditionReason,
-				"The AuthoritativeAPI is set to %s", string(m.Status.AuthoritativeAPI),
-			))
-			if patchErr := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), nil, originalConditions); patchErr != nil {
-				klog.Errorf("%v: error patching status: %v", machineName, patchErr)
+		switch m.Status.AuthoritativeAPI {
+		case "":
+			// An empty .status.authoritativeAPI normally means the resource has not yet been reconciled.
+			// and that the value in .spec.authoritativeAPI has not been propagated to .status.authoritativeAPI yet.
+			// This value can be set by two separate controllers, depending which one of them is running at that time,
+			// or in case they are both running, which one gets to set it first (the operation is idempotent so there is no harm in racing).
+			// - the cluster-capi-operator machine-api-migration's migration controller
+			// - this controller
+
+			klog.Infof("%v: machine .status.authoritativeAPI is not yet set, setting it to .spec.authoritativeAPI", m.Name)
+
+			if err := r.patchStatusAuthoritativeAPI(ctx, m, m.Spec.AuthoritativeAPI); err != nil {
+				klog.Errorf("%v: error patching status to set .status.authoritativeAPI for machine: %v", m.Name, err)
+				return reconcile.Result{}, fmt.Errorf("error patching status to set .status.authoritativeAPI for machine %s: %w", m.Name, err)
+			}
+
+			// Return to give a chance to the changes to get propagated.
+			return reconcile.Result{}, nil
+
+		case machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating:
+			// In cases when .status.authoritativeAPI is set to machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating
+			// the resource should be paused and not reconciled further.
+			desiredCondition := conditions.TrueConditionWithReason(
+				PausedCondition, PausedConditionReason,
+				"The AuthoritativeAPI status is set to '%s'", string(m.Status.AuthoritativeAPI),
+			)
+
+			if _, err := r.ensureUpdatedPausedCondition(ctx, m, desiredCondition,
+				fmt.Sprintf("%v: machine .status.authoritativeAPI is set to '%s', ensuring machine is paused", machineName, m.Status.AuthoritativeAPI)); err != nil {
+				return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
 			}
 
 			klog.Infof("%v: machine is paused, taking no further action", machineName)
+
 			return reconcile.Result{}, nil
-		}
 
-		var pausedFalseReason string
-		if m.Status.AuthoritativeAPI != "" {
-			pausedFalseReason = fmt.Sprintf("The AuthoritativeAPI is set to %s", string(m.Status.AuthoritativeAPI))
-		} else {
-			pausedFalseReason = "The AuthoritativeAPI is not set"
-		}
+		case machinev1.MachineAuthorityMachineAPI:
+			// The authority is MachineAPI and the resource should not be paused.
+			desiredCondition := conditions.FalseCondition(
+				PausedCondition, NotPausedConditionReason, machinev1.ConditionSeverityInfo, "%s",
+				fmt.Sprintf("The AuthoritativeAPI status is set to '%s'", string(m.Status.AuthoritativeAPI)),
+			)
+
+			if updated, err := r.ensureUpdatedPausedCondition(ctx, m, desiredCondition,
+				fmt.Sprintf("%v: machine .status.authoritativeAPI is set to '%s', unpausing machine", machineName, m.Status.AuthoritativeAPI)); err != nil {
+				return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
+			} else if updated {
+				klog.Infof("%v: setting machine paused condition to false", machineName)
+			}
 
-		// Set the paused condition to false, continue reconciliation
-		conditions.Set(m, conditions.FalseCondition(
-			PausedCondition,
-			NotPausedConditionReason,
-			machinev1.ConditionSeverityInfo,
-			"%s",
-			pausedFalseReason,
-		))
-		if patchErr := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), nil, originalConditions); patchErr != nil {
-			klog.Errorf("%v: error patching status: %v", machineName, patchErr)
+			// Fallthrough and continue reconcilation.
+		default:
+			klog.Errorf("%v: invalid .status.authoritativeAPI '%s'", machineName, m.Status.AuthoritativeAPI)
+			return reconcile.Result{}, nil // Do not return an error to avoid immediate requeue.
 		}
 	}
 
@@ -433,6 +450,23 @@ func (r *ReconcileMachine) deleteNode(ctx context.Context, name string) error {
 	return r.Client.Delete(ctx, &node)
 }
 
+// ensureUpdatedPausedCondition updates the paused condition if needed.
+func (r *ReconcileMachine) ensureUpdatedPausedCondition(ctx context.Context, m *machinev1.Machine, desiredCondition *machinev1.Condition, logMessage string) (bool, error) {
+	oldM := m.DeepCopy()
+	if !conditions.IsEquivalentTo(conditions.Get(m, PausedCondition), desiredCondition) {
+		klog.Info(logMessage)
+		conditions.Set(m, desiredCondition)
+		if err := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), nil, oldM.Status.Conditions); err != nil {
+			klog.Errorf("%v: error updating status: %v", oldM.Name, err)
+			return false, fmt.Errorf("error updating status for machine %s: %w", oldM.Name, err)
+		}
+
+		return true, nil
+	}
+
+	return false, nil
+}
+
 func delayIfRequeueAfterError(err error) (reconcile.Result, error) {
 	var requeueAfterError *RequeueAfterError
 	if errors.As(err, &requeueAfterError) {
@@ -538,6 +572,17 @@ func (r *ReconcileMachine) updateStatus(ctx context.Context, machine *machinev1.
 	return nil
 }
 
+func (r *ReconcileMachine) patchStatusAuthoritativeAPI(ctx context.Context, machine *machinev1.Machine, authoritativeAPI machinev1.MachineAuthority) error {
+	baseToPatch := client.MergeFrom(machine.DeepCopy())
+	machine.Status.AuthoritativeAPI = authoritativeAPI
+
+	if err := r.Client.Status().Patch(ctx, machine, baseToPatch); err != nil {
+		return fmt.Errorf("error patching machine status: %w", err)
+	}
+
+	return nil
+}
+
 func (r *ReconcileMachine) patchFailedMachineInstanceAnnotation(ctx context.Context, machine *machinev1.Machine) error {
 	baseToPatch := client.MergeFrom(machine.DeepCopy())
 	if machine.Annotations == nil {

pkg/controller/machine/machine_controller_test.go

@@ -104,20 +104,15 @@ var _ = Describe("Machine Reconciler", func() {
 		testutils.CleanupResources(Default, ctx, cfg, k8sClient, namespace.GetName(),
 			&machinev1.Machine{},
 		)
-
 	})
 
 	It("Should reconcile a Machine", func() {
-		instance := machineBuilder.Build()
+		instance := machineBuilder.WithAuthoritativeAPI(machinev1.MachineAuthorityMachineAPI).Build()
 
 		By("Creating the Machine")
 		Expect(k8sClient.Create(ctx, instance)).To(Succeed())
 
-		By("Setting the AuthoritativeAPI to MachineAPI")
-		Eventually(k.UpdateStatus(instance, func() {
-			instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
-		})).Should(Succeed())
-
+		By("Verying the Machine has been reconciled (status set)")
 		Eventually(k.Object(instance), timeout).Should(HaveField("Status", Not(Equal(machinev1.MachineStatus{}))))
 
 		// // Expect platform status is not empty. This (check) means we've called the
@@ -127,27 +122,22 @@ var _ = Describe("Machine Reconciler", func() {
 
 	})
 
-	Describe("Paused condition", func() {
+	Describe("Paused Condition", func() {
 		var instance *machinev1.Machine
 		BeforeEach(func() {
-			instance = machineBuilder.Build()
+			instance = machineBuilder.WithAuthoritativeAPI(machinev1.MachineAuthorityClusterAPI).Build()
 		})
 
 		It("Should set the Paused condition appropriately", func() {
-
-			By("Creating the Machine")
+			By("Creating the Machine (spec.authoritativeAPI=ClusterAPI)")
 			Expect(k8sClient.Create(ctx, instance)).To(Succeed())
 
-			By("Setting the AuthoritativeAPI to ClusterAPI")
-			Eventually(k.UpdateStatus(instance, func() {
-				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityClusterAPI
-			})).Should(Succeed())
-
-			By("Verifying the paused condition is approproately set to true")
+			By("Verifying the status.authoritativeAPI has been set to ClusterAPI and the paused condition is appropriately set to true")
 			Eventually(k.Object(instance), timeout).Should(SatisfyAll(
 				HaveField("Status.Conditions", ContainElement(SatisfyAll(
 					HaveField("Type", Equal(PausedCondition)),
 					HaveField("Status", Equal(corev1.ConditionTrue)),
+					HaveField("Message", Equal("The AuthoritativeAPI status is set to 'ClusterAPI'")),
 				))),
 				HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityClusterAPI)),
 			))
@@ -189,36 +179,35 @@ var _ = Describe("Machine Reconciler", func() {
 					})
 			}()
 
-			By("Transitioning the AuthoritativeAPI though 'Migrating' to MachineAPI")
+			By("Changing status.authoritativeAPI from ClusterAPI to Migrating")
 			Eventually(k.UpdateStatus(instance, func() {
 				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMigrating
 			})).Should(Succeed())
 
-			By("Updating the AuthoritativeAPI from Migrating to MachineAPI")
+			By("Verifying the paused condition is appropriately set to true while in Migrating")
+			Eventually(k.Object(instance), timeout).Should(SatisfyAll(
+				HaveField("Status.Conditions", ContainElement(SatisfyAll(
+					HaveField("Type", Equal(PausedCondition)),
+					HaveField("Status", Equal(corev1.ConditionTrue)),
+					HaveField("Message", Equal("The AuthoritativeAPI status is set to 'Migrating'")),
+				))),
+				HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityMigrating)),
+			))
+
+			By("Changing status.authoritativeAPI from Migrating to MachineAPI")
 			Eventually(k.UpdateStatus(instance, func() {
 				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
 			})).Should(Succeed())
 
-			By("Verifying the paused condition is approproately set to false")
+			By("Verifying the paused condition is appropriately set to false when in MachineAPI")
 			Eventually(k.Object(instance), timeout).Should(SatisfyAll(
 				HaveField("Status.Conditions", ContainElement(SatisfyAll(
 					HaveField("Type", Equal(PausedCondition)),
 					HaveField("Status", Equal(corev1.ConditionFalse)),
+					HaveField("Message", Equal("The AuthoritativeAPI status is set to 'MachineAPI'")),
 				))),
 				HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityMachineAPI)),
 			))
-
-			By("Unsetting the AuthoritativeAPI field in the status")
-			Eventually(k.UpdateStatus(instance, func() {
-				instance.Status.AuthoritativeAPI = ""
-			})).Should(Succeed())
-
-			By("Verifying the paused condition is still approproately set to false")
-			Eventually(k.Object(instance), timeout).Should(HaveField("Status.Conditions", ContainElement(SatisfyAll(
-				HaveField("Type", Equal(PausedCondition)),
-				HaveField("Status", Equal(corev1.ConditionFalse)),
-				HaveField("Message", Equal("The AuthoritativeAPI is not set")),
-			))))
 		})
 	})
 })

pkg/controller/machineset/controller.go

@@ -174,59 +174,61 @@ func (r *ReconcileMachineSet) Reconcile(ctx context.Context, request reconcile.R
 	}
 
 	if r.gate.Enabled(featuregate.Feature(openshiftfeatures.FeatureGateMachineAPIMigration)) {
-		machineSetName := machineSet.GetName()
-		machineSetCopy := machineSet.DeepCopy()
-		// Check Status.AuthoritativeAPI. If it's not set to MachineAPI. Set the
-		// paused condition true and return early.
-		//
-		// Once we have a webhook, we want to remove the check that the AuthoritativeAPI
-		// field is populated.
-		if machineSet.Status.AuthoritativeAPI != "" &&
-			machineSet.Status.AuthoritativeAPI != machinev1.MachineAuthorityMachineAPI {
-			conditions.Set(machineSetCopy, conditions.TrueConditionWithReason(
-				machine.PausedCondition,
-				machine.PausedConditionReason,
-				"The AuthoritativeAPI is set to %s", string(machineSet.Status.AuthoritativeAPI),
-			))
-
-			_, err := updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
-			if err != nil && !apierrors.IsNotFound(err) {
-				klog.Errorf("%v: error updating status: %v", machineSetName, err)
-				return reconcile.Result{}, fmt.Errorf("error updating status: %w", err)
-			} else if apierrors.IsNotFound(err) {
-				return reconcile.Result{}, nil
+		switch machineSet.Status.AuthoritativeAPI {
+		case "":
+			// An empty .status.authoritativeAPI normally means the resource has not yet been reconciled.
+			// and that the value in .spec.authoritativeAPI has not been propagated to .status.authoritativeAPI yet.
+			// This value can be set by two separate controllers, depending which one of them is running at that time,
+			// or in case they are both running, which one gets to set it first (the operation is idempotent so there is no harm in racing).
+			// - the cluster-capi-operator machine-api-migration's migration controller
+			// - this controller
+
+			klog.Infof("%v: machine set .status.authoritativeAPI is not yet set, setting it to .spec.authoritativeAPI", machineSet.Name)
+
+			if err := r.patchStatusAuthoritativeAPI(ctx, machineSet, machineSet.Spec.AuthoritativeAPI); err != nil {
+				klog.Errorf("%v: error patching status to set .status.authoritativeAPI for machine set: %v", machineSet.Name, err)
+				return reconcile.Result{}, fmt.Errorf("error patching status to set .status.authoritativeAPI for machine set %s: %w", machineSet.Name, err)
 			}
 
-			klog.Infof("%v: machine set is paused, taking no further action", machineSetName)
+			// Return to give a chance to the changes to get propagated.
 			return reconcile.Result{}, nil
-		}
 
-		var pausedFalseReason string
-		if machineSet.Status.AuthoritativeAPI != "" {
-			pausedFalseReason = fmt.Sprintf("The AuthoritativeAPI is set to %s", string(machineSet.Status.AuthoritativeAPI))
-		} else {
-			pausedFalseReason = "The AuthoritativeAPI is not set"
-		}
+		case machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating:
+			// In case .status.authoritativeAPI is set to machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating
+			// the resource should be paused and not reconciled further.
+			desiredCondition := conditions.TrueConditionWithReason(
+				machine.PausedCondition, machine.PausedConditionReason,
+				"The AuthoritativeAPI status is set to '%s'", string(machineSet.Status.AuthoritativeAPI),
+			)
+
+			if _, err := r.ensureUpdatedPausedCondition(machineSet, desiredCondition,
+				fmt.Sprintf("%v: machine set .status.authoritativeAPI is set to '%s', ensuring machine set is paused", machineSet.Name, machineSet.Status.AuthoritativeAPI)); err != nil {
+				return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
+			}
 
-		// Set the paused condition to false, continue reconciliation
-		conditions.Set(machineSetCopy, conditions.FalseCondition(
-			machine.PausedCondition,
-			machine.NotPausedConditionReason,
-			machinev1.ConditionSeverityInfo,
-			"%s",
-			pausedFalseReason,
-		))
+			klog.Infof("%v: machine set is paused, taking no further action", machineSet.Name)
 
-		var err error
-		machineSet, err = updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
-		if err != nil && !apierrors.IsNotFound(err) {
-			klog.Errorf("%v: error updating status: %v", machineSetName, err)
-			return reconcile.Result{}, fmt.Errorf("error updating status: %w", err)
-		} else if apierrors.IsNotFound(err) {
 			return reconcile.Result{}, nil
-		}
 
-		klog.Infof("%v: setting paused to false and continuing reconcile", machineSetName)
+		case machinev1.MachineAuthorityMachineAPI:
+			// The authority is MachineAPI and the resource should not be paused.
+			desiredCondition := conditions.FalseCondition(
+				machine.PausedCondition, machine.NotPausedConditionReason, machinev1.ConditionSeverityInfo, "%s",
+				fmt.Sprintf("The AuthoritativeAPI status is set to '%s'", string(machineSet.Status.AuthoritativeAPI)),
+			)
+
+			if updated, err := r.ensureUpdatedPausedCondition(machineSet, desiredCondition,
+				fmt.Sprintf("%v: machine set .status.authoritativeAPI is set to '%s', unpausing machine set", machineSet.Name, machineSet.Status.AuthoritativeAPI)); err != nil {
+				return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
+			} else if updated {
+				klog.Infof("%v: setting machine set paused condition to false", machineSet.Name)
+			}
+
+			// Fallthrough and continue reconciliation.
+		default:
+			klog.Errorf("%v: invalid .status.authoritativeAPI '%s'", machineSet.Name, machineSet.Status.AuthoritativeAPI)
+			return reconcile.Result{}, nil // Do not return an error to avoid immediate requeue.
+		}
 	}
 
 	result, err := r.reconcile(ctx, machineSet)
@@ -329,6 +331,23 @@ func (r *ReconcileMachineSet) reconcile(ctx context.Context, machineSet *machine
 	return reconcile.Result{}, nil
 }
 
+// ensureUpdatedPausedCondition updates the paused condition if needed.
+func (r *ReconcileMachineSet) ensureUpdatedPausedCondition(ms *machinev1.MachineSet, desiredCondition *machinev1.Condition, logMessage string) (bool, error) {
+	newMs := ms.DeepCopy()
+	if !conditions.IsEquivalentTo(conditions.Get(ms, machine.PausedCondition), desiredCondition) {
+		klog.Info(logMessage)
+		conditions.Set(newMs, desiredCondition)
+		if _, err := updateMachineSetStatus(r.Client, ms, newMs.Status); err != nil {
+			klog.Errorf("%v: error updating status: %v", newMs.Name, err)
+			return false, fmt.Errorf("error updating status for machine set %s: %w", newMs.Name, err)
+		}
+
+		return true, nil
+	}
+
+	return false, nil
+}
+
 // syncReplicas essentially scales machine resources up and down.
 func (r *ReconcileMachineSet) syncReplicas(ms *machinev1.MachineSet, machines []*machinev1.Machine) error {
 	if ms.Spec.Replicas == nil {
@@ -500,6 +519,17 @@ func (r *ReconcileMachineSet) waitForMachineDeletion(machineList []*machinev1.Ma
 	return nil
 }
 
+func (r *ReconcileMachineSet) patchStatusAuthoritativeAPI(ctx context.Context, machineSet *machinev1.MachineSet, authoritativeAPI machinev1.MachineAuthority) error {
+	baseToPatch := client.MergeFrom(machineSet.DeepCopy())
+	machineSet.Status.AuthoritativeAPI = authoritativeAPI
+
+	if err := r.Client.Status().Patch(ctx, machineSet, baseToPatch); err != nil {
+		return fmt.Errorf("error patching machine set status: %w", err)
+	}
+
+	return nil
+}
+
 func validateMachineset(m *machinev1.MachineSet) field.ErrorList {
 	errors := field.ErrorList{}