Merge pull request #3234 from sedefsavas/3170

🐛 KubeadmControlPlane shouldn't rely on hashing to determine if a Machine is outdated

api/v1alpha3/common_types.go

@@ -39,11 +39,11 @@ const (
 	PausedAnnotation = "cluster.x-k8s.io/paused"
 
 	// TemplateClonedFromNameAnnotation is the infrastructure machine annotation that stores the name of the infrastructure template resource
-	// that was cloned for the machine.
+	// that was cloned for the machine. This annotation is set only during cloning a template. Older/adopted machines will not have this annotation.
 	TemplateClonedFromNameAnnotation = "cluster.x-k8s.io/cloned-from-name"
 
 	// TemplateClonedFromGroupKindAnnotation is the infrastructure machine annotation that stores the group-kind of the infrastructure template resource
-	// that was cloned for the machine.
+	// that was cloned for the machine. This annotation is set only during cloning a template. Older/adopted machines will not have this annotation.
 	TemplateClonedFromGroupKindAnnotation = "cluster.x-k8s.io/cloned-from-groupkind"
 
 	// ClusterSecretType defines the type of secret created by core components

controlplane/kubeadm/api/v1alpha3/kubeadm_control_plane_types.go

@@ -31,6 +31,10 @@ const (
 
 	// SkipCoreDNSAnnotation annotation explicitly skips reconciling CoreDNS if set
 	SkipCoreDNSAnnotation = "controlplane.cluster.x-k8s.io/skip-coredns"
+
+	// KubeadmClusterConfigurationAnnotation is a machine annotation that stores the json-marshalled string of KCP ClusterConfiguration.
+	// This annotation is used to detect any changes in ClusterConfiguration and trigger machine rollout in KCP.
+	KubeadmClusterConfigurationAnnotation = "controlplane.cluster.x-k8s.io/kubeadm-cluster-configuration"
 )
 
 // KubeadmControlPlaneSpec defines the desired state of KubeadmControlPlane.

controlplane/kubeadm/controllers/controller.go

@@ -291,14 +291,14 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, cluster *
 	conditions.SetAggregate(controlPlane.KCP, controlplanev1.MachinesReadyCondition, ownedMachines.ConditionGetters(), conditions.AddSourceRef())
 
 	// Control plane machines rollout due to configuration changes (e.g. upgrades) takes precedence over other operations.
-	needRollout := controlPlane.MachinesNeedingRollout()
+	needRollout := r.machinesNeedingRollout(ctx, controlPlane)
 	switch {
 	case len(needRollout) > 0:
 		logger.Info("Rolling out Control Plane machines")
 		// NOTE: we are using Status.UpdatedReplicas from the previous reconciliation only to provide a meaningful message
 		// and this does not influence any reconciliation logic.
 		conditions.MarkFalse(controlPlane.KCP, controlplanev1.MachinesSpecUpToDateCondition, controlplanev1.RollingUpdateInProgressReason, clusterv1.ConditionSeverityWarning, "Rolling %d replicas with outdated spec (%d replicas up to date)", len(needRollout), kcp.Status.UpdatedReplicas)
-		return r.upgradeControlPlane(ctx, cluster, kcp, controlPlane)
+		return r.upgradeControlPlane(ctx, cluster, kcp, controlPlane, needRollout)
 	default:
 		// make sure last upgrade operation is marked as completed.
 		// NOTE: we are checking the condition already exists in order to avoid to set this condition at the first
@@ -327,7 +327,8 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, cluster *
 	// We are scaling down
 	case numMachines > desiredReplicas:
 		logger.Info("Scaling down control plane", "Desired", desiredReplicas, "Existing", numMachines)
-		return r.scaleDownControlPlane(ctx, cluster, kcp, controlPlane)
+		// The last parameter (i.e. machines needing to be rolled out) should always be empty here.
+		return r.scaleDownControlPlane(ctx, cluster, kcp, controlPlane, internal.FilterableMachineCollection{})
 	}
 
 	// Get the workload cluster client.

controlplane/kubeadm/controllers/helpers.go

@@ -18,6 +18,7 @@ package controllers
 
 import (
 	"context"
+	"encoding/json"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -33,6 +34,7 @@ import (
 	controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/hash"
+	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/machinefilters"
 	capierrors "sigs.k8s.io/cluster-api/errors"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/certs"
@@ -241,8 +243,36 @@ func (r *KubeadmControlPlaneReconciler) generateMachine(ctx context.Context, kcp
 		},
 	}
 
+	// Machine's bootstrap config may be missing ClusterConfiguration if it is not the first machine in the control plane.
+	// We store ClusterConfiguration as annotation here to detect any changes in KCP ClusterConfiguration and rollout the machine if any.
+	clusterConfig, err := json.Marshal(kcp.Spec.KubeadmConfigSpec.ClusterConfiguration)
+	if err != nil {
+		return errors.Wrap(err, "failed to marshal cluster configuration")
+	}
+	machine.SetAnnotations(map[string]string{controlplanev1.KubeadmClusterConfigurationAnnotation: string(clusterConfig)})
+
 	if err := r.Client.Create(ctx, machine); err != nil {
-		return errors.Wrap(err, "Failed to create machine")
+		return errors.Wrap(err, "failed to create machine")
 	}
 	return nil
 }
+
+// machinesNeedingRollout return a list of machines that need to be rolled out.
+func (r *KubeadmControlPlaneReconciler) machinesNeedingRollout(ctx context.Context, c *internal.ControlPlane) internal.FilterableMachineCollection {
+	now := metav1.Now()
+
+	// Ignore machines to be deleted.
+	machines := c.Machines.Filter(machinefilters.Not(machinefilters.HasDeletionTimestamp))
+
+	// Return machines if their creation timestamp is older than the KCP.Spec.UpgradeAfter, or any machine with an outdated configuration.
+	if c.KCP.Spec.UpgradeAfter != nil && c.KCP.Spec.UpgradeAfter.Before(&now) {
+		return machines.Filter(machinefilters.Or(
+			// Machines that are old.
+			machinefilters.OlderThan(c.KCP.Spec.UpgradeAfter),
+			// Machines that do not match with KCP config.
+			machinefilters.Not(machinefilters.MatchesKCPConfiguration(ctx, r.Client, *c.KCP, *c.Cluster)),
+		))
+	}
+
+	return machines.Filter(machinefilters.Not(machinefilters.MatchesKCPConfiguration(ctx, r.Client, *c.KCP, *c.Cluster)))
+}