From 2439fb9e2838b3670bc97a89db47b9050962c2aa Mon Sep 17 00:00:00 2001 From: willie-yao Date: Mon, 24 Jul 2023 23:33:05 +0000 Subject: [PATCH] Fix tests --- exp/api/v1alpha3/conversion.go | 1 + exp/api/v1alpha4/conversion.go | 1 + .../topology/cluster/reconcile_state.go | 46 +++++++++++++++++-- internal/webhooks/patch_validation.go | 9 ++++ 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/exp/api/v1alpha3/conversion.go b/exp/api/v1alpha3/conversion.go index 8f6903b81abc..b1eccab89345 100644 --- 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 index fa9d49368d8d..d66548e85829 100644 --- 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 index 52081f46b6db..9cfedb3ebd78 100644 --- 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 index 81557bc06b57..d08016c3f9c9 100644 --- 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", + )) + } } }