From a5b08df37e3e268973ce4d9ca5604d2dbb03c4ae Mon Sep 17 00:00:00 2001 From: willie-yao Date: Tue, 15 Aug 2023 20:30:46 +0000 Subject: [PATCH] Second review round --- .../controllers/topology/cluster/blueprint.go | 2 +- .../topology/cluster/cluster_controller.go | 1 + .../topology/cluster/current_state.go | 4 +-- .../topology/cluster/desired_state.go | 27 ++++++++++--------- .../topology/cluster/patches/engine.go | 6 ++--- .../patches/inline/json_patch_generator.go | 2 +- .../topology/cluster/reconcile_state.go | 4 +-- .../topology/cluster/scope/blueprint.go | 4 +-- .../topology/cluster/scope/state.go | 18 ++++++------- internal/controllers/topology/cluster/util.go | 5 ++++ internal/webhooks/cluster.go | 5 ++++ internal/webhooks/clusterclass.go | 4 +-- .../handlers/topologymutation/handler.go | 2 +- ...r.x-k8s.io_dockermachinepooltemplates.yaml | 4 +-- .../cluster-template-mp-development.yaml | 24 ----------------- 15 files changed, 51 insertions(+), 61 deletions(-) diff --git a/internal/controllers/topology/cluster/blueprint.go b/internal/controllers/topology/cluster/blueprint.go index 7e74a9bdfe53..eb5cde78a4b8 100644 --- a/internal/controllers/topology/cluster/blueprint.go +++ b/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) } diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index 2bc5f8d892d3..ecf9045187cd 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -58,6 +58,7 @@ import ( // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;clusters/status,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusterclasses,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinedeployments,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinepools,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinehealthchecks,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;create;delete diff --git a/internal/controllers/topology/cluster/current_state.go b/internal/controllers/topology/cluster/current_state.go index e52ea3d8355b..ea0bcd88f67e 100644 --- a/internal/controllers/topology/cluster/current_state.go +++ b/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})) } diff --git a/internal/controllers/topology/cluster/desired_state.go b/internal/controllers/topology/cluster/desired_state.go index 62d3c43d86f1..a7c2d291e796 100644 --- a/internal/controllers/topology/cluster/desired_state.go +++ b/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,7 +925,7 @@ 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 { @@ -933,8 +933,8 @@ func computeMachinePool(_ context.Context, s *scope.Scope, machinePoolTopology c } 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 = ¤tMachinePool.Object.Spec.Template.Spec.InfrastructureRef + currentInfraMachinePoolRef = ¤tMachinePool.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 diff --git a/internal/controllers/topology/cluster/patches/engine.go b/internal/controllers/topology/cluster/patches/engine.go index a15649a13b96..5cb7938cf80c 100644 --- a/internal/controllers/topology/cluster/patches/engine.go +++ b/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 diff --git a/internal/controllers/topology/cluster/patches/inline/json_patch_generator.go b/internal/controllers/topology/cluster/patches/inline/json_patch_generator.go index c924265e1114..1c80acb634a7 100644 --- a/internal/controllers/topology/cluster/patches/inline/json_patch_generator.go +++ b/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" && diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index 0657cb7d471c..c12c075e581a 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/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 diff --git a/internal/controllers/topology/cluster/scope/blueprint.go b/internal/controllers/topology/cluster/scope/blueprint.go index 14d206828134..1f93ba84258e 100644 --- a/internal/controllers/topology/cluster/scope/blueprint.go +++ b/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 diff --git a/internal/controllers/topology/cluster/scope/state.go b/internal/controllers/topology/cluster/scope/state.go index b4fab190631f..4d92d38405b2 100644 --- a/internal/controllers/topology/cluster/scope/state.go +++ b/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 } } diff --git a/internal/controllers/topology/cluster/util.go b/internal/controllers/topology/cluster/util.go index 1a5f6ddd8fe5..600d1b536df2 100644 --- a/internal/controllers/topology/cluster/util.go +++ b/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) diff --git a/internal/webhooks/cluster.go b/internal/webhooks/cluster.go index 32f42152b9d8..f3c7daad7b0f 100644 --- a/internal/webhooks/cluster.go +++ b/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 } diff --git a/internal/webhooks/clusterclass.go b/internal/webhooks/clusterclass.go index 23ec44d81dd8..88a4e3d27dcb 100644 --- a/internal/webhooks/clusterclass.go +++ b/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) diff --git a/test/extension/handlers/topologymutation/handler.go b/test/extension/handlers/topologymutation/handler.go index 7913577d4da4..1fc836212525 100644 --- a/test/extension/handlers/topologymutation/handler.go +++ b/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 } diff --git a/test/infrastructure/docker/config/crd/bases/infrastructure.cluster.x-k8s.io_dockermachinepooltemplates.yaml b/test/infrastructure/docker/config/crd/bases/infrastructure.cluster.x-k8s.io_dockermachinepooltemplates.yaml index c02d9d68a075..81732291dd7a 100644 --- a/test/infrastructure/docker/config/crd/bases/infrastructure.cluster.x-k8s.io_dockermachinepooltemplates.yaml +++ b/test/infrastructure/docker/config/crd/bases/infrastructure.cluster.x-k8s.io_dockermachinepooltemplates.yaml @@ -69,8 +69,8 @@ spec: type: object type: object spec: - description: Spec is the specification of the desired behavior - of the machine. + description: DockerMachinePoolSpec defines the desired state of + DockerMachinePool. properties: providerID: description: ProviderID is the identification ID of the Machine diff --git a/test/infrastructure/docker/templates/cluster-template-mp-development.yaml b/test/infrastructure/docker/templates/cluster-template-mp-development.yaml index 9baa23691f80..f2cb7ef249a9 100644 --- a/test/infrastructure/docker/templates/cluster-template-mp-development.yaml +++ b/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