Second review round

internal/controllers/topology/cluster/blueprint.go

@@ -110,7 +110,7 @@ func (r *Reconciler) getBlueprint(ctx context.Context, cluster *clusterv1.Cluste
 		}
 
 		// Get the bootstrap config.
-		machinePoolBlueprint.BootstrapConfig, err = r.getReference(ctx, machinePoolClass.Template.Bootstrap.Ref)
+		machinePoolBlueprint.BootstrapTemplate, err = r.getReference(ctx, machinePoolClass.Template.Bootstrap.Ref)
 		if err != nil {
 			return nil, errors.Wrapf(err, "failed to get bootstrap config for %s, MachinePool class %q", tlog.KObj{Obj: blueprint.ClusterClass}, machinePoolClass.Class)
 		}

internal/controllers/topology/cluster/current_state.go

@@ -290,7 +290,7 @@ func (r *Reconciler) getCurrentMachinePoolState(ctx context.Context, blueprintMa
 
 	// List all the machine pools in the current cluster and in a managed topology.
 	mp := &expv1.MachinePoolList{}
-	err := r.APIReader.List(ctx, mp,
+	err := r.Client.List(ctx, mp,
 		client.MatchingLabels{
 			clusterv1.ClusterNameLabel:          cluster.Name,
 			clusterv1.ClusterTopologyOwnedLabel: "",
@@ -340,7 +340,7 @@ func (r *Reconciler) getCurrentMachinePoolState(ctx context.Context, blueprintMa
 			if !ok {
 				return nil, fmt.Errorf("failed to find MachinePool class %s in ClusterClass", mpClassName)
 			}
-			bootstrapRef, err = alignRefAPIVersion(mpBluePrint.BootstrapConfig, bootstrapRef)
+			bootstrapRef, err = alignRefAPIVersion(mpBluePrint.BootstrapTemplate, bootstrapRef)
 			if err != nil {
 				return nil, errors.Wrap(err, fmt.Sprintf("%s Bootstrap reference could not be retrieved", tlog.KObj{Obj: m}))
 			}

internal/controllers/topology/cluster/desired_state.go

@@ -893,7 +893,7 @@ func (r *Reconciler) computeMachinePools(ctx context.Context, s *scope.Scope) (s
 	for _, mpTopology := range s.Blueprint.Topology.Workers.MachinePools {
 		desiredMachinePool, err := computeMachinePool(ctx, s, mpTopology)
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to compute MachineDepoyment for topology %q", mpTopology.Name)
+			return nil, errors.Wrapf(err, "failed to compute MachinePool for topology %q", mpTopology.Name)
 		}
 		machinePoolsStateMap[mpTopology.Name] = desiredMachinePool
 	}
@@ -925,16 +925,16 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c
 		return nil, errors.Errorf("MachinePool class %s not found in %s", className, tlog.KObj{Obj: s.Blueprint.ClusterClass})
 	}
 
-	// Compute the bootstrap template.
+	// Compute the bootstrap config.
 	currentMachinePool := s.Current.MachinePools[machinePoolTopology.Name]
 	var currentBootstrapConfigRef *corev1.ObjectReference
 	if currentMachinePool != nil && currentMachinePool.BootstrapObject != nil {
 		currentBootstrapConfigRef = currentMachinePool.Object.Spec.Template.Spec.Bootstrap.ConfigRef
 	}
 	var err error
 	desiredMachinePool.BootstrapObject, err = templateToObject(templateToInput{
-		template:              machinePoolBlueprint.BootstrapConfig,
-		templateClonedFromRef: contract.ObjToRef(machinePoolBlueprint.BootstrapConfig),
+		template:              machinePoolBlueprint.BootstrapTemplate,
+		templateClonedFromRef: contract.ObjToRef(machinePoolBlueprint.BootstrapTemplate),
 		cluster:               s.Current.Cluster,
 		namePrefix:            bootstrapTemplateNamePrefix(s.Current.Cluster.Name, machinePoolTopology.Name),
 		currentObjectRef:      currentBootstrapConfigRef,
@@ -951,17 +951,17 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c
 	bootstrapTemplateLabels[clusterv1.ClusterTopologyMachinePoolNameLabel] = machinePoolTopology.Name
 	desiredMachinePool.BootstrapObject.SetLabels(bootstrapTemplateLabels)
 
-	// Compute the Infrastructure template.
-	var currentInfraMachineTemplateRef *corev1.ObjectReference
+	// Compute the Infrastructure ref.
+	var currentInfraMachinePoolRef *corev1.ObjectReference
 	if currentMachinePool != nil && currentMachinePool.InfrastructureMachinePoolObject != nil {
-		currentInfraMachineTemplateRef = &currentMachinePool.Object.Spec.Template.Spec.InfrastructureRef
+		currentInfraMachinePoolRef = &currentMachinePool.Object.Spec.Template.Spec.InfrastructureRef
 	}
 	desiredMachinePool.InfrastructureMachinePoolObject, err = templateToObject(templateToInput{
 		template:              machinePoolBlueprint.InfrastructureMachinePoolTemplate,
 		templateClonedFromRef: contract.ObjToRef(machinePoolBlueprint.InfrastructureMachinePoolTemplate),
 		cluster:               s.Current.Cluster,
-		namePrefix:            infrastructureMachineTemplateNamePrefix(s.Current.Cluster.Name, machinePoolTopology.Name),
-		currentObjectRef:      currentInfraMachineTemplateRef,
+		namePrefix:            infrastructureMachinePoolNamePrefix(s.Current.Cluster.Name, machinePoolTopology.Name),
+		currentObjectRef:      currentInfraMachinePoolRef,
 	})
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to compute infrastructure object for topology %q", machinePoolTopology.Name)
@@ -1002,9 +1002,9 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to calculate desired bootstrap config ref")
 	}
-	desiredInfraMachineTemplateRef, err := calculateRefDesiredAPIVersion(currentInfraMachineTemplateRef, desiredMachinePool.InfrastructureMachinePoolObject)
+	desiredInfraMachinePoolRef, err := calculateRefDesiredAPIVersion(currentInfraMachinePoolRef, desiredMachinePool.InfrastructureMachinePoolObject)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed to calculate desired infrastructure machine template ref")
+		return nil, errors.Wrap(err, "failed to calculate desired infrastructure machine pool ref")
 	}
 
 	desiredMachinePoolObj := &expv1.MachinePool{
@@ -1024,7 +1024,7 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c
 					ClusterName:             s.Current.Cluster.Name,
 					Version:                 pointer.String(version),
 					Bootstrap:               clusterv1.Bootstrap{ConfigRef: desiredBootstrapConfigRef},
-					InfrastructureRef:       *desiredInfraMachineTemplateRef,
+					InfrastructureRef:       *desiredInfraMachinePoolRef,
 					NodeDrainTimeout:        nodeDrainTimeout,
 					NodeVolumeDetachTimeout: nodeVolumeDetachTimeout,
 					NodeDeletionTimeout:     nodeDeletionTimeout,
@@ -1041,6 +1041,9 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c
 
 	// Apply annotations
 	machinePoolAnnotations := util.MergeMap(machinePoolTopology.Metadata.Annotations, machinePoolBlueprint.Metadata.Annotations)
+	// Ensure the annotations used to control the upgrade sequence are never propagated.
+	delete(machinePoolAnnotations, clusterv1.ClusterTopologyHoldUpgradeSequenceAnnotation)
+	delete(machinePoolAnnotations, clusterv1.ClusterTopologyDeferUpgradeAnnotation)
 	desiredMachinePoolObj.SetAnnotations(machinePoolAnnotations)
 	desiredMachinePoolObj.Spec.Template.Annotations = machinePoolAnnotations
 

internal/controllers/topology/cluster/patches/engine.go

@@ -354,12 +354,12 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat
 		}
 
 		// Add the BootstrapTemplate.
-		t, err := newRequestItemBuilder(mpClass.BootstrapConfig).
+		t, err := newRequestItemBuilder(mpClass.BootstrapTemplate).
 			WithHolder(mp.Object, "spec.template.spec.bootstrap.configRef").
 			Build()
 		if err != nil {
 			return nil, errors.Wrapf(err, "failed to prepare BootstrapConfig template %s for MachinePool topology %s for patching",
-				tlog.KObj{Obj: mpClass.BootstrapConfig}, mpTopologyName)
+				tlog.KObj{Obj: mpClass.BootstrapTemplate}, mpTopologyName)
 		}
 		req.Items = append(req.Items, *t)
 
@@ -572,7 +572,7 @@ func updateDesiredState(ctx context.Context, req *runtimehooksv1.GeneratePatches
 	// Update the templates for all MachinePools.
 	for mpTopologyName, mp := range desired.MachinePools {
 		topologyName := requestTopologyName{mpTopologyName: mpTopologyName}
-		// Update the BootstrapConfig.
+		// Update the BootstrapTemplate.
 		bootstrapTemplate, err := getTemplateAsUnstructured(req, "MachinePool", "spec.template.spec.bootstrap.configRef", topologyName)
 		if err != nil {
 			return err

internal/controllers/topology/cluster/patches/inline/json_patch_generator.go

@@ -186,7 +186,7 @@ func matchesSelector(req *runtimehooksv1.GeneratePatchesRequestItem, templateVar
 		}
 	}
 
-	// Check if the request is for a BootstrapConfigTemplate or an InfrastructureMachineTemplate
+	// Check if the request is for a BootstrapConfigTemplate or an InfrastructureMachinePoolTemplate
 	// of one of the configured MachinePoolClasses.
 	if selector.MatchResources.MachinePoolClass != nil {
 		if req.HolderReference.Kind == "MachinePool" &&

internal/controllers/topology/cluster/reconcile_state.go

@@ -939,8 +939,8 @@ type machineDiff struct {
 	toCreate, toUpdate, toDelete []string
 }
 
-// calculateMachineDeploymentDiff compares two maps of MachinePoolState and calculates which
-// MachinePools should be created, updated or deleted.
+// calculateMachineDeploymentDiff compares two maps of MachineDeploymentState and calculates which
+// MachineDeployments should be created, updated or deleted.
 func calculateMachineDeploymentDiff(current, desired map[string]*scope.MachineDeploymentState) machineDiff {
 	var diff machineDiff
 

internal/controllers/topology/cluster/scope/blueprint.go

@@ -84,8 +84,8 @@ type MachinePoolBlueprint struct {
 	// NOTE: This is a convenience copy of the metadata field from Cluster.Spec.Topology.Workers.MachinePools[x].
 	Metadata clusterv1.ObjectMeta
 
-	// BootstrapConfig holds the bootstrap template for a MachinePool referenced from ClusterClass.
-	BootstrapConfig *unstructured.Unstructured
+	// BootstrapTemplate holds the bootstrap template for a MachinePool referenced from ClusterClass.
+	BootstrapTemplate *unstructured.Unstructured
 
 	// InfrastructureMachinePoolTemplate holds the infrastructure machine template for a MachinePool referenced from ClusterClass.
 	InfrastructureMachinePoolTemplate *unstructured.Unstructured

internal/controllers/topology/cluster/scope/state.go

@@ -169,28 +169,28 @@ func (mp *MachinePoolState) IsUpgrading(ctx context.Context, c client.Client) (b
 	if mp.Object.Spec.Template.Spec.Version == nil {
 		return false, nil
 	}
-	infraMachineSelector := metav1.LabelSelector{
+	machineSelector := metav1.LabelSelector{
 		MatchLabels: map[string]string{
 			clusterv1.MachinePoolNameLabel: format.MustFormatValue(mp.Object.Name),
 			clusterv1.ClusterNameLabel:     mp.Object.Spec.ClusterName,
 		},
 	}
-	selectorMap, err := metav1.LabelSelectorAsMap(&infraMachineSelector)
+	selectorMap, err := metav1.LabelSelectorAsMap(&machineSelector)
 	if err != nil {
 		return false, errors.Wrapf(err, "failed to check if MachinePool %s is upgrading: failed to convert label selector to map", mp.Object.Name)
 	}
-	machinePools := &expv1.MachinePoolList{}
-	if err := c.List(ctx, machinePools, client.InNamespace(mp.Object.Namespace), client.MatchingLabels(selectorMap)); err != nil {
+	machines := &clusterv1.MachineList{}
+	if err := c.List(ctx, machines, client.InNamespace(mp.Object.Namespace), client.MatchingLabels(selectorMap)); err != nil {
 		return false, errors.Wrapf(err, "failed to check if MachinePool %s is upgrading: failed to list MachinePools", mp.Object.Name)
 	}
 	mpVersion := *mp.Object.Spec.Template.Spec.Version
 	// Check if the versions of the all the MachinePoolMachines match the MachinePool version.
-	for i := range machinePools.Items {
-		machinePool := machinePools.Items[i]
-		if machinePool.Spec.Template.Spec.Version == nil {
-			return false, fmt.Errorf("failed to check if MachinePool %s is upgrading: MachinePool %s has no version", mp.Object.Name, machinePool.Name)
+	for i := range machines.Items {
+		machine := machines.Items[i]
+		if machine.Spec.Version == nil {
+			return false, fmt.Errorf("failed to check if MachinePool %s is upgrading: Machine %s has no version", mp.Object.Name, machine.Name)
 		}
-		if *machinePool.Spec.Template.Spec.Version != mpVersion {
+		if *machine.Spec.Version != mpVersion {
 			return true, nil
 		}
 	}

internal/controllers/topology/cluster/util.go

@@ -37,6 +37,11 @@ func infrastructureMachineTemplateNamePrefix(clusterName, machineDeploymentTopol
 	return fmt.Sprintf("%s-%s-infra-", clusterName, machineDeploymentTopologyName)
 }
 
+// infrastructureMachinePoolNamePrefix calculates the name prefix for a InfrastructureMachinePool.
+func infrastructureMachinePoolNamePrefix(clusterName, machinePoolTopologyName string) string {
+	return fmt.Sprintf("%s-%s-infra-", clusterName, machinePoolTopologyName)
+}
+
 // infrastructureMachineTemplateNamePrefix calculates the name prefix for a InfrastructureMachineTemplate.
 func controlPlaneInfrastructureMachineTemplateNamePrefix(clusterName string) string {
 	return fmt.Sprintf("%s-control-plane-", clusterName)

internal/webhooks/cluster.go

@@ -663,6 +663,11 @@ func validateTopologyMetadata(topology *clusterv1.Topology, fldPath *field.Path)
 				fldPath.Child("workers", "machineDeployments").Index(idx).Child("metadata"),
 			)...)
 		}
+		for idx, mp := range topology.Workers.MachinePools {
+			allErrs = append(allErrs, mp.Metadata.Validate(
+				fldPath.Child("workers", "machinePools").Index(idx).Child("metadata"),
+			)...)
+		}
 	}
 	return allErrs
 }

internal/webhooks/clusterclass.go

@@ -269,7 +269,7 @@ func validateUpdatesToMachineHealthCheckClasses(clusters []clusterv1.Cluster, ol
 func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(clusters []clusterv1.Cluster, oldClusterClass, newClusterClass *clusterv1.ClusterClass) field.ErrorList {
 	var allErrs field.ErrorList
 
-	removedClasses := webhook.removedMachineClasses(oldClusterClass, newClusterClass)
+	removedClasses := webhook.removedMachineDeploymentClasses(oldClusterClass, newClusterClass)
 	// If no classes have been removed return early as no further checks are needed.
 	if len(removedClasses) == 0 {
 		return nil
@@ -313,7 +313,7 @@ func (webhook *ClusterClass) validateRemovedMachinePoolClassesAreNotUsed(cluster
 	return allErrs
 }
 
-func (webhook *ClusterClass) removedMachineClasses(oldClusterClass, newClusterClass *clusterv1.ClusterClass) sets.Set[string] {
+func (webhook *ClusterClass) removedMachineDeploymentClasses(oldClusterClass, newClusterClass *clusterv1.ClusterClass) sets.Set[string] {
 	removedClasses := sets.Set[string]{}
 
 	mdClasses := webhook.classNamesFromMDWorkerClass(newClusterClass.Spec.Workers)

test/extension/handlers/topologymutation/handler.go

@@ -382,7 +382,7 @@ func patchDockerMachinePoolTemplate(ctx context.Context, dockerMachinePoolTempla
 		kindMapping := kind.GetMapping(semVer, "")
 
 		log.Info(fmt.Sprintf("Setting MachinePool custom image to %q", kindMapping.Image))
-		dockerMachinePoolTemplate.Spec.Template.Spec.CustomImage = kindMapping.Image
+		dockerMachinePoolTemplate.Spec.Template.Spec.Template.CustomImage = kindMapping.Image
 		// return early if we have successfully patched a control plane dockerMachineTemplate
 		return nil
 	}

test/infrastructure/docker/templates/cluster-template-mp-development.yaml

@@ -34,27 +34,3 @@ spec:
       - class: default-worker
         name: mp-0
         replicas: 1
-# ---
-# apiVersion: cluster.x-k8s.io/v1beta1
-# kind: MachinePool
-# metadata:
-#   name: worker-mp-0
-#   namespace: default
-# spec:
-#   clusterName: "${CLUSTER_NAME}"
-#   replicas: 2
-#   template:
-#     spec:
-#       version: v1.27.1
-#       clusterName: "${CLUSTER_NAME}"
-#       bootstrap:
-#         configRef:
-#           apiVersion: bootstrap.cluster.x-k8s.io/v1beta1
-#           kind: KubeadmConfig
-#           name: worker-mp-0-config
-#           namespace: default
-#       infrastructureRef:
-#         apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
-#         kind: DockerMachinePool
-#         name: worker-dmp-0
-#         namespace: default