in-place propagation from MS to Machines

Author: Yuvaraj Kakaraparthi

internal/controllers/machineset/machineset_controller.go

@@ -28,6 +28,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -42,6 +43,7 @@ import (
 	"sigs.k8s.io/cluster-api/controllers/remote"
 	"sigs.k8s.io/cluster-api/internal/controllers/machine"
 	capilabels "sigs.k8s.io/cluster-api/internal/labels"
+	"sigs.k8s.io/cluster-api/internal/util/ssa"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
 	"sigs.k8s.io/cluster-api/util/collections"
@@ -64,6 +66,8 @@ var (
 	stateConfirmationInterval = 100 * time.Millisecond
 )
 
+const machineSetManagerName = "capi-machineset"
+
 // +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create;patch
 // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch
 // +kubebuilder:rbac:groups=core,resources=nodes,verbs=get;list;watch;create;update;patch;delete
@@ -290,39 +294,6 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 		filteredMachines = append(filteredMachines, machine)
 	}
 
-	// If not already present, add a label specifying the MachineSet name to Machines.
-	// Ensure all required labels exist on the controlled Machines.
-	// This logic is needed to add the `cluster.x-k8s.io/set-name` label to Machines
-	// which were created before the `cluster.x-k8s.io/set-name` label was added to
-	// all Machines created by a MachineSet or if a user manually removed the label.
-	for _, machine := range filteredMachines {
-		mdNameOnMachineSet, mdNameSetOnMachineSet := machineSet.Labels[clusterv1.MachineDeploymentNameLabel]
-		mdNameOnMachine := machine.Labels[clusterv1.MachineDeploymentNameLabel]
-
-		// Note: MustEqualValue is used here as the value of this label will be a hash if the MachineSet name is longer than 63 characters.
-		if msNameLabelValue, ok := machine.Labels[clusterv1.MachineSetNameLabel]; ok && capilabels.MustEqualValue(machineSet.Name, msNameLabelValue) &&
-			(!mdNameSetOnMachineSet || mdNameOnMachineSet == mdNameOnMachine) {
-			// Continue if the MachineSet name label is already set correctly and
-			// either the MachineDeployment name label is not set on the MachineSet or
-			// the MachineDeployment name label is set correctly on the Machine.
-			continue
-		}
-
-		helper, err := patch.NewHelper(machine, r.Client)
-		if err != nil {
-			return ctrl.Result{}, errors.Wrapf(err, "failed to apply %s label to Machine %q", clusterv1.MachineSetNameLabel, machine.Name)
-		}
-		// Note: MustFormatValue is used here as the value of this label will be a hash if the MachineSet name is longer than 63 characters.
-		machine.Labels[clusterv1.MachineSetNameLabel] = capilabels.MustFormatValue(machineSet.Name)
-		// Propagate the MachineDeploymentNameLabel from MachineSet to Machine if it is set on the MachineSet.
-		if mdNameSetOnMachineSet {
-			machine.Labels[clusterv1.MachineDeploymentNameLabel] = mdNameOnMachineSet
-		}
-		if err := helper.Patch(ctx, machine); err != nil {
-			return ctrl.Result{}, errors.Wrapf(err, "failed to apply %s label to Machine %q", clusterv1.MachineSetNameLabel, machine.Name)
-		}
-	}
-
 	// Remediate failed Machines by deleting them.
 	var errs []error
 	for _, machine := range filteredMachines {
@@ -352,6 +323,10 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 		return ctrl.Result{}, errors.Wrap(err, "failed to remediate machines")
 	}
 
+	if err := r.syncMachines(ctx, machineSet, filteredMachines); err != nil {
+		return ctrl.Result{}, errors.Wrap(err, "failed to update Machines")
+	}
+
 	syncErr := r.syncReplicas(ctx, machineSet, filteredMachines)
 
 	// Always updates status as machines come up or die.
@@ -389,6 +364,43 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
 	return ctrl.Result{}, nil
 }
 
+// syncMachines updates Machines to propagate in-place mutable fields from the MachineSet.
+// Note: It also cleans up managed fields of all Machines so that Machines that were created/patched before (< v1.4.0)
+// the controller adopted Server-Side-Apply (SSA) can also work with SSA. Otherwise fields would be co-owned by
+// our "old" "manager" and "capi-machineset" and then we would not be able to e.g. drop labels and annotations.
+// TODO: update the labels and annotations to the corresponding infra machines and the boostrap configs of the filtered machines.
+func (r *Reconciler) syncMachines(ctx context.Context, machineSet *clusterv1.MachineSet, machines []*clusterv1.Machine) error {
+	log := ctrl.LoggerFrom(ctx)
+	for i := range machines {
+		m := machines[i]
+		// If the machine is already being deleted, we don't need to update it.
+		if !m.DeletionTimestamp.IsZero() {
+			continue
+		}
+
+		// Cleanup managed fields of all Machines.
+		// We do this so that Machines that were created/patched before the controller adopted Server-Side-Apply (SSA)
+		// (< v1.4.0) can also work with SSA. Otherwise, fields would be co-owned by our "old" "manager" and
+		// "capi-machineset" and then we would not be able to e.g. drop labels and annotations.
+		if err := ssa.CleanUpManagedFieldsForSSAAdoption(ctx, m, machineSetManagerName, r.Client); err != nil {
+			return errors.Wrapf(err, "failed to update machine: failed to adjust the managedFields of the Machine %q", m.Name)
+		}
+
+		// Update Machine to propagate in-place mutable fields from the MachineSet.
+		updatedMachine := r.computeDesiredMachine(machineSet, m)
+		patchOptions := []client.PatchOption{
+			client.ForceOwnership,
+			client.FieldOwner(machineSetManagerName),
+		}
+		if err := r.Client.Patch(ctx, updatedMachine, client.Apply, patchOptions...); err != nil {
+			log.Error(err, "failed to update Machine", "Machine", klog.KObj(updatedMachine))
+			return errors.Wrapf(err, "failed to update Machine %q", klog.KObj(updatedMachine))
+		}
+		machines[i] = updatedMachine
+	}
+	return nil
+}
+
 // syncReplicas scales Machine resources up or down.
 func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet, machines []*clusterv1.Machine) error {
 	log := ctrl.LoggerFrom(ctx)
@@ -414,18 +426,18 @@ func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet,
 		for i := 0; i < diff; i++ {
 			// Create a new logger so the global logger is not modified.
 			log := log
-			machine := r.getNewMachine(ms)
-
+			machine := r.computeDesiredMachine(ms, nil)
 			// Clone and set the infrastructure and bootstrap references.
 			var (
 				infraRef, bootstrapRef *corev1.ObjectReference
 				err                    error
 			)
 
-			if machine.Spec.Bootstrap.ConfigRef != nil {
+			// Create the BootstrapConfig if necessary.
+			if ms.Spec.Template.Spec.Bootstrap.ConfigRef != nil {
 				bootstrapRef, err = external.CreateFromTemplate(ctx, &external.CreateFromTemplateInput{
 					Client:      r.Client,
-					TemplateRef: machine.Spec.Bootstrap.ConfigRef,
+					TemplateRef: ms.Spec.Template.Spec.Bootstrap.ConfigRef,
 					Namespace:   machine.Namespace,
 					ClusterName: machine.Spec.ClusterName,
 					Labels:      machine.Labels,
@@ -439,15 +451,18 @@ func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet,
 				})
 				if err != nil {
 					conditions.MarkFalse(ms, clusterv1.MachinesCreatedCondition, clusterv1.BootstrapTemplateCloningFailedReason, clusterv1.ConditionSeverityError, err.Error())
-					return errors.Wrapf(err, "failed to clone bootstrap configuration from %s %s while creating a machine", machine.Spec.Bootstrap.ConfigRef.Kind, klog.KRef(machine.Spec.Bootstrap.ConfigRef.Namespace, machine.Spec.Bootstrap.ConfigRef.Name))
+					return errors.Wrapf(err, "failed to clone bootstrap configuration from %s %s while creating a machine",
+						ms.Spec.Template.Spec.Bootstrap.ConfigRef.Kind,
+						klog.KRef(ms.Spec.Template.Spec.Bootstrap.ConfigRef.Namespace, ms.Spec.Template.Spec.Bootstrap.ConfigRef.Name))
 				}
 				machine.Spec.Bootstrap.ConfigRef = bootstrapRef
 				log = log.WithValues(bootstrapRef.Kind, klog.KRef(bootstrapRef.Namespace, bootstrapRef.Name))
 			}
 
+			// Create the InfraMachine.
 			infraRef, err = external.CreateFromTemplate(ctx, &external.CreateFromTemplateInput{
 				Client:      r.Client,
-				TemplateRef: &machine.Spec.InfrastructureRef,
+				TemplateRef: &ms.Spec.Template.Spec.InfrastructureRef,
 				Namespace:   machine.Namespace,
 				ClusterName: machine.Spec.ClusterName,
 				Labels:      machine.Labels,
@@ -461,12 +476,19 @@ func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet,
 			})
 			if err != nil {
 				conditions.MarkFalse(ms, clusterv1.MachinesCreatedCondition, clusterv1.InfrastructureTemplateCloningFailedReason, clusterv1.ConditionSeverityError, err.Error())
-				return errors.Wrapf(err, "failed to clone infrastructure machine from %s %s while creating a machine", machine.Spec.InfrastructureRef.Kind, klog.KRef(machine.Spec.InfrastructureRef.Namespace, machine.Spec.InfrastructureRef.Name))
+				return errors.Wrapf(err, "failed to clone infrastructure machine from %s %s while creating a machine",
+					ms.Spec.Template.Spec.InfrastructureRef.Kind,
+					klog.KRef(ms.Spec.Template.Spec.InfrastructureRef.Namespace, ms.Spec.Template.Spec.InfrastructureRef.Name))
 			}
 			log = log.WithValues(infraRef.Kind, klog.KRef(infraRef.Namespace, infraRef.Name))
 			machine.Spec.InfrastructureRef = *infraRef
 
-			if err := r.Client.Create(ctx, machine); err != nil {
+			// Create the Machine.
+			patchOptions := []client.PatchOption{
+				client.ForceOwnership,
+				client.FieldOwner(machineSetManagerName),
+			}
+			if err := r.Client.Patch(ctx, machine, client.Apply, patchOptions...); err != nil {
 				log.Error(err, "Error while creating a machine")
 				r.recorder.Eventf(ms, corev1.EventTypeWarning, "FailedCreate", "Failed to create machine: %v", err)
 				errs = append(errs, err)
@@ -529,45 +551,87 @@ func (r *Reconciler) syncReplicas(ctx context.Context, ms *clusterv1.MachineSet,
 	return nil
 }
 
-// getNewMachine creates a new Machine object. The name of the newly created resource is going
-// to be created by the API server, we set the generateName field.
-func (r *Reconciler) getNewMachine(machineSet *clusterv1.MachineSet) *clusterv1.Machine {
-	gv := clusterv1.GroupVersion
-	machine := &clusterv1.Machine{
+// computeDesiredMachine computes the desired Machine.
+// This Machine will be used during reconciliation to:
+// * create a Machine
+// * update an existing Machine
+// Because we are using Server-Side-Apply we always have to calculate the full object.
+// There are small differences in how we calculate the Machine depending on if it
+// is a create or update. Example: for a new Machine we have to calculate a new name,
+// while for an existing Machine we have to use the name of the existing Machine.
+func (r *Reconciler) computeDesiredMachine(machineSet *clusterv1.MachineSet, existingMachine *clusterv1.Machine) *clusterv1.Machine {
+	desiredMachine := &clusterv1.Machine{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: clusterv1.GroupVersion.String(),
+			Kind:       "Machine",
+		},
 		ObjectMeta: metav1.ObjectMeta{
-			GenerateName: fmt.Sprintf("%s-", machineSet.Name),
-			// Note: by setting the ownerRef on creation we signal to the Machine controller that this is not a stand-alone Machine.
+			Name:      names.SimpleNameGenerator.GenerateName(fmt.Sprintf("%s-", machineSet.Name)),
+			Namespace: machineSet.Namespace,
+			// Note: By setting the ownerRef on creation we signal to the Machine controller that this is not a stand-alone Machine.
 			OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(machineSet, machineSetKind)},
-			Namespace:       machineSet.Namespace,
-			Labels:          make(map[string]string),
-			Annotations:     machineSet.Spec.Template.Annotations,
-		},
-		TypeMeta: metav1.TypeMeta{
-			Kind:       gv.WithKind("Machine").Kind,
-			APIVersion: gv.String(),
+			Labels:          map[string]string{},
+			Annotations:     map[string]string{},
 		},
-		Spec: machineSet.Spec.Template.Spec,
-	}
-	machine.Spec.ClusterName = machineSet.Spec.ClusterName
-
-	// Set the labels from machineSet.Spec.Template.Labels as labels for the new Machine.
+		Spec: *machineSet.Spec.Template.Spec.DeepCopy(),
+	}
+	// Set ClusterName.
+	desiredMachine.Spec.ClusterName = machineSet.Spec.ClusterName
+
+	// Clean up the refs to the incorrect objects.
+	// The InfrastructureRef and the Bootstrap.ConfigRef in Machine should point to the InfrastructureMachine
+	// and the BootstrapConfig objects. In the MachineSet these values point to InfrastructureMachineTemplate
+	// BootstrapConfigTemplate. Drop the values that were copied over from MachineSet during DeepCopy
+	// to make sure to not point to incorrect refs.
+	// Note: During Machine creation, these refs will be updated with the correct values after the corresponding
+	// objects are created.
+	desiredMachine.Spec.InfrastructureRef = corev1.ObjectReference{}
+	desiredMachine.Spec.Bootstrap.ConfigRef = nil
+
+	// If we are updating an existing Machine reuse the name, uid, infrastructureRef and bootstrap.configRef
+	// from the existingMachine.
+	// Note: we use UID to force SSA to update the existing Machine and to not accidentally create a new Machine.
+	// infrastructureRef and bootstrap.configRef remain the same for an existing Machine.
+	if existingMachine != nil {
+		desiredMachine.SetName(existingMachine.Name)
+		desiredMachine.SetUID(existingMachine.UID)
+		desiredMachine.Spec.Bootstrap.ConfigRef = existingMachine.Spec.Bootstrap.ConfigRef
+		desiredMachine.Spec.InfrastructureRef = existingMachine.Spec.InfrastructureRef
+	}
+
+	// Set the in-place mutable fields.
+	// When we create a new Machine we will just create the Machine with those fields.
+	// When we update an existing Machine will we update the fields on the existing Machine (in-place mutate).
+
+	// Set Labels
 	// Note: We can't just set `machineSet.Spec.Template.Labels` directly and thus "share" the labels
 	// map between Machine and machineSet.Spec.Template.Labels. This would mean that adding the
 	// MachineSetNameLabel and MachineDeploymentNameLabel later on the Machine would also add the labels
 	// to machineSet.Spec.Template.Labels and thus modify the labels of the MachineSet.
 	for k, v := range machineSet.Spec.Template.Labels {
-		machine.Labels[k] = v
+		desiredMachine.Labels[k] = v
 	}
-
-	// Enforce that the MachineSetNameLabel label is set
-	// Note: the MachineSetNameLabel is added by the default webhook to MachineSet.spec.template.labels if a spec.selector is empty.
-	machine.Labels[clusterv1.MachineSetNameLabel] = capilabels.MustFormatValue(machineSet.Name)
+	// Always set the MachineSetNameLabel.
+	// Note: If a client tries to create a MachineSet without a selector, the MachineSet webhook
+	// will add this label automatically. But we want this label to always be present even if the MachineSet
+	// has a selector which doesn't include it. Therefore, we have to set it here explicitly.
+	desiredMachine.Labels[clusterv1.MachineSetNameLabel] = capilabels.MustFormatValue(machineSet.Name)
 	// Propagate the MachineDeploymentNameLabel from MachineSet to Machine if it exists.
 	if mdName, ok := machineSet.Labels[clusterv1.MachineDeploymentNameLabel]; ok {
-		machine.Labels[clusterv1.MachineDeploymentNameLabel] = mdName
+		desiredMachine.Labels[clusterv1.MachineDeploymentNameLabel] = mdName
+	}
+
+	// Set Annotations
+	for k, v := range machineSet.Spec.Template.Annotations {
+		desiredMachine.Annotations[k] = v
 	}
 
-	return machine
+	// Set all other in-place mutable fields.
+	desiredMachine.Spec.NodeDrainTimeout = machineSet.Spec.Template.Spec.NodeDrainTimeout
+	desiredMachine.Spec.NodeDeletionTimeout = machineSet.Spec.Template.Spec.NodeDeletionTimeout
+	desiredMachine.Spec.NodeVolumeDetachTimeout = machineSet.Spec.Template.Spec.NodeVolumeDetachTimeout
+
+	return desiredMachine
 }
 
 // shouldExcludeMachine returns true if the machine should be filtered out, false otherwise.