feat: set new replica fields for machine pools

This updates the MachinePool reconciliation to set the values for the
replica fields in the status section to be consistent with things
like the MachineSets.

If a infra machine pool doesn't have "machine pool machines" then we
can't realistically set the values for all the fields, so in this case
we set the values the same as the replicas.

This change also introduces setting the "UptoDate" condition on the
Machines for the first time. It currently only takes the deletion
timestamp into account when determining if a machine is outdated. We
can iterate on this going forward.

Signed-off-by: Richard Case <[email protected]>

Author: richardcase

exp/internal/controllers/machinepool_controller.go

@@ -49,6 +49,7 @@ import (
 	"sigs.k8s.io/cluster-api/util/conditions"
 	v1beta1conditions "sigs.k8s.io/cluster-api/util/conditions/deprecated/v1beta1"
 	"sigs.k8s.io/cluster-api/util/finalizers"
+	"sigs.k8s.io/cluster-api/util/labels/format"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/cluster-api/util/paused"
 	"sigs.k8s.io/cluster-api/util/predicates"
@@ -62,10 +63,8 @@ import (
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io;bootstrap.cluster.x-k8s.io,resources=*,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinepools;machinepools/status;machinepools/finalizers,verbs=get;list;watch;create;update;patch;delete
 
-var (
-	// machinePoolKind contains the schema.GroupVersionKind for the MachinePool type.
-	machinePoolKind = clusterv1.GroupVersion.WithKind("MachinePool")
-)
+// machinePoolKind contains the schema.GroupVersionKind for the MachinePool type.
+var machinePoolKind = clusterv1.GroupVersion.WithKind("MachinePool")
 
 const (
 	// MachinePoolControllerName defines the controller used when creating clients.
@@ -89,21 +88,6 @@ type MachinePoolReconciler struct {
 	predicateLog *logr.Logger
 }
 
-// scope holds the different objects that are read and used during the reconcile.
-type scope struct {
-	// cluster is the Cluster object the Machine belongs to.
-	// It is set at the beginning of the reconcile function.
-	cluster *clusterv1.Cluster
-
-	// machinePool is the MachinePool object. It is set at the beginning
-	// of the reconcile function.
-	machinePool *clusterv1.MachinePool
-
-	// nodeRefMapResult is a map of providerIDs to Nodes that are associated with the Cluster.
-	// It is set after reconcileInfrastructure is called.
-	nodeRefMap map[string]*corev1.Node
-}
-
 func (r *MachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
 	if r.Client == nil || r.APIReader == nil || r.ClusterCache == nil {
 		return errors.New("Client, APIReader and ClusterCache must not be nil")
@@ -189,7 +173,17 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		return ctrl.Result{}, err
 	}
 
+	// Handle normal reconciliation loop.
+	scope := &scope{
+		cluster:     cluster,
+		machinePool: mp,
+	}
+
 	defer func() {
+		if err := r.updateStatus(ctx, scope); err != nil {
+			reterr = kerrors.NewAggregate([]error{reterr, err})
+		}
+
 		r.reconcilePhase(mp)
 		// TODO(jpang): add support for metrics.
 
@@ -223,79 +217,69 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		}
 	}()
 
-	// Reconcile labels.
-	if mp.Labels == nil {
-		mp.Labels = make(map[string]string)
+	alwaysReconcile := []machinePoolReconcileFunc{
+		wrapErrMachinePoolReconcileFunc(r.reconcileSetOwnerAndLabels, "failed to set MachinePool owner and labels"),
 	}
-	mp.Labels[clusterv1.ClusterNameLabel] = mp.Spec.ClusterName
 
 	// Handle deletion reconciliation loop.
 	if !mp.DeletionTimestamp.IsZero() {
-		return ctrl.Result{}, r.reconcileDelete(ctx, cluster, mp)
+		reconcileDelete := append(
+			alwaysReconcile,
+			wrapErrMachinePoolReconcileFunc(r.reconcileDelete, "failed to reconcile delete"),
+		)
+		return doReconcile(ctx, scope, reconcileDelete)
 	}
 
-	// Handle normal reconciliation loop.
-	scope := &scope{
-		cluster:     cluster,
-		machinePool: mp,
-	}
-	return r.reconcile(ctx, scope)
+	reconcileNormal := append(alwaysReconcile,
+		wrapErrMachinePoolReconcileFunc(r.reconcileBootstrap, "failed to reconcile bootstrap config"),
+		wrapErrMachinePoolReconcileFunc(r.reconcileInfrastructure, "failed to reconcile infrastructure"),
+		wrapErrMachinePoolReconcileFunc(r.getMachinesForMachinePool, "failed to get Machines for MachinePool"),
+		wrapErrMachinePoolReconcileFunc(r.reconcileNodeRefs, "failed to reconcile nodeRefs"),
+		wrapErrMachinePoolReconcileFunc(r.setMachinesUptoDate, "failed to set machines up to date"),
+	)
+
+	return doReconcile(ctx, scope, reconcileNormal)
 }
 
-func (r *MachinePoolReconciler) reconcile(ctx context.Context, s *scope) (ctrl.Result, error) {
-	// Ensure the MachinePool is owned by the Cluster it belongs to.
+func (r *MachinePoolReconciler) reconcileSetOwnerAndLabels(_ context.Context, s *scope) (ctrl.Result, error) {
 	cluster := s.cluster
 	mp := s.machinePool
+
+	if mp.Labels == nil {
+		mp.Labels = make(map[string]string)
+	}
+	mp.Labels[clusterv1.ClusterNameLabel] = mp.Spec.ClusterName
+
 	mp.SetOwnerReferences(util.EnsureOwnerRef(mp.GetOwnerReferences(), metav1.OwnerReference{
 		APIVersion: clusterv1.GroupVersion.String(),
 		Kind:       "Cluster",
 		Name:       cluster.Name,
 		UID:        cluster.UID,
 	}))
 
-	phases := []func(context.Context, *scope) (ctrl.Result, error){
-		r.reconcileBootstrap,
-		r.reconcileInfrastructure,
-		r.reconcileNodeRefs,
-	}
-
-	res := ctrl.Result{}
-	errs := []error{}
-	for _, phase := range phases {
-		// Call the inner reconciliation methods.
-		phaseResult, err := phase(ctx, s)
-		if err != nil {
-			errs = append(errs, err)
-		}
-		if len(errs) > 0 {
-			continue
-		}
-
-		res = util.LowestNonZeroResult(res, phaseResult)
-	}
-	return res, kerrors.NewAggregate(errs)
+	return ctrl.Result{}, nil
 }
 
 // reconcileDelete delete machinePool related resources.
-func (r *MachinePoolReconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster, machinePool *clusterv1.MachinePool) error {
-	if ok, err := r.reconcileDeleteExternal(ctx, machinePool); !ok || err != nil {
+func (r *MachinePoolReconciler) reconcileDelete(ctx context.Context, s *scope) (ctrl.Result, error) {
+	if ok, err := r.reconcileDeleteExternal(ctx, s.machinePool); !ok || err != nil {
 		// Return early and don't remove the finalizer if we got an error or
 		// the external reconciliation deletion isn't ready.
-		return err
+		return ctrl.Result{}, fmt.Errorf("failed deleting external references: %s", err)
 	}
 
 	// check nodes delete timeout passed.
-	if !r.isMachinePoolNodeDeleteTimeoutPassed(machinePool) {
-		if err := r.reconcileDeleteNodes(ctx, cluster, machinePool); err != nil {
+	if !r.isMachinePoolNodeDeleteTimeoutPassed(s.machinePool) {
+		if err := r.reconcileDeleteNodes(ctx, s.cluster, s.machinePool); err != nil {
 			// Return early and don't remove the finalizer if we got an error.
-			return err
+			return ctrl.Result{}, fmt.Errorf("failed deleting nodes: %w", err)
 		}
 	} else {
 		ctrl.LoggerFrom(ctx).Info("NodeDeleteTimeout passed, skipping Nodes deletion")
 	}
 
-	controllerutil.RemoveFinalizer(machinePool, clusterv1.MachinePoolFinalizer)
-	return nil
+	controllerutil.RemoveFinalizer(s.machinePool, clusterv1.MachinePoolFinalizer)
+	return ctrl.Result{}, nil
 }
 
 // reconcileDeleteNodes delete the cluster nodes.
@@ -430,3 +414,110 @@ func (r *MachinePoolReconciler) nodeToMachinePool(ctx context.Context, o client.
 
 	return nil
 }
+
+func (r *MachinePoolReconciler) getMachinesForMachinePool(ctx context.Context, s *scope) (ctrl.Result, error) {
+	infraMachineSelector := metav1.LabelSelector{
+		MatchLabels: map[string]string{
+			clusterv1.MachinePoolNameLabel: format.MustFormatValue(s.machinePool.Name),
+			clusterv1.ClusterNameLabel:     s.machinePool.Spec.ClusterName,
+		},
+	}
+
+	allMachines := &clusterv1.MachineList{}
+	if err := r.Client.List(ctx,
+		allMachines,
+		client.InNamespace(s.machinePool.Namespace),
+		client.MatchingLabels(infraMachineSelector.MatchLabels)); err != nil {
+		return ctrl.Result{}, err
+	}
+
+	filteredMachines := make([]*clusterv1.Machine, 0, len(allMachines.Items))
+	for idx := range allMachines.Items {
+		machine := &allMachines.Items[idx]
+		if shouldExcludeMachine(s.machinePool, machine) {
+			continue
+		}
+		filteredMachines = append(filteredMachines, machine)
+	}
+
+	s.machines = filteredMachines
+
+	return ctrl.Result{}, nil
+}
+
+func (r *MachinePoolReconciler) setMachinesUptoDate(ctx context.Context, s *scope) (ctrl.Result, error) {
+	var errs []error
+	for _, machine := range s.machines {
+		patchHelper, err := patch.NewHelper(machine, r.Client)
+		if err != nil {
+			errs = append(errs, err)
+			continue
+		}
+
+		upToDateCondition := &metav1.Condition{
+			Type: clusterv1.MachineUpToDateCondition,
+		}
+
+		if machine.DeletionTimestamp.IsZero() {
+			upToDateCondition.Status = metav1.ConditionTrue
+			upToDateCondition.Reason = clusterv1.MachineUpToDateReason
+		} else {
+			upToDateCondition.Status = metav1.ConditionFalse
+			upToDateCondition.Reason = clusterv1.MachineNotUpToDateReason
+			upToDateCondition.Message = "Machine is being deleted"
+		}
+		conditions.Set(machine, *upToDateCondition)
+
+		if err := patchHelper.Patch(ctx, machine, patch.WithOwnedConditions{Conditions: []string{
+			clusterv1.MachineUpToDateCondition,
+		}}); err != nil {
+			errs = append(errs, err)
+			continue
+		}
+	}
+
+	if len(errs) > 0 {
+		return ctrl.Result{}, kerrors.NewAggregate(errs)
+	}
+
+	return ctrl.Result{}, nil
+}
+
+type machinePoolReconcileFunc func(ctx context.Context, s *scope) (ctrl.Result, error)
+
+func wrapErrMachinePoolReconcileFunc(f machinePoolReconcileFunc, msg string) machinePoolReconcileFunc {
+	return func(ctx context.Context, s *scope) (ctrl.Result, error) {
+		res, err := f(ctx, s)
+		return res, errors.Wrap(err, msg)
+	}
+}
+
+func doReconcile(ctx context.Context, s *scope, phases []machinePoolReconcileFunc) (ctrl.Result, kerrors.Aggregate) {
+	res := ctrl.Result{}
+	errs := []error{}
+	for _, phase := range phases {
+		// Call the inner reconciliation methods.
+		phaseResult, err := phase(ctx, s)
+		if err != nil {
+			errs = append(errs, err)
+		}
+		if len(errs) > 0 {
+			continue
+		}
+		res = util.LowestNonZeroResult(res, phaseResult)
+	}
+
+	if len(errs) > 0 {
+		return ctrl.Result{}, kerrors.NewAggregate(errs)
+	}
+
+	return res, nil
+}
+
+func shouldExcludeMachine(machinePool *clusterv1.MachinePool, machine *clusterv1.Machine) bool {
+	if metav1.GetControllerOf(machine) != nil && !metav1.IsControlledBy(machine, machinePool) {
+		return true
+	}
+
+	return false
+}

exp/internal/controllers/machinepool_controller_phases.go

@@ -276,6 +276,7 @@ func (r *MachinePoolReconciler) reconcileInfrastructure(ctx context.Context, s *
 		return ctrl.Result{}, err
 	}
 	infraConfig := infraReconcileResult.Result
+	s.infraMachinePool = infraConfig
 
 	if !infraConfig.GetDeletionTimestamp().IsZero() {
 		return ctrl.Result{}, nil