🌱 Move up to date condition to ms for workers (#12959)

* Move setUpToDate condition from MS controller to Machine controller

* Use MS and MD from scope instead of reading them again

* Introduce util for isUpdating

Author: fabriziopandini

api/core/v1beta2/machine_types.go

@@ -162,6 +162,10 @@ const (
 
 	// MachineNotUpToDateReason surface when a Machine spec does not match the spec of the Machine's owner resource, e.g. KubeadmControlPlane or MachineDeployment.
 	MachineNotUpToDateReason = "NotUpToDate"
+
+	// MachineUpToDateUpdatingReason surface when a Machine spec matches the spec of the Machine's owner resource,
+	// but the Machine is still updating in-place.
+	MachineUpToDateUpdatingReason = "Updating"
 )
 
 // Machine's Updating condition and corresponding reasons.

controlplane/kubeadm/internal/controllers/controller.go

@@ -48,6 +48,7 @@ import (
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
 	runtimeclient "sigs.k8s.io/cluster-api/exp/runtime/client"
 	"sigs.k8s.io/cluster-api/feature"
+	"sigs.k8s.io/cluster-api/internal/util/inplace"
 	"sigs.k8s.io/cluster-api/internal/util/ssa"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/cache"
@@ -1039,6 +1040,16 @@ func reconcileMachineUpToDateCondition(_ context.Context, controlPlane *internal
 			})
 			continue
 		}
+
+		if inplace.IsUpdateInProgress(machine) {
+			conditions.Set(machine, metav1.Condition{
+				Type:   clusterv1.MachineUpToDateCondition,
+				Status: metav1.ConditionFalse,
+				Reason: clusterv1.MachineUpToDateUpdatingReason,
+			})
+			continue
+		}
+
 		conditions.Set(machine, metav1.Condition{
 			Type:   clusterv1.MachineUpToDateCondition,
 			Status: metav1.ConditionTrue,

controlplane/kubeadm/internal/controllers/controller_test.go

@@ -2322,6 +2322,90 @@ func TestKubeadmControlPlaneReconciler_reconcileControlPlaneAndMachinesCondition
 				},
 			},
 		},
+		{
+			name: "Machines in place updating, machine not up-to-date date",
+			controlPlane: func() *internal.ControlPlane {
+				controlPlane, err := internal.NewControlPlane(ctx, nil, env.GetClient(), defaultCluster, defaultKCP.DeepCopy(), collections.FromMachines(
+					func() *clusterv1.Machine {
+						m := defaultMachine1.DeepCopy()
+						m.Annotations = map[string]string{
+							clusterv1.UpdateInProgressAnnotation: "",
+						}
+						return m
+					}(),
+				))
+				if err != nil {
+					panic(err)
+				}
+				return controlPlane
+			}(),
+			managementCluster: &fakeManagementCluster{
+				Workload: &fakeWorkloadCluster{
+					Workload: &internal.Workload{
+						Client: fake.NewClientBuilder().Build(),
+					},
+				},
+			},
+			lastProbeSuccessTime: now.Add(-3 * time.Minute),
+			expectKCPConditions: []metav1.Condition{
+				{
+					Type:   controlplanev1.KubeadmControlPlaneInitializedCondition,
+					Status: metav1.ConditionTrue,
+					Reason: controlplanev1.KubeadmControlPlaneInitializedReason,
+				},
+				{
+					Type:   controlplanev1.KubeadmControlPlaneEtcdClusterHealthyCondition,
+					Status: metav1.ConditionUnknown,
+					Reason: controlplanev1.KubeadmControlPlaneEtcdClusterHealthUnknownReason,
+					Message: "* Machine machine1-test:\n" +
+						"  * EtcdMemberHealthy: Waiting for a Node with spec.providerID foo to exist",
+				},
+				{
+					Type:   controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthyCondition,
+					Status: metav1.ConditionUnknown,
+					Reason: controlplanev1.KubeadmControlPlaneControlPlaneComponentsHealthUnknownReason,
+					Message: "* Machine machine1-test:\n" +
+						"  * Control plane components: Waiting for a Node with spec.providerID foo to exist",
+				},
+			},
+			expectMachineConditions: []metav1.Condition{
+				{
+					Type:    controlplanev1.KubeadmControlPlaneMachineAPIServerPodHealthyCondition,
+					Status:  metav1.ConditionUnknown,
+					Reason:  controlplanev1.KubeadmControlPlaneMachinePodInspectionFailedReason,
+					Message: "Waiting for a Node with spec.providerID foo to exist",
+				},
+				{
+					Type:    controlplanev1.KubeadmControlPlaneMachineControllerManagerPodHealthyCondition,
+					Status:  metav1.ConditionUnknown,
+					Reason:  controlplanev1.KubeadmControlPlaneMachinePodInspectionFailedReason,
+					Message: "Waiting for a Node with spec.providerID foo to exist",
+				},
+				{
+					Type:    controlplanev1.KubeadmControlPlaneMachineSchedulerPodHealthyCondition,
+					Status:  metav1.ConditionUnknown,
+					Reason:  controlplanev1.KubeadmControlPlaneMachinePodInspectionFailedReason,
+					Message: "Waiting for a Node with spec.providerID foo to exist",
+				},
+				{
+					Type:    controlplanev1.KubeadmControlPlaneMachineEtcdPodHealthyCondition,
+					Status:  metav1.ConditionUnknown,
+					Reason:  controlplanev1.KubeadmControlPlaneMachinePodInspectionFailedReason,
+					Message: "Waiting for a Node with spec.providerID foo to exist",
+				},
+				{
+					Type:    controlplanev1.KubeadmControlPlaneMachineEtcdMemberHealthyCondition,
+					Status:  metav1.ConditionUnknown,
+					Reason:  controlplanev1.KubeadmControlPlaneMachineEtcdMemberInspectionFailedReason,
+					Message: "Waiting for a Node with spec.providerID foo to exist",
+				},
+				{
+					Type:   clusterv1.MachineUpToDateCondition,
+					Status: metav1.ConditionFalse,
+					Reason: clusterv1.MachineUpToDateUpdatingReason,
+				},
+			},
+		},
 		{
 			name: "Machines not up to date",
 			controlPlane: func() *internal.ControlPlane {

internal/controllers/machine/machine_controller.go

@@ -254,6 +254,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
 		machine: m,
 	}
 
+	//  If the Machine is controlled by a MachineSet, read it.
+	s.owningMachineSet, err = r.getOwnerMachineSet(ctx, s.machine)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	//  If the MachineSet is controlled by a MachineDeployment, get it as well.
+	s.owningMachineDeployment, err = r.getOwnerMachineDeployment(ctx, s.owningMachineSet)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
 	defer func() {
 		updateRes := r.updateStatus(ctx, s)
 		retres = util.LowestNonZeroResult(retres, updateRes)
@@ -296,6 +308,35 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
 	return doReconcile(ctx, reconcileNormal, s)
 }
 
+func (r *Reconciler) getOwnerMachineSet(ctx context.Context, m *clusterv1.Machine) (*clusterv1.MachineSet, error) {
+	machineSetKey, notFound, err := getOwnerMachineSetObjectKey(m.ObjectMeta)
+	if err != nil || notFound || machineSetKey == nil {
+		return nil, err
+	}
+	ms := &clusterv1.MachineSet{}
+	if err := r.Client.Get(ctx, *machineSetKey, ms); err != nil {
+		return nil, fmt.Errorf("failed to retrieve owner MachineSet for Machine %s: %w", klog.KObj(m), err)
+	}
+	return ms, nil
+}
+
+func (r *Reconciler) getOwnerMachineDeployment(ctx context.Context, ms *clusterv1.MachineSet) (*clusterv1.MachineDeployment, error) {
+	if ms == nil {
+		return nil, nil
+	}
+
+	mdName := ms.Labels[clusterv1.MachineDeploymentNameLabel]
+	if mdName == "" {
+		return nil, nil
+	}
+
+	md := &clusterv1.MachineDeployment{}
+	if err := r.Client.Get(ctx, client.ObjectKey{Namespace: ms.Namespace, Name: mdName}, md); err != nil {
+		return nil, fmt.Errorf("failed to retrieve owner MachineDeployment for MachineSet %s: %w", klog.KObj(ms), err)
+	}
+	return md, nil
+}
+
 func patchMachine(ctx context.Context, patchHelper *patch.Helper, machine *clusterv1.Machine, options ...patch.Option) error {
 	// Always update the readyCondition by summarizing the state of other conditions.
 	// A step counter is added to represent progress during the provisioning process (instead we are hiding it
@@ -378,6 +419,14 @@ type scope struct {
 	// of the reconcile function.
 	machine *clusterv1.Machine
 
+	// owningMachineSet return the MachineSet owning this machine.
+	// Note: The value will be nil in case of stand-alone machines.
+	owningMachineSet *clusterv1.MachineSet
+
+	// owningMachineDeployment return the MachineDeployment controlling the MachineSet owning this machine.
+	// Note: The value will be nil in case of stand-alone Machines or in case of stand-alone MachineSet.
+	owningMachineDeployment *clusterv1.MachineDeployment
+
 	// infraMachine is the Infrastructure Machine object that is referenced by the
 	// Machine. It is set after reconcileInfrastructure is called.
 	infraMachine *unstructured.Unstructured

internal/controllers/machine/machine_controller_noderef.go

@@ -24,7 +24,6 @@ import (
 
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -143,7 +142,7 @@ func (r *Reconciler) reconcileNode(ctx context.Context, s *scope) (ctrl.Result,
 	_, nodeHadInterruptibleLabel := s.node.Labels[clusterv1.InterruptibleLabel]
 
 	// Reconcile node taints
-	if err := r.patchNode(ctx, remoteClient, s.node, nodeLabels, nodeAnnotations, machine); err != nil {
+	if err := r.patchNode(ctx, remoteClient, s.node, nodeLabels, nodeAnnotations, s.owningMachineSet, s.owningMachineDeployment); err != nil {
 		return ctrl.Result{}, errors.Wrapf(err, "failed to reconcile Node %s", klog.KObj(s.node))
 	}
 	if !nodeHadInterruptibleLabel && interruptible {
@@ -246,7 +245,7 @@ func (r *Reconciler) getNode(ctx context.Context, c client.Reader, providerID st
 
 // PatchNode is required to workaround an issue on Node.Status.Address which is incorrectly annotated as patchStrategy=merge
 // and this causes SSA patch to fail in case there are two addresses with the same key https://github.com/kubernetes-sigs/cluster-api/issues/8417
-func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client, node *corev1.Node, newLabels, newAnnotations map[string]string, m *clusterv1.Machine) error {
+func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client, node *corev1.Node, newLabels, newAnnotations map[string]string, ms *clusterv1.MachineSet, md *clusterv1.MachineDeployment) error {
 	newNode := node.DeepCopy()
 
 	// Adds the annotations from the Machine.
@@ -336,20 +335,14 @@ func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client,
 	hasTaintChanges := taints.RemoveNodeTaint(newNode, clusterv1.NodeUninitializedTaint)
 
 	// Set Taint to a node in an old MachineSet and unset Taint from a node in a new MachineSet
-	isOutdated, notFound, err := shouldNodeHaveOutdatedTaint(ctx, r.Client, m)
+	isOutdated, err := shouldNodeHaveOutdatedTaint(ms, md)
 	if err != nil {
 		return errors.Wrapf(err, "failed to check if Node %s is outdated", klog.KRef("", node.Name))
 	}
-
-	// It is only possible to identify if we have to set or remove the NodeOutdatedRevisionTaint if shouldNodeHaveOutdatedTaint
-	// found all relevant objects.
-	// Example: when the MachineDeployment or Machineset can't be found due to a background deletion of objects.
-	if !notFound {
-		if isOutdated {
-			hasTaintChanges = taints.EnsureNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
-		} else {
-			hasTaintChanges = taints.RemoveNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
-		}
+	if isOutdated {
+		hasTaintChanges = taints.EnsureNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
+	} else {
+		hasTaintChanges = taints.RemoveNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
 	}
 
 	if !hasAnnotationChanges && !hasLabelChanges && !hasTaintChanges {
@@ -362,47 +355,23 @@ func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client,
 // shouldNodeHaveOutdatedTaint tries to compare the revision of the owning MachineSet to the MachineDeployment.
 // It returns notFound = true if the OwnerReference is not set or the APIServer returns NotFound for the MachineSet or MachineDeployment.
 // Note: This three cases could happen during background deletion of objects.
-func shouldNodeHaveOutdatedTaint(ctx context.Context, c client.Client, m *clusterv1.Machine) (outdated bool, notFound bool, err error) {
-	if _, hasLabel := m.Labels[clusterv1.MachineDeploymentNameLabel]; !hasLabel {
-		return false, false, nil
+func shouldNodeHaveOutdatedTaint(ms *clusterv1.MachineSet, md *clusterv1.MachineDeployment) (outdated bool, err error) {
+	if ms == nil || md == nil {
+		return false, nil
 	}
 
-	// Resolve the MachineSet name via owner references because the label value
-	// could also be a hash.
-	objKey, notFound, err := getOwnerMachineSetObjectKey(m.ObjectMeta)
-	if err != nil || notFound {
-		return false, notFound, err
-	}
-	ms := &clusterv1.MachineSet{}
-	if err := c.Get(ctx, *objKey, ms); err != nil {
-		if apierrors.IsNotFound(err) {
-			return false, true, nil
-		}
-		return false, false, err
-	}
-	md := &clusterv1.MachineDeployment{}
-	objKey = &client.ObjectKey{
-		Namespace: m.Namespace,
-		Name:      m.Labels[clusterv1.MachineDeploymentNameLabel],
-	}
-	if err := c.Get(ctx, *objKey, md); err != nil {
-		if apierrors.IsNotFound(err) {
-			return false, true, nil
-		}
-		return false, false, err
-	}
 	msRev, err := mdutil.Revision(ms)
 	if err != nil {
-		return false, false, err
+		return false, err
 	}
 	mdRev, err := mdutil.Revision(md)
 	if err != nil {
-		return false, false, err
+		return false, err
 	}
 	if msRev < mdRev {
-		return true, false, nil
+		return true, nil
 	}
-	return false, false, nil
+	return false, nil
 }
 
 func getOwnerMachineSetObjectKey(obj metav1.ObjectMeta) (*client.ObjectKey, bool, error) {