Fix tests

kubernetes-sigs · Jul 25, 2023 · 2439fb9 · 2439fb9
1 parent 42635c3
commit 2439fb9
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 3 deletions.
diff --git a/exp/api/v1alpha3/conversion.go b/exp/api/v1alpha3/conversion.go
@@ -76,6 +76,7 @@ func (src *MachinePool) ConvertTo(dstRaw conversion.Hub) error {
 	}
 	dst.Spec.Template.Spec.NodeDeletionTimeout = restored.Spec.Template.Spec.NodeDeletionTimeout
 	dst.Spec.Template.Spec.NodeVolumeDetachTimeout = restored.Spec.Template.Spec.NodeVolumeDetachTimeout
+	dst.Spec.Selector = restored.Spec.Selector
 	return nil
 }
 

diff --git a/exp/api/v1alpha4/conversion.go b/exp/api/v1alpha4/conversion.go
@@ -42,6 +42,7 @@ func (src *MachinePool) ConvertTo(dstRaw conversion.Hub) error {
 	}
 	dst.Spec.Template.Spec.NodeDeletionTimeout = restored.Spec.Template.Spec.NodeDeletionTimeout
 	dst.Spec.Template.Spec.NodeVolumeDetachTimeout = restored.Spec.Template.Spec.NodeVolumeDetachTimeout
+	dst.Spec.Selector = restored.Spec.Selector
 	return nil
 }
 

diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go
@@ -734,11 +734,25 @@ func (r *Reconciler) reconcileMachinePools(ctx context.Context, s *scope.Scope)
 	diff := calculateMachinePoolDiff(s.Current.MachinePools, s.Desired.MachinePools)
 
 	// Create MachinePools.
-	for _, mpTopologyName := range diff.toCreate {
-		mp := s.Desired.MachinePools[mpTopologyName]
-		if err := r.createMachinePool(ctx, s.Current.Cluster, mp); err != nil {
+	if len(diff.toCreate) > 0 {
+		currentMPTopologyNames, err := r.getCurrentMachinePools(ctx, s)
+		if err != nil {
 			return err
 		}
+		for _, mpTopologyName := range diff.toCreate {
+			mp := s.Desired.MachinePools[mpTopologyName]
+
+			// Skip the MP creation if the MP already exists.
+			if currentMPTopologyNames.Has(mpTopologyName) {
+				log := tlog.LoggerFrom(ctx).WithMachinePool(mp.Object)
+				log.V(3).Infof(fmt.Sprintf("Skipping creation of MachinePool %s because MachinePool for topology %s already exists (only considered creation because of stale cache)", tlog.KObj{Obj: mp.Object}, mpTopologyName))
+				continue
+			}
+
+			if err := r.createMachinePool(ctx, s.Current.Cluster, mp); err != nil {
+				return err
+			}
+		}
 	}
 
 	// Update MachinePools.
@@ -761,6 +775,32 @@ func (r *Reconciler) reconcileMachinePools(ctx context.Context, s *scope.Scope)
 	return nil
 }
 
+// getCurrentMachineDeployments gets the current list of MachinePools via the APIReader.
+func (r *Reconciler) getCurrentMachinePools(ctx context.Context, s *scope.Scope) (sets.Set[string], error) {
+	// TODO: We should consider using PartialObjectMetadataList here. Currently this doesn't work as our
+	// implementation for topology dryrun doesn't support PartialObjectMetadataList.
+	mpList := &expv1.MachinePoolList{}
+	err := r.APIReader.List(ctx, mpList,
+		client.MatchingLabels{
+			clusterv1.ClusterNameLabel:          s.Current.Cluster.Name,
+			clusterv1.ClusterTopologyOwnedLabel: "",
+		},
+		client.InNamespace(s.Current.Cluster.Namespace),
+	)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to read MachinePools for managed topology")
+	}
+
+	currentMPs := sets.Set[string]{}
+	for _, mp := range mpList.Items {
+		mpTopologyName, ok := mp.ObjectMeta.Labels[clusterv1.ClusterTopologyMachinePoolNameLabel]
+		if ok || mpTopologyName != "" {
+			currentMPs.Insert(mpTopologyName)
+		}
+	}
+	return currentMPs, nil
+}
+
 // createMachinePool creates a MachinePool and the corresponding templates.
 func (r *Reconciler) createMachinePool(ctx context.Context, cluster *clusterv1.Cluster, mp *scope.MachinePoolState) error {
 	log := tlog.LoggerFrom(ctx).WithMachinePool(mp.Object)

diff --git a/internal/webhooks/patch_validation.go b/internal/webhooks/patch_validation.go
@@ -207,6 +207,7 @@ func validateSelectors(selector clusterv1.PatchSelector, class *clusterv1.Cluste
 
 	if selector.MatchResources.MachineDeploymentClass != nil && len(selector.MatchResources.MachineDeploymentClass.Names) > 0 {
 		for i, name := range selector.MatchResources.MachineDeploymentClass.Names {
+			match := false
 			if strings.Contains(name, "*") {
 				// selector can at most have a single * rune
 				if strings.Count(name, "*") > 1 {
@@ -248,10 +249,18 @@ func validateSelectors(selector clusterv1.PatchSelector, class *clusterv1.Cluste
 				if matches {
 					if selectorMatchTemplate(selector, md.Template.Infrastructure.Ref) ||
 						selectorMatchTemplate(selector, md.Template.Bootstrap.Ref) {
+						match = true
 						break
 					}
 				}
 			}
+			if !match {
+				allErrs = append(allErrs, field.Invalid(
+					path.Child("matchResources", "machineDeploymentClass", "names").Index(i),
+					name,
+					"selector is enabled but matches neither the bootstrap ref nor the infrastructure ref of a MachineDeployment class",
+				))
+			}
 		}
 	}