Merge pull request #1383 from damdo/revert-1380-fix-controllers-guard-on-empty-status-authoritativeAPI

Revert "OCPCLOUD-2986,OCPBUGS-56849: fix: controllers: guard on empty .status.authoritativeAPI"

Author: openshift-merge-bot[bot]

pkg/controller/machine/controller.go

@@ -177,60 +177,43 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
 	originalConditions := conditions.DeepCopyConditions(m.Status.Conditions)
 
 	if r.gate.Enabled(featuregate.Feature(openshiftfeatures.FeatureGateMachineAPIMigration)) {
-		switch m.Status.AuthoritativeAPI {
-		case "":
-			// An empty .status.authoritativeAPI normally means the resource has not yet been reconciled
-			// by the migration controller, which has the responsibility for propagating .spec.authoritativeAPI to the status.
-			// Pause the resource and take no further action but return until that field is populated.
-			desiredCondition := conditions.TrueConditionWithReason(
-				PausedCondition, PausedConditionReason,
-				"The AuthoritativeAPI status is not yet set",
-			)
-
-			if _, err := r.ensureUpdatedPausedCondition(ctx, m, desiredCondition,
-				fmt.Sprintf("%v: machine .status.authoritativeAPI is not yet set, ensuring machine is paused", machineName)); 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
-
-		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)
+		// 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)
 			}
 
 			klog.Infof("%v: machine is paused, taking no further action", machineName)
-
 			return reconcile.Result{}, nil
+		}
 
-		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)
-			}
+		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"
+		}
 
-			// 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.
+		// 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)
 		}
 	}
 
@@ -450,23 +433,6 @@ 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) {

pkg/controller/machine/machine_controller_test.go

@@ -135,20 +135,19 @@ var _ = Describe("Machine Reconciler", func() {
 
 		It("Should set the Paused condition appropriately", func() {
 
-			By("Creating the Machine (empty status.authoritativeAPI)")
+			By("Creating the Machine")
 			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 appropriately set to true")
+			By("Verifying the paused condition is approproately 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)),
 			))
@@ -190,50 +189,36 @@ var _ = Describe("Machine Reconciler", func() {
 					})
 			}()
 
-			By("Changing status.authoritativeAPI from ClusterAPI to Migrating")
+			By("Transitioning the AuthoritativeAPI though 'Migrating' to MachineAPI")
 			Eventually(k.UpdateStatus(instance, func() {
 				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMigrating
 			})).Should(Succeed())
 
-			By("Verifying 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 'Migrating'")),
-				))),
-				HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityMigrating)),
-			))
-
-			By("Changing status.authoritativeAPI from Migrating to MachineAPI")
+			By("Updating the AuthoritativeAPI from Migrating to MachineAPI")
 			Eventually(k.UpdateStatus(instance, func() {
 				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
 			})).Should(Succeed())
 
-			By("Verifying the paused condition is appropriately set to false")
+			By("Verifying the paused condition is approproately set to false")
 			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("Changing status.authoritativeAPI from MachineAPI to empty")
+			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 appropriately set to true when empty status.authoritativeAPI")
-			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 not yet set")),
-				))),
-				HaveField("Status.AuthoritativeAPI", BeEquivalentTo("")),
-			))
+			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,60 +174,59 @@ func (r *ReconcileMachineSet) Reconcile(ctx context.Context, request reconcile.R
 	}
 
 	if r.gate.Enabled(featuregate.Feature(openshiftfeatures.FeatureGateMachineAPIMigration)) {
-		switch machineSet.Status.AuthoritativeAPI {
-		case "":
-			// An empty .status.authoritativeAPI normally means the resource has not yet been reconciled
-			// by the migration controller, which has the responsibility for propagating .spec.authoritativeAPI to the status.
-			// Pause the resource and take no further action but return until that field is populated.
-			desiredCondition := conditions.TrueConditionWithReason(
-				machine.PausedCondition, machine.PausedConditionReason,
-				"The AuthoritativeAPI status is not yet set",
-			)
-
-			if _, err := r.ensureUpdatedPausedCondition(machineSet, desiredCondition,
-				fmt.Sprintf("%v: machine set .status.authoritativeAPI is not yet set, ensuring machine set is paused", machineSet.Name)); err != nil {
-				return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
+		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
 			}
-			klog.Infof("%v: machine set is paused, taking no further action", machineSet.Name)
 
+			klog.Infof("%v: machine set is paused, taking no further action", machineSetName)
 			return reconcile.Result{}, nil
+		}
 
-		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)
-			}
+		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"
+		}
 
-			klog.Infof("%v: machine set is paused, taking no further action", machineSet.Name)
+		// Set the paused condition to false, continue reconciliation
+		conditions.Set(machineSetCopy, conditions.FalseCondition(
+			machine.PausedCondition,
+			machine.NotPausedConditionReason,
+			machinev1.ConditionSeverityInfo,
+			"%s",
+			pausedFalseReason,
+		))
 
+		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
-
-		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.
 		}
+
+		klog.Infof("%v: setting paused to false and continuing reconcile", machineSetName)
 	}
 
 	result, err := r.reconcile(ctx, machineSet)
@@ -330,23 +329,6 @@ 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 {