Avoid duplicate reconciles if only generation of Paused condition changed

Author: sbueringer

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -207,7 +207,13 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, err
 	}
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, config); err != nil || isPaused || conditionChanged {
+	// Initialize the patch helper.
+	patchHelper, err := patch.NewHelper(config, r.Client)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, config); err != nil || isPaused || requeue {
 		return ctrl.Result{}, err
 	}
 
@@ -218,12 +224,6 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		Cluster:     cluster,
 	}
 
-	// Initialize the patch helper.
-	patchHelper, err := patch.NewHelper(config, r.Client)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
 	// Attempt to Patch the KubeadmConfig object and status after each reconciliation if no error occurs.
 	defer func() {
 		// always update the readyCondition; the summary is represented using the "1 of x completed" notation.
@@ -261,6 +261,7 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 				bootstrapv1.CertificatesAvailableCondition,
 			}},
 			patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+				clusterv1.PausedV1Beta2Condition,
 				bootstrapv1.KubeadmConfigReadyV1Beta2Condition,
 				bootstrapv1.KubeadmConfigDataSecretAvailableV1Beta2Condition,
 				bootstrapv1.KubeadmConfigCertificatesAvailableV1Beta2Condition,

controlplane/kubeadm/internal/controllers/controller.go

@@ -189,17 +189,17 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
 	log = log.WithValues("Cluster", klog.KObj(cluster))
 	ctx = ctrl.LoggerInto(ctx, log)
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, kcp); err != nil || isPaused || conditionChanged {
-		return ctrl.Result{}, err
-	}
-
 	// Initialize the patch helper.
 	patchHelper, err := patch.NewHelper(kcp, r.Client)
 	if err != nil {
 		log.Error(err, "Failed to configure the patch helper")
 		return ctrl.Result{Requeue: true}, nil
 	}
 
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, kcp); err != nil || isPaused || requeue {
+		return ctrl.Result{}, err
+	}
+
 	// Initialize the control plane scope; this includes also checking for orphan machines and
 	// adopt them if necessary.
 	controlPlane, adoptableMachineFound, err := r.initControlPlaneScope(ctx, cluster, kcp)
@@ -337,6 +337,7 @@ func patchKubeadmControlPlane(ctx context.Context, patchHelper *patch.Helper, kc
 			controlplanev1.CertificatesAvailableCondition,
 		}},
 		patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+			clusterv1.PausedV1Beta2Condition,
 			controlplanev1.KubeadmControlPlaneAvailableV1Beta2Condition,
 			controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition,
 			controlplanev1.KubeadmControlPlaneCertificatesAvailableV1Beta2Condition,

exp/internal/controllers/machinepool_controller.go

@@ -179,16 +179,16 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			mp.Spec.ClusterName, mp.Name, mp.Namespace)
 	}
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, mp); err != nil || isPaused || conditionChanged {
-		return ctrl.Result{}, err
-	}
-
 	// Initialize the patch helper.
 	patchHelper, err := patch.NewHelper(mp, r.Client)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, mp); err != nil || isPaused || requeue {
+		return ctrl.Result{}, err
+	}
+
 	defer func() {
 		r.reconcilePhase(mp)
 		// TODO(jpang): add support for metrics.
@@ -211,6 +211,9 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 				clusterv1.InfrastructureReadyCondition,
 				expv1.ReplicasReadyCondition,
 			}},
+			patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+				clusterv1.PausedV1Beta2Condition,
+			}},
 		}
 		if reterr == nil {
 			patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})

exp/runtime/internal/controllers/extensionconfig_controller.go

@@ -132,7 +132,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, err
 	}
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, nil, extensionConfig); err != nil || isPaused || conditionChanged {
+	// Copy to avoid modifying the original extensionConfig.
+	original := extensionConfig.DeepCopy()
+
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, nil, extensionConfig); err != nil || isPaused || requeue {
 		return ctrl.Result{}, err
 	}
 
@@ -141,9 +144,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return r.reconcileDelete(ctx, extensionConfig)
 	}
 
-	// Copy to avoid modifying the original extensionConfig.
-	original := extensionConfig.DeepCopy()
-
 	// Inject CABundle from secret if annotation is set. Otherwise https calls may fail.
 	if err := reconcileCABundle(ctx, r.Client, extensionConfig); err != nil {
 		return ctrl.Result{}, err
@@ -183,6 +183,7 @@ func patchExtensionConfig(ctx context.Context, client client.Client, original, m
 			runtimev1.RuntimeExtensionDiscoveredCondition,
 		}},
 		patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+			clusterv1.PausedV1Beta2Condition,
 			runtimev1.ExtensionConfigDiscoveredV1Beta2Condition,
 		}},
 	)

exp/runtime/internal/controllers/extensionconfig_controller_test.go

@@ -180,19 +180,7 @@ func TestExtensionReconciler_Reconcile(t *testing.T) {
 			return nil
 		}, 30*time.Second, 100*time.Millisecond).Should(Succeed())
 
-		// Reconcile the extension and assert discovery has succeeded  (the first reconcile updates observedGeneration in the Paused condition).
-		_, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)})
-		g.Expect(err).ToNot(HaveOccurred())
-
-		// Wait until the ExtensionConfig in the cache has the up-to-date Paused condition so the next Reconcile can do discovery.
-		g.Eventually(func(g Gomega) {
-			conf := &runtimev1.ExtensionConfig{}
-			g.Expect(env.Get(ctx, util.ObjectKey(extensionConfig), conf)).To(Succeed())
-			pausedCondition := v1beta2conditions.Get(conf, clusterv1.PausedV1Beta2Condition)
-			g.Expect(pausedCondition).ToNot(BeNil())
-			g.Expect(pausedCondition.ObservedGeneration).To(Equal(conf.Generation))
-		}).WithTimeout(10 * time.Second).WithPolling(100 * time.Millisecond).Should(Succeed())
-
+		// Reconcile the extension and assert discovery has succeeded.
 		_, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)})
 		g.Expect(err).ToNot(HaveOccurred())
 

internal/controllers/cluster/cluster_controller.go

@@ -159,7 +159,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retRes ct
 		return ctrl.Result{}, err
 	}
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, cluster); err != nil || isPaused || conditionChanged {
+	// Initialize the patch helper.
+	patchHelper, err := patch.NewHelper(cluster, r.Client)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, cluster); err != nil || isPaused || requeue {
 		return ctrl.Result{}, err
 	}
 
@@ -173,12 +179,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retRes ct
 		}
 	}
 
-	// Initialize the patch helper.
-	patchHelper, err := patch.NewHelper(cluster, r.Client)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
 	defer func() {
 		// Always reconcile the Status.
 		r.updateStatus(ctx, s)
@@ -272,6 +272,7 @@ func patchCluster(ctx context.Context, patchHelper *patch.Helper, cluster *clust
 			clusterv1.InfrastructureReadyCondition,
 		}},
 		patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+			clusterv1.PausedV1Beta2Condition,
 			clusterv1.ClusterInfrastructureReadyV1Beta2Condition,
 			clusterv1.ClusterControlPlaneAvailableV1Beta2Condition,
 			clusterv1.ClusterControlPlaneInitializedV1Beta2Condition,

internal/controllers/clusterclass/clusterclass_controller.go

@@ -114,7 +114,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
 		return ctrl.Result{}, err
 	}
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, nil, clusterClass); err != nil || isPaused || conditionChanged {
+	patchHelper, err := patch.NewHelper(clusterClass, r.Client)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, nil, clusterClass); err != nil || isPaused || requeue {
 		return ctrl.Result{}, err
 	}
 
@@ -126,11 +131,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
 		clusterClass: clusterClass,
 	}
 
-	patchHelper, err := patch.NewHelper(clusterClass, r.Client)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
 	defer func() {
 		updateStatus(ctx, s)
 
@@ -140,6 +140,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
 				clusterv1.ClusterClassVariablesReconciledCondition,
 			}},
 			patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+				clusterv1.PausedV1Beta2Condition,
 				clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition,
 				clusterv1.ClusterClassVariablesReadyV1Beta2Condition,
 			}},

internal/controllers/clusterresourceset/clusterresourceset_controller.go

@@ -142,20 +142,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		return ctrl.Result{}, err
 	}
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, nil, clusterResourceSet); err != nil || isPaused || conditionChanged {
-		return ctrl.Result{}, err
-	}
-
 	// Initialize the patch helper.
 	patchHelper, err := patch.NewHelper(clusterResourceSet, r.Client)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, nil, clusterResourceSet); err != nil || isPaused || requeue {
+		return ctrl.Result{}, err
+	}
+
 	defer func() {
 		// Always attempt to patch the object and status after each reconciliation.
 		// Patch ObservedGeneration only if the reconciliation completed successfully.
-		patchOpts := []patch.Option{}
+		patchOpts := []patch.Option{
+			patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+				clusterv1.PausedV1Beta2Condition,
+				addonsv1.ResourcesAppliedV1Beta2Condition,
+			}},
+		}
 		if reterr == nil {
 			patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
 		}

internal/controllers/machine/machine_controller.go

@@ -221,7 +221,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 			m.Spec.ClusterName, m.Name, m.Namespace)
 	}
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, m); err != nil || isPaused || conditionChanged {
+	// Initialize the patch helper
+	patchHelper, err := patch.NewHelper(m, r.Client)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, m); err != nil || isPaused || requeue {
 		return ctrl.Result{}, err
 	}
 
@@ -240,12 +246,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		machine: m,
 	}
 
-	// Initialize the patch helper
-	patchHelper, err := patch.NewHelper(m, r.Client)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
-
 	defer func() {
 		r.updateStatus(ctx, s)
 
@@ -315,6 +315,7 @@ func patchMachine(ctx context.Context, patchHelper *patch.Helper, machine *clust
 			clusterv1.DrainingSucceededCondition,
 		}},
 		patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+			clusterv1.PausedV1Beta2Condition,
 			clusterv1.MachineAvailableV1Beta2Condition,
 			clusterv1.MachineReadyV1Beta2Condition,
 			clusterv1.MachineBootstrapConfigReadyV1Beta2Condition,

internal/controllers/machinedeployment/machinedeployment_controller.go

@@ -146,16 +146,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
 		return ctrl.Result{}, err
 	}
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, deployment); err != nil || isPaused || conditionChanged {
-		return ctrl.Result{}, err
-	}
-
 	// Initialize the patch helper
 	patchHelper, err := patch.NewHelper(deployment, r.Client)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
 
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, deployment); err != nil || isPaused || requeue {
+		return ctrl.Result{}, err
+	}
+
 	s := &scope{
 		machineDeployment: deployment,
 		cluster:           cluster,
@@ -215,6 +215,7 @@ func patchMachineDeployment(ctx context.Context, patchHelper *patch.Helper, md *
 			clusterv1.MachineDeploymentAvailableCondition,
 		}},
 		patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+			clusterv1.PausedV1Beta2Condition,
 			clusterv1.MachineDeploymentAvailableV1Beta2Condition,
 			clusterv1.MachineDeploymentMachinesReadyV1Beta2Condition,
 			clusterv1.MachineDeploymentMachinesUpToDateV1Beta2Condition,

internal/controllers/machinehealthcheck/machinehealthcheck_controller.go

@@ -132,8 +132,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 	log := ctrl.LoggerFrom(ctx)
 
 	// Fetch the MachineHealthCheck instance
-	m := &clusterv1.MachineHealthCheck{}
-	if err := r.Client.Get(ctx, req.NamespacedName, m); err != nil {
+	mhc := &clusterv1.MachineHealthCheck{}
+	if err := r.Client.Get(ctx, req.NamespacedName, mhc); err != nil {
 		if apierrors.IsNotFound(err) {
 			// Object not found, return.  Created objects are automatically garbage collected.
 			// For additional cleanup logic use finalizers.
@@ -144,22 +144,22 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		return ctrl.Result{}, err
 	}
 
-	log = log.WithValues("Cluster", klog.KRef(m.Namespace, m.Spec.ClusterName))
+	log = log.WithValues("Cluster", klog.KRef(mhc.Namespace, mhc.Spec.ClusterName))
 	ctx = ctrl.LoggerInto(ctx, log)
 
-	cluster, err := util.GetClusterByName(ctx, r.Client, m.Namespace, m.Spec.ClusterName)
+	cluster, err := util.GetClusterByName(ctx, r.Client, mhc.Namespace, mhc.Spec.ClusterName)
 	if err != nil {
 		log.Error(err, "Failed to fetch Cluster for MachineHealthCheck")
 		return ctrl.Result{}, err
 	}
 
-	if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, m); err != nil || isPaused || conditionChanged {
+	// Initialize the patch helper
+	patchHelper, err := patch.NewHelper(mhc, r.Client)
+	if err != nil {
 		return ctrl.Result{}, err
 	}
 
-	// Initialize the patch helper
-	patchHelper, err := patch.NewHelper(m, r.Client)
-	if err != nil {
+	if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, mhc); err != nil || isPaused || requeue {
 		return ctrl.Result{}, err
 	}
 
@@ -171,24 +171,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 				clusterv1.RemediationAllowedCondition,
 			}},
 			patch.WithOwnedV1Beta2Conditions{Conditions: []string{
+				clusterv1.PausedV1Beta2Condition,
 				clusterv1.MachineHealthCheckRemediationAllowedV1Beta2Condition,
 			}},
 		}
 		if reterr == nil {
 			patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
 		}
-		if err := patchHelper.Patch(ctx, m, patchOpts...); err != nil {
+		if err := patchHelper.Patch(ctx, mhc, patchOpts...); err != nil {
 			reterr = kerrors.NewAggregate([]error{reterr, err})
 		}
 	}()
 
 	// Reconcile labels.
-	if m.Labels == nil {
-		m.Labels = make(map[string]string)
+	if mhc.Labels == nil {
+		mhc.Labels = make(map[string]string)
 	}
-	m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName
+	mhc.Labels[clusterv1.ClusterNameLabel] = mhc.Spec.ClusterName
 
-	return r.reconcile(ctx, log, cluster, m)
+	return r.reconcile(ctx, log, cluster, mhc)
 }
 
 func (r *Reconciler) reconcile(ctx context.Context, logger logr.Logger, cluster *clusterv1.Cluster, m *clusterv1.MachineHealthCheck) (ctrl.Result, error) {