diff --git a/exp/api/v1alpha3/conversion.go b/exp/api/v1alpha3/conversion.go index 8f6903b81abc..6978b3668890 100644 --- a/exp/api/v1alpha3/conversion.go +++ b/exp/api/v1alpha3/conversion.go @@ -44,10 +44,6 @@ func Convert_v1alpha3_MachinePool_To_v1beta1_MachinePool(in *MachinePool, out *e return nil } -func Convert_v1beta1_MachinePoolSpec_To_v1alpha3_MachinePoolSpec(in *expv1.MachinePoolSpec, out *MachinePoolSpec, s apimachineryconversion.Scope) error { - return autoConvert_v1beta1_MachinePoolSpec_To_v1alpha3_MachinePoolSpec(in, out, s) -} - func Convert_v1beta1_MachinePool_To_v1alpha3_MachinePool(in *expv1.MachinePool, out *MachinePool, s apimachineryconversion.Scope) error { if err := autoConvert_v1beta1_MachinePool_To_v1alpha3_MachinePool(in, out, s); err != nil { return err diff --git a/exp/api/v1alpha3/zz_generated.conversion.go b/exp/api/v1alpha3/zz_generated.conversion.go index 19ae36da3b85..d152713ecaf9 100644 --- a/exp/api/v1alpha3/zz_generated.conversion.go +++ b/exp/api/v1alpha3/zz_generated.conversion.go @@ -50,6 +50,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddGeneratedConversionFunc((*v1beta1.MachinePoolSpec)(nil), (*MachinePoolSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_MachinePoolSpec_To_v1alpha3_MachinePoolSpec(a.(*v1beta1.MachinePoolSpec), b.(*MachinePoolSpec), scope) + }); err != nil { + return err + } if err := s.AddGeneratedConversionFunc((*MachinePoolStatus)(nil), (*v1beta1.MachinePoolStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha3_MachinePoolStatus_To_v1beta1_MachinePoolStatus(a.(*MachinePoolStatus), b.(*v1beta1.MachinePoolStatus), scope) }); err != nil { @@ -70,11 +75,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddConversionFunc((*v1beta1.MachinePoolSpec)(nil), (*MachinePoolSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_MachinePoolSpec_To_v1alpha3_MachinePoolSpec(a.(*v1beta1.MachinePoolSpec), b.(*MachinePoolSpec), scope) - }); err != nil { - return err - } if err := s.AddConversionFunc((*v1beta1.MachinePool)(nil), (*MachinePool)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_MachinePool_To_v1alpha3_MachinePool(a.(*v1beta1.MachinePool), b.(*MachinePool), scope) }); err != nil { @@ -172,6 +172,11 @@ func autoConvert_v1beta1_MachinePoolSpec_To_v1alpha3_MachinePoolSpec(in *v1beta1 return nil } +// Convert_v1beta1_MachinePoolSpec_To_v1alpha3_MachinePoolSpec is an autogenerated conversion function. +func Convert_v1beta1_MachinePoolSpec_To_v1alpha3_MachinePoolSpec(in *v1beta1.MachinePoolSpec, out *MachinePoolSpec, s conversion.Scope) error { + return autoConvert_v1beta1_MachinePoolSpec_To_v1alpha3_MachinePoolSpec(in, out, s) +} + func autoConvert_v1alpha3_MachinePoolStatus_To_v1beta1_MachinePoolStatus(in *MachinePoolStatus, out *v1beta1.MachinePoolStatus, s conversion.Scope) error { out.NodeRefs = *(*[]v1.ObjectReference)(unsafe.Pointer(&in.NodeRefs)) out.Replicas = in.Replicas diff --git a/exp/api/v1alpha4/conversion.go b/exp/api/v1alpha4/conversion.go index fa9d49368d8d..cb266a915b0c 100644 --- a/exp/api/v1alpha4/conversion.go +++ b/exp/api/v1alpha4/conversion.go @@ -17,17 +17,12 @@ limitations under the License. package v1alpha4 import ( - apimachineryconversion "k8s.io/apimachinery/pkg/conversion" "sigs.k8s.io/controller-runtime/pkg/conversion" expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" utilconversion "sigs.k8s.io/cluster-api/util/conversion" ) -func Convert_v1beta1_MachinePoolSpec_To_v1alpha4_MachinePoolSpec(in *expv1.MachinePoolSpec, out *MachinePoolSpec, s apimachineryconversion.Scope) error { - return autoConvert_v1beta1_MachinePoolSpec_To_v1alpha4_MachinePoolSpec(in, out, s) -} - func (src *MachinePool) ConvertTo(dstRaw conversion.Hub) error { dst := dstRaw.(*expv1.MachinePool) diff --git a/exp/api/v1alpha4/zz_generated.conversion.go b/exp/api/v1alpha4/zz_generated.conversion.go index 1c2ef9cc44f4..3609aa05f248 100644 --- a/exp/api/v1alpha4/zz_generated.conversion.go +++ b/exp/api/v1alpha4/zz_generated.conversion.go @@ -65,6 +65,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddGeneratedConversionFunc((*v1beta1.MachinePoolSpec)(nil), (*MachinePoolSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_MachinePoolSpec_To_v1alpha4_MachinePoolSpec(a.(*v1beta1.MachinePoolSpec), b.(*MachinePoolSpec), scope) + }); err != nil { + return err + } if err := s.AddGeneratedConversionFunc((*MachinePoolStatus)(nil), (*v1beta1.MachinePoolStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha4_MachinePoolStatus_To_v1beta1_MachinePoolStatus(a.(*MachinePoolStatus), b.(*v1beta1.MachinePoolStatus), scope) }); err != nil { @@ -75,11 +80,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddConversionFunc((*v1beta1.MachinePoolSpec)(nil), (*MachinePoolSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_MachinePoolSpec_To_v1alpha4_MachinePoolSpec(a.(*v1beta1.MachinePoolSpec), b.(*MachinePoolSpec), scope) - }); err != nil { - return err - } return nil } @@ -186,6 +186,11 @@ func autoConvert_v1beta1_MachinePoolSpec_To_v1alpha4_MachinePoolSpec(in *v1beta1 return nil } +// Convert_v1beta1_MachinePoolSpec_To_v1alpha4_MachinePoolSpec is an autogenerated conversion function. +func Convert_v1beta1_MachinePoolSpec_To_v1alpha4_MachinePoolSpec(in *v1beta1.MachinePoolSpec, out *MachinePoolSpec, s conversion.Scope) error { + return autoConvert_v1beta1_MachinePoolSpec_To_v1alpha4_MachinePoolSpec(in, out, s) +} + func autoConvert_v1alpha4_MachinePoolStatus_To_v1beta1_MachinePoolStatus(in *MachinePoolStatus, out *v1beta1.MachinePoolStatus, s conversion.Scope) error { out.NodeRefs = *(*[]v1.ObjectReference)(unsafe.Pointer(&in.NodeRefs)) out.Replicas = in.Replicas diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index 13865e89d1e8..62d3c43d86f1 100644 --- a/internal/controllers/topology/cluster/desired_state.go +++ b/internal/controllers/topology/cluster/desired_state.go @@ -75,6 +75,13 @@ func (r *Reconciler) computeDesiredState(ctx context.Context, s *scope.Scope) (* } s.UpgradeTracker.MachineDeployments.MarkUpgrading(mdUpgradingNames...) + // Mark all the MachinePools that are currently upgrading. + mpUpgradingNames, err := s.Current.MachinePools.Upgrading(ctx, r.Client) + if err != nil { + return nil, errors.Wrap(err, "failed to check if any MachinePool is upgrading") + } + s.UpgradeTracker.MachinePools.MarkUpgrading(mpUpgradingNames...) + // Compute the desired state of the ControlPlane object, eventually adding a reference to the // InfrastructureMachineTemplate generated by the previous step. if desiredState.ControlPlane.Object, err = r.computeControlPlane(ctx, s, desiredState.ControlPlane.InfrastructureMachineTemplate); err != nil { @@ -920,9 +927,9 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c // Compute the bootstrap template. currentMachinePool := s.Current.MachinePools[machinePoolTopology.Name] - var currentBootstrapTemplateRef *corev1.ObjectReference + var currentBootstrapConfigRef *corev1.ObjectReference if currentMachinePool != nil && currentMachinePool.BootstrapObject != nil { - currentBootstrapTemplateRef = currentMachinePool.Object.Spec.Template.Spec.Bootstrap.ConfigRef + currentBootstrapConfigRef = currentMachinePool.Object.Spec.Template.Spec.Bootstrap.ConfigRef } var err error desiredMachinePool.BootstrapObject, err = templateToObject(templateToInput{ @@ -930,7 +937,7 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c templateClonedFromRef: contract.ObjToRef(machinePoolBlueprint.BootstrapConfig), cluster: s.Current.Cluster, namePrefix: bootstrapTemplateNamePrefix(s.Current.Cluster.Name, machinePoolTopology.Name), - currentObjectRef: currentBootstrapTemplateRef, + currentObjectRef: currentBootstrapConfigRef, }) if err != nil { return nil, errors.Wrapf(err, "failed to compute bootstrap object for topology %q", machinePoolTopology.Name) @@ -991,9 +998,9 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c } // Compute the MachinePool object. - desiredBootstrapTemplateRef, err := calculateRefDesiredAPIVersion(currentBootstrapTemplateRef, desiredMachinePool.BootstrapObject) + desiredBootstrapConfigRef, err := calculateRefDesiredAPIVersion(currentBootstrapConfigRef, desiredMachinePool.BootstrapObject) if err != nil { - return nil, errors.Wrap(err, "failed to calculate desired bootstrap template ref") + return nil, errors.Wrap(err, "failed to calculate desired bootstrap config ref") } desiredInfraMachineTemplateRef, err := calculateRefDesiredAPIVersion(currentInfraMachineTemplateRef, desiredMachinePool.InfrastructureMachinePoolObject) if err != nil { @@ -1016,7 +1023,7 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c Spec: clusterv1.MachineSpec{ ClusterName: s.Current.Cluster.Name, Version: pointer.String(version), - Bootstrap: clusterv1.Bootstrap{ConfigRef: desiredBootstrapTemplateRef}, + Bootstrap: clusterv1.Bootstrap{ConfigRef: desiredBootstrapConfigRef}, InfrastructureRef: *desiredInfraMachineTemplateRef, NodeDrainTimeout: nodeDrainTimeout, NodeVolumeDetachTimeout: nodeVolumeDetachTimeout, diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index 596ae6a8c023..0657cb7d471c 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -257,7 +257,11 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope len(s.UpgradeTracker.MachineDeployments.UpgradingNames()) == 0 && // Machine deployments are not upgrading or not about to upgrade !s.UpgradeTracker.MachineDeployments.IsAnyPendingCreate() && // No MachineDeployments are pending create !s.UpgradeTracker.MachineDeployments.IsAnyPendingUpgrade() && // No MachineDeployments are pending an upgrade - !s.UpgradeTracker.MachineDeployments.DeferredUpgrade() { // No MachineDeployments have deferred an upgrade + !s.UpgradeTracker.MachineDeployments.DeferredUpgrade() && // No MachineDeployments have deferred an upgrade + len(s.UpgradeTracker.MachinePools.UpgradingNames()) == 0 && // Machine pools are not upgrading or not about to upgrade + !s.UpgradeTracker.MachinePools.IsAnyPendingCreate() && // No MachinePools are pending create + !s.UpgradeTracker.MachinePools.IsAnyPendingUpgrade() && // No MachinePools are pending an upgrade + !s.UpgradeTracker.MachinePools.DeferredUpgrade() { // No MachinePools have deferred an upgrade // Everything is stable and the cluster can be considered fully upgraded. hookRequest := &runtimehooksv1.AfterClusterUpgradeRequest{ Cluster: *s.Current.Cluster, @@ -747,7 +751,7 @@ func (r *Reconciler) reconcileMachinePools(ctx context.Context, s *scope.Scope) continue } - if err := r.createMachinePool(ctx, s.Current.Cluster, mp); err != nil { + if err := r.createMachinePool(ctx, s, mp); err != nil { return err } } @@ -800,9 +804,23 @@ func (r *Reconciler) getCurrentMachinePools(ctx context.Context, s *scope.Scope) } // 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) +func (r *Reconciler) createMachinePool(ctx context.Context, s *scope.Scope, mp *scope.MachinePoolState) error { + // Do not create the MachinePool if it is marked as pending create. + // This will also block MHC creation because creating the MHC without the corresponding + // MachinePool is unnecessary. + mpTopologyName, ok := mp.Object.Labels[clusterv1.ClusterTopologyMachinePoolNameLabel] + if !ok || mpTopologyName == "" { + // Note: This is only an additional safety check and should not happen. The label will always be added when computing + // the desired MachinePool. + return errors.Errorf("new MachinePool is missing the %q label", clusterv1.ClusterTopologyMachinePoolNameLabel) + } + // Return early if the MachinePool is pending create. + if s.UpgradeTracker.MachinePools.IsPendingCreate(mpTopologyName) { + return nil + } + log := tlog.LoggerFrom(ctx).WithMachinePool(mp.Object) + cluster := s.Current.Cluster infraCtx, _ := log.WithObject(mp.InfrastructureMachinePoolObject).Into(ctx) if err := r.reconcileReferencedObject(infraCtx, reconcileReferencedObjectInput{ cluster: cluster, @@ -830,6 +848,23 @@ func (r *Reconciler) createMachinePool(ctx context.Context, cluster *clusterv1.C } r.recorder.Eventf(cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: mp.Object}) + // Wait until MachinePool is visible in the cache. + // Note: We have to do this because otherwise using a cached client in current state could + // miss a newly created MachinePool (because the cache might be stale). + err = wait.PollUntilContextTimeout(ctx, 5*time.Millisecond, 5*time.Second, true, func(ctx context.Context) (bool, error) { + key := client.ObjectKey{Namespace: mp.Object.Namespace, Name: mp.Object.Name} + if err := r.Client.Get(ctx, key, &expv1.MachinePool{}); err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, err + } + return true, nil + }) + if err != nil { + return errors.Wrapf(err, "failed waiting for MachinePool %s to be visible in the cache after create", mp.Object.Kind) + } + return nil } @@ -857,7 +892,7 @@ func (r *Reconciler) updateMachinePool(ctx context.Context, cluster *clusterv1.C return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: currentMP.Object}) } - // Check differences between current and desired MachineDeployment, and eventually patch the current object. + // Check differences between current and desired MachinePool, and eventually patch the current object. log = log.WithObject(desiredMP.Object) patchHelper, err := r.patchHelperFactory(ctx, currentMP.Object, desiredMP.Object) if err != nil { @@ -904,8 +939,8 @@ type machineDiff struct { toCreate, toUpdate, toDelete []string } -// calculateMachineDeploymentDiff compares two maps of MachineDeploymentState and calculates which -// MachineDeployments should be created, updated or deleted. +// calculateMachineDeploymentDiff compares two maps of MachinePoolState and calculates which +// MachinePools should be created, updated or deleted. func calculateMachineDeploymentDiff(current, desired map[string]*scope.MachineDeploymentState) machineDiff { var diff machineDiff diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 4466fda390ae..23ec44d81dd8 100644 --- a/internal/webhooks/clusterclass.go +++ b/internal/webhooks/clusterclass.go @@ -293,7 +293,7 @@ func (webhook *ClusterClass) validateRemovedMachineDeploymentClassesAreNotUsed(c func (webhook *ClusterClass) validateRemovedMachinePoolClassesAreNotUsed(clusters []clusterv1.Cluster, oldClusterClass, newClusterClass *clusterv1.ClusterClass) field.ErrorList { var allErrs field.ErrorList - removedClasses := webhook.removedMachineClasses(oldClusterClass, newClusterClass) + removedClasses := webhook.removedMachinePoolClasses(oldClusterClass, newClusterClass) // If no classes have been removed return early as no further checks are needed. if len(removedClasses) == 0 { return nil @@ -322,6 +322,11 @@ func (webhook *ClusterClass) removedMachineClasses(oldClusterClass, newClusterCl removedClasses.Insert(oldClass.Class) } } + return removedClasses +} + +func (webhook *ClusterClass) removedMachinePoolClasses(oldClusterClass, newClusterClass *clusterv1.ClusterClass) sets.Set[string] { + removedClasses := sets.Set[string]{} mpClasses := webhook.classNamesFromMPWorkerClass(newClusterClass.Spec.Workers) for _, oldClass := range oldClusterClass.Spec.Workers.MachinePools { diff --git a/test/extension/handlers/topologymutation/handler.go b/test/extension/handlers/topologymutation/handler.go index 52aebead0bf4..7913577d4da4 100644 --- a/test/extension/handlers/topologymutation/handler.go +++ b/test/extension/handlers/topologymutation/handler.go @@ -359,13 +359,34 @@ func patchDockerMachineTemplate(ctx context.Context, dockerMachineTemplate *infr return errors.New("no version variables found for DockerMachineTemplate patch") } -// patchDockerMachineTemplate patches the DockerMachinePoolTemplate. +// patchDockerMachinePoolTemplate patches the DockerMachinePoolTemplate. // It sets the CustomImage to an image for the version in use by the MachinePool. // NOTE: this patch is not required anymore after the introduction of the kind mapper in kind, however we keep it // as example of version aware patches. func patchDockerMachinePoolTemplate(ctx context.Context, dockerMachinePoolTemplate *infraexpv1.DockerMachinePoolTemplate, templateVariables map[string]apiextensionsv1.JSON) error { log := ctrl.LoggerFrom(ctx) + // If the DockerMachinePoolTemplate belongs to the ControlPlane, set the images using the ControlPlane version. + // NOTE: ControlPlane version might be different than Cluster.version or MachinePool's versions; + // the builtin variables provides the right version to use. + // NOTE: This works by checking the existence of a builtin variable that exists only for templates liked to the ControlPlane. + cpVersion, found, err := topologymutation.GetStringVariable(templateVariables, "builtin.controlPlane.version") + if err != nil { + return errors.Wrap(err, "could not set customImage to control plane dockerMachinePoolTemplate") + } + if found { + semVer, err := version.ParseMajorMinorPatchTolerant(cpVersion) + if err != nil { + return errors.Wrap(err, "could not parse control plane version") + } + kindMapping := kind.GetMapping(semVer, "") + + log.Info(fmt.Sprintf("Setting MachinePool custom image to %q", kindMapping.Image)) + dockerMachinePoolTemplate.Spec.Template.Spec.CustomImage = kindMapping.Image + // return early if we have successfully patched a control plane dockerMachineTemplate + return nil + } + // If the DockerMachinePoolTemplate belongs to a MachinePool, set the images the MachinePool version. // NOTE: MachinePool version might be different than Cluster.version or other MachinePool's versions; // the builtin variables provides the right version to use.