🌱 KCP: Add current/desired objects to NotUpToDateResult & refactor object creation (#12817)

* KCP: Add current/desired objects to NotUpToDateResult

Signed-off-by: Stefan Büringer [email protected]

* Fix review findings

* Fix review findings

---------

Signed-off-by: Stefan Büringer [email protected]

Author: sbueringer

api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

@@ -347,6 +347,11 @@ type APIEndpoint struct {
 	BindPort int32 `json:"bindPort,omitempty"`
 }
 
+// IsDefined returns true if the APIEndpoint is defined.
+func (r *APIEndpoint) IsDefined() bool {
+	return r.AdvertiseAddress != "" || r.BindPort != 0
+}
+
 // NodeRegistrationOptions holds fields that relate to registering a new control-plane or node to the cluster, either via "kubeadm init" or "kubeadm join".
 // Note: The NodeRegistrationOptions struct has to be kept in sync with the structs in MarshalJSON.
 // +kubebuilder:validation:MinProperties=1

controlplane/kubeadm/internal/cluster_labels.go

@@ -50,7 +50,12 @@ type ControlPlane struct {
 	Machines             collections.Machines
 	machinesPatchHelpers map[string]*patch.Helper
 
-	machinesNotUptoDate        collections.Machines
+	// MachinesNotUpToDate is the source of truth for Machines that are not up-to-date.
+	// It should be used to check if a Machine is up-to-date (not machinesNotUpToDateResults).
+	MachinesNotUpToDate collections.Machines
+	// machinesNotUpToDateResults is used to store the result of the UpToDate call for all Machines
+	// (even for Machines that are up-to-date).
+	// MachinesNotUpToDate should always be used instead to check if a Machine is up-to-date.
 	machinesNotUpToDateResults map[string]NotUpToDateResult
 
 	// reconciliationTime is the time of the current reconciliation, and should be used for all "now" calculations
@@ -119,22 +124,24 @@ func NewControlPlane(ctx context.Context, managementCluster ManagementCluster, c
 	machinesNotUptoDate := make(collections.Machines, len(ownedMachines))
 	machinesNotUpToDateResults := map[string]NotUpToDateResult{}
 	for _, m := range ownedMachines {
-		upToDate, notUpToDateResult, err := UpToDate(m, kcp, &reconciliationTime, infraMachines, kubeadmConfigs)
+		upToDate, notUpToDateResult, err := UpToDate(ctx, client, cluster, m, kcp, &reconciliationTime, infraMachines, kubeadmConfigs)
 		if err != nil {
 			return nil, err
 		}
 		if !upToDate {
 			machinesNotUptoDate.Insert(m)
-			machinesNotUpToDateResults[m.Name] = *notUpToDateResult
 		}
+		// Set this even if machine is UpToDate. This is needed to complete triggering in-place updates
+		// MachinesNotUpToDate should always be used instead to check if a Machine is up-to-date.
+		machinesNotUpToDateResults[m.Name] = *notUpToDateResult
 	}
 
 	return &ControlPlane{
 		KCP:                        kcp,
 		Cluster:                    cluster,
 		Machines:                   ownedMachines,
 		machinesPatchHelpers:       patchHelpers,
-		machinesNotUptoDate:        machinesNotUptoDate,
+		MachinesNotUpToDate:        machinesNotUptoDate,
 		machinesNotUpToDateResults: machinesNotUpToDateResults,
 		KubeadmConfigs:             kubeadmConfigs,
 		InfraResources:             infraMachines,
@@ -216,25 +223,6 @@ func getGetFailureDomainIDs(failureDomains []clusterv1.FailureDomain) []string {
 	return ids
 }
 
-// InitialControlPlaneConfig returns a new KubeadmConfigSpec that is to be used for an initializing control plane.
-func (c *ControlPlane) InitialControlPlaneConfig() *bootstrapv1.KubeadmConfigSpec {
-	bootstrapSpec := c.KCP.Spec.KubeadmConfigSpec.DeepCopy()
-	// Note: When building a KubeadmConfig for the first CP machine empty out the unnecessary JoinConfiguration.
-	bootstrapSpec.JoinConfiguration = bootstrapv1.JoinConfiguration{}
-	return bootstrapSpec
-}
-
-// JoinControlPlaneConfig returns a new KubeadmConfigSpec that is to be used for joining control planes.
-func (c *ControlPlane) JoinControlPlaneConfig() *bootstrapv1.KubeadmConfigSpec {
-	bootstrapSpec := c.KCP.Spec.KubeadmConfigSpec.DeepCopy()
-	// Note: When building a KubeadmConfig for a joining CP machine empty out the unnecessary InitConfiguration.
-	bootstrapSpec.InitConfiguration = bootstrapv1.InitConfiguration{}
-	// NOTE: For the joining we are preserving the ClusterConfiguration in order to determine if the
-	// cluster is using an external etcd in the kubeadm bootstrap provider (even if this is not required by kubeadm Join).
-	// TODO: Determine if this copy of cluster configuration can be used for rollouts (thus allowing to remove the annotation at machine level)
-	return bootstrapSpec
-}
-
 // HasDeletingMachine returns true if any machine in the control plane is in the process of being deleted.
 func (c *ControlPlane) HasDeletingMachine() bool {
 	return len(c.Machines.Filter(collections.HasDeletionTimestamp)) > 0
@@ -254,19 +242,19 @@ func (c *ControlPlane) GetKubeadmConfig(machineName string) (*bootstrapv1.Kubead
 // MachinesNeedingRollout return a list of machines that need to be rolled out.
 func (c *ControlPlane) MachinesNeedingRollout() (collections.Machines, map[string]NotUpToDateResult) {
 	// Note: Machines already deleted are dropped because they will be replaced by new machines after deletion completes.
-	return c.machinesNotUptoDate.Filter(collections.Not(collections.HasDeletionTimestamp)), c.machinesNotUpToDateResults
+	return c.MachinesNotUpToDate.Filter(collections.Not(collections.HasDeletionTimestamp)), c.machinesNotUpToDateResults
 }
 
 // NotUpToDateMachines return a list of machines that are not up to date with the control
 // plane's configuration.
 func (c *ControlPlane) NotUpToDateMachines() (collections.Machines, map[string]NotUpToDateResult) {
-	return c.machinesNotUptoDate, c.machinesNotUpToDateResults
+	return c.MachinesNotUpToDate, c.machinesNotUpToDateResults
 }
 
 // UpToDateMachines returns the machines that are up to date with the control
 // plane's configuration.
 func (c *ControlPlane) UpToDateMachines() collections.Machines {
-	return c.Machines.Difference(c.machinesNotUptoDate)
+	return c.Machines.Difference(c.MachinesNotUpToDate)
 }
 
 // getInfraMachines fetches the InfraMachine for each machine in the collection and returns a map of machine.Name -> InfraMachine.

controlplane/kubeadm/internal/control_plane.go

@@ -124,13 +124,15 @@ func TestControlPlane(t *testing.T) {
 
 		machinesNotUptoDate, machinesNotUpToDateResults := controlPlane.NotUpToDateMachines()
 		g.Expect(machinesNotUptoDate.Names()).To(ConsistOf("m2", "m3"))
-		g.Expect(machinesNotUpToDateResults).To(HaveLen(2))
+		// machinesNotUpToDateResults contains results for all Machines (including up-to-date Machines).
+		g.Expect(machinesNotUpToDateResults).To(HaveLen(5))
 		g.Expect(machinesNotUpToDateResults["m2"].ConditionMessages).To(Equal([]string{"Version v1.29.0, v1.31.0 required"}))
 		g.Expect(machinesNotUpToDateResults["m3"].ConditionMessages).To(Equal([]string{"Version v1.29.3, v1.31.0 required"}))
 
 		machinesNeedingRollout, machinesNotUpToDateResults := controlPlane.MachinesNeedingRollout()
 		g.Expect(machinesNeedingRollout.Names()).To(ConsistOf("m2"))
-		g.Expect(machinesNotUpToDateResults).To(HaveLen(2))
+		// machinesNotUpToDateResults contains results for all Machines (including up-to-date Machines).
+		g.Expect(machinesNotUpToDateResults).To(HaveLen(5))
 		g.Expect(machinesNotUpToDateResults["m2"].LogMessages).To(Equal([]string{"Machine version \"v1.29.0\" is not equal to KCP version \"v1.31.0\""}))
 		g.Expect(machinesNotUpToDateResults["m3"].LogMessages).To(Equal([]string{"Machine version \"v1.29.3\" is not equal to KCP version \"v1.31.0\""}))
 
@@ -339,7 +341,7 @@ func TestStatusToLogKeyAndValues(t *testing.T) {
 	c := &ControlPlane{
 		KCP:                 &controlplanev1.KubeadmControlPlane{},
 		Machines:            collections.FromMachines(healthyMachine, machineWithoutNode, machineJustDeleted, machineNotUpToDate, machineMarkedForRemediation),
-		machinesNotUptoDate: collections.FromMachines(machineNotUpToDate),
+		MachinesNotUpToDate: collections.FromMachines(machineNotUpToDate),
 		EtcdMembers:         []*etcd.Member{{Name: "m1"}, {Name: "m2"}, {Name: "m3"}},
 	}
 

controlplane/kubeadm/internal/control_plane_test.go

@@ -802,7 +802,11 @@ func (r *KubeadmControlPlaneReconciler) syncMachines(ctx context.Context, contro
 		if err != nil {
 			return errors.Wrapf(err, "failed to update Machine: %s", klog.KObj(m))
 		}
+		// Note: Ensure ControlPlane has the latest version of the Machine.
 		controlPlane.Machines[machineName] = updatedMachine
+		if _, ok := controlPlane.MachinesNotUpToDate[machineName]; ok {
+			controlPlane.MachinesNotUpToDate[machineName] = updatedMachine
+		}
 		// Since the machine is updated, re-create the patch helper so that any subsequent
 		// Patch calls use the correct base machine object to calculate the diffs.
 		// Example: reconcileControlPlaneAndMachinesConditions patches the machine objects in a subsequent call

controlplane/kubeadm/internal/controllers/controller.go

@@ -52,6 +52,7 @@ import (
 	"sigs.k8s.io/cluster-api/controllers/clustercache"
 	"sigs.k8s.io/cluster-api/controllers/external"
 	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
+	"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/desiredstate"
 	controlplanev1webhooks "sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/webhooks"
 	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/contract"
@@ -520,7 +521,7 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: cluster.Namespace,
 					Name:      name,
-					Labels:    internal.ControlPlaneMachineLabelsForCluster(kcp, cluster.Name),
+					Labels:    desiredstate.ControlPlaneMachineLabels(kcp, cluster.Name),
 				},
 				Spec: clusterv1.MachineSpec{
 					Bootstrap: clusterv1.Bootstrap{
@@ -588,7 +589,7 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: cluster.Namespace,
 					Name:      name,
-					Labels:    internal.ControlPlaneMachineLabelsForCluster(kcp, cluster.Name),
+					Labels:    desiredstate.ControlPlaneMachineLabels(kcp, cluster.Name),
 				},
 				Spec: clusterv1.MachineSpec{
 					Bootstrap: clusterv1.Bootstrap{
@@ -703,7 +704,7 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: cluster.Namespace,
 					Name:      name,
-					Labels:    internal.ControlPlaneMachineLabelsForCluster(kcp, cluster.Name),
+					Labels:    desiredstate.ControlPlaneMachineLabels(kcp, cluster.Name),
 				},
 				Spec: clusterv1.MachineSpec{
 					Bootstrap: clusterv1.Bootstrap{
@@ -761,7 +762,7 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
 					ObjectMeta: metav1.ObjectMeta{
 						Namespace: cluster.Namespace,
 						Name:      "test0",
-						Labels:    internal.ControlPlaneMachineLabelsForCluster(kcp, cluster.Name),
+						Labels:    desiredstate.ControlPlaneMachineLabels(kcp, cluster.Name),
 					},
 					Spec: clusterv1.MachineSpec{
 						Bootstrap: clusterv1.Bootstrap{
@@ -1841,7 +1842,7 @@ func TestKubeadmControlPlaneReconciler_syncMachines(t *testing.T) {
 				NodeVolumeDetachTimeoutSeconds: duration5s,
 				NodeDeletionTimeoutSeconds:     duration5s,
 			},
-			ReadinessGates: mandatoryMachineReadinessGates,
+			ReadinessGates: desiredstate.MandatoryMachineReadinessGates,
 		},
 	}
 	g.Expect(env.PatchAndWait(ctx, deletingMachine, client.FieldOwner(kcpManagerName))).To(Succeed())
@@ -3921,17 +3922,17 @@ func TestObjectsPendingDelete(t *testing.T) {
 
 // test utils.
 
-func newFakeClient(initObjs ...client.Object) client.Client {
+func newFakeClient(initObjs ...client.Object) client.WithWatch {
 	return &fakeClient{
 		startTime: time.Now(),
-		Client:    fake.NewClientBuilder().WithObjects(initObjs...).WithStatusSubresource(&controlplanev1.KubeadmControlPlane{}).Build(),
+		WithWatch: fake.NewClientBuilder().WithObjects(initObjs...).WithStatusSubresource(&controlplanev1.KubeadmControlPlane{}).Build(),
 	}
 }
 
 type fakeClient struct {
 	startTime time.Time
 	mux       sync.Mutex
-	client.Client
+	client.WithWatch
 }
 
 type fakeClientI interface {
@@ -3947,7 +3948,7 @@ func (c *fakeClient) Create(ctx context.Context, obj client.Object, opts ...clie
 		f.SetCreationTimestamp(metav1.NewTime(c.startTime))
 		c.mux.Unlock()
 	}
-	return c.Client.Create(ctx, obj, opts...)
+	return c.WithWatch.Create(ctx, obj, opts...)
 }
 
 func createClusterWithControlPlane(namespace string) (*clusterv1.Cluster, *controlplanev1.KubeadmControlPlane, *unstructured.Unstructured) {
@@ -4038,7 +4039,7 @@ func createMachineNodePair(name string, cluster *clusterv1.Cluster, kcp *control
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace:   cluster.Namespace,
 			Name:        name,
-			Labels:      internal.ControlPlaneMachineLabelsForCluster(kcp, cluster.Name),
+			Labels:      desiredstate.ControlPlaneMachineLabels(kcp, cluster.Name),
 			Annotations: map[string]string{},
 			OwnerReferences: []metav1.OwnerReference{
 				*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane")),