fix: controllers: guard on empty .status.authoritativeAPI, take 2

Author: damdo

pkg/controller/machine/controller.go

@@ -179,21 +179,21 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
 	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)
+			// 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)
 			}
 
-			klog.Infof("%v: machine is paused, taking no further action", machineName)
-
+			// Return to give a chance to the changes to get propagated.
 			return reconcile.Result{}, nil
 
 		case machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating:
@@ -572,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,23 +122,17 @@ 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 (empty status.authoritativeAPI)")
+			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 appropriately 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)),
@@ -195,7 +184,7 @@ var _ = Describe("Machine Reconciler", func() {
 				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMigrating
 			})).Should(Succeed())
 
-			By("Verifying the paused condition is appropriately set to true")
+			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)),
@@ -210,7 +199,7 @@ var _ = Describe("Machine Reconciler", func() {
 				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
 			})).Should(Succeed())
 
-			By("Verifying the paused condition is appropriately 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)),
@@ -219,21 +208,6 @@ var _ = Describe("Machine Reconciler", func() {
 				))),
 				HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityMachineAPI)),
 			))
-
-			By("Changing status.authoritativeAPI from MachineAPI to empty")
-			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("")),
-			))
 		})
 	})
 })

pkg/controller/machineset/controller.go

@@ -176,20 +176,21 @@ 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)
+			// 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", machineSet.Name)
 
+			// Return to give a chance to the changes to get propagated.
 			return reconcile.Result{}, nil
 
 		case machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating:
@@ -518,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{}
 

pkg/controller/machineset/machineset_controller_test.go

@@ -108,15 +108,13 @@ var _ = Describe("MachineSet Reconciler", func() {
 	})
 
 	It("Should reconcile a MachineSet", func() {
-		instance := machineSetBuilder.Build()
+		instance := machineSetBuilder.WithAuthoritativeAPI(machinev1.MachineAuthorityMachineAPI).Build()
 
-		By("Creating the MachineSet (empty status.authoritativeAPI)")
+		By("Creating the MachineSet")
 		Expect(k8sClient.Create(ctx, instance)).To(Succeed())
 
-		By("Setting status.authoritativeAPI to MachineAPI")
-		Eventually(k.UpdateStatus(instance, func() {
-			instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
-		})).Should(Succeed())
+		By("Verying the MachineSet has been reconciled (status set)")
+		Eventually(k.Object(instance), timeout).Should(HaveField("Status", Not(Equal(machinev1.MachineStatus{}))))
 
 		machines := &machinev1.MachineList{}
 		By("Verifying that we have 2 replicas")
@@ -153,19 +151,14 @@ var _ = Describe("MachineSet Reconciler", func() {
 			instance = machineSetBuilder.
 				WithGenerateName("baz-").
 				WithMachineTemplateLabels(map[string]string{"baz": "bar"}).
+				WithAuthoritativeAPI(machinev1.MachineAuthorityClusterAPI).
 				Build()
 		})
 		It("Should set the Paused condition appropriately", func() {
-
-			By("Creating the MachineSet (empty status.authoritativeAPI)")
+			By("Creating the MachineSet (spec.authoritativeAPI=ClusterAPI)")
 			Expect(k8sClient.Create(ctx, instance)).To(Succeed())
 
-			By("Setting status.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 status.authoritativeAPI has been set to ClusterAPI and paused condition is appropriately set to true")
 			Eventually(k.Object(instance), timeout).Should(SatisfyAll(
 				HaveField("Status.Conditions", ContainElement(SatisfyAll(
 					HaveField("Type", Equal(machine.PausedCondition)),
@@ -217,7 +210,7 @@ var _ = Describe("MachineSet Reconciler", func() {
 				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMigrating
 			})).Should(Succeed())
 
-			By("Verifying the paused condition is appropriately set to true")
+			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(machine.PausedCondition)),
@@ -232,7 +225,7 @@ var _ = Describe("MachineSet Reconciler", func() {
 				instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
 			})).Should(Succeed())
 
-			By("Verifying the paused condition is appropriately 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(machine.PausedCondition)),
@@ -241,21 +234,6 @@ var _ = Describe("MachineSet Reconciler", func() {
 				))),
 				HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityMachineAPI)),
 			))
-
-			By("Changing status.authoritativeAPI from MachineAPI to empty")
-			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(machine.PausedCondition)),
-					HaveField("Status", Equal(corev1.ConditionTrue)),
-					HaveField("Message", Equal("The AuthoritativeAPI status is not yet set")),
-				))),
-				HaveField("Status.AuthoritativeAPI", BeEquivalentTo("")),
-			))
 		})
 	})
 })