From 10f016c2cd9878e6309c5a33fa7805c518c59da2 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Wed, 23 Nov 2022 17:31:47 +0700 Subject: [PATCH] fix review findings --- .../kubeadm/internal/controllers/controller.go | 4 ++-- .../machinedeployment_controller.go | 3 +-- .../machineset/machineset_controller.go | 18 ++++++++++++------ 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/controlplane/kubeadm/internal/controllers/controller.go index 12478be9fe4c..63c28e026f6b 100644 --- a/controlplane/kubeadm/internal/controllers/controller.go +++ b/controlplane/kubeadm/internal/controllers/controller.go @@ -331,10 +331,10 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, cluster * // Ensure all required labels exist on the controlled Machines. // This logic is needed to add the `cluster.x-k8s.io/control-plane-name` label to Machines - // which were created before the `cluster.x-k8s.io/control-plane-name` label was introduced. + // which were created before the `cluster.x-k8s.io/control-plane-name` label was introduced + // or if a user manually removed the label. // NOTE: Changes will be applied to the Machines in reconcileControlPlaneConditions. // NOTE: cluster.x-k8s.io/control-plane is already set at this stage (it is used when reading controlPlane.Machines). - // TODO(sbueringer): Drop the following code with v1.4 after all existing Machines are guaranteed to have the new label. for i := range controlPlane.Machines { machine := controlPlane.Machines[i] if value, ok := machine.Labels[clusterv1.MachineControlPlaneNameLabel]; !ok || value != kcp.Name { diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index 57b4707e9eef..51a3373ff878 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -231,8 +231,7 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, // Ensure all required labels exist on the controlled MachineSets. // This logic is needed to add the `cluster.x-k8s.io/deployment-name` label to MachineSets // which were created before the `cluster.x-k8s.io/deployment-name` label was added - // to all MachineSets created by a MachineDeployment. - // TODO(sbueringer): Drop the following code with v1.4 after all existing MachineSets are guaranteed to have the new label. + // to all MachineSets created by a MachineDeployment or if a user manually removed the label. for idx := range msList { machineSet := msList[idx] if name, ok := machineSet.Labels[clusterv1.MachineDeploymentLabelName]; ok && name == d.Name { diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index 26f8b6eba4d1..0f8b5a1bb652 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -293,10 +293,16 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, // Ensure all required labels exist on the controlled Machines. // This logic is needed to add the `cluster.x-k8s.io/set-name` label to Machines // which were created before the `cluster.x-k8s.io/set-name` label was added to - // all Machines created by a MachineSet. - // TODO(sbueringer): Drop the following code with v1.4 after all existing Machines are guaranteed to have the new label. + // all Machines created by a MachineSet or if a user manually removed the label. for _, machine := range filteredMachines { - if name, ok := machine.Labels[clusterv1.MachineSetLabelName]; ok && name == machineSet.Name { + mdNameOnMachineSet, mdNameSetOnMachineSet := machineSet.Labels[clusterv1.MachineDeploymentLabelName] + mdNameOnMachine := machine.Labels[clusterv1.MachineDeploymentLabelName] + + if msName, ok := machine.Labels[clusterv1.MachineSetLabelName]; ok && msName == machineSet.Name && + (!mdNameSetOnMachineSet || mdNameOnMachineSet == mdNameOnMachine) { + // Continue if the MachineSet name label is already set correctly and + // either the MachineDeployment name label is not set on the MachineSet or + // the MachineDeployment name label is set correctly on the Machine. continue } @@ -305,9 +311,9 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, return ctrl.Result{}, errors.Wrapf(err, "failed to apply %s label to Machine %q", clusterv1.MachineSetLabelName, machine.Name) } machine.Labels[clusterv1.MachineSetLabelName] = machineSet.Name - // Propagate the MachineDeploymentLabelName from MachineSet to Machine if it exists. - if mdName, ok := machineSet.Labels[clusterv1.MachineDeploymentLabelName]; ok { - machine.Labels[clusterv1.MachineDeploymentLabelName] = mdName + // Propagate the MachineDeploymentLabelName from MachineSet to Machine if it is set on the MachineSet. + if mdNameSetOnMachineSet { + machine.Labels[clusterv1.MachineDeploymentLabelName] = mdNameOnMachineSet } if err := helper.Patch(ctx, machine); err != nil { return ctrl.Result{}, errors.Wrapf(err, "failed to apply %s label to Machine %q", clusterv1.MachineSetLabelName, machine.Name)