From 0908bfc666f686a1aedb5df93981f6c33d60b148 Mon Sep 17 00:00:00 2001 From: Vince Prignano Date: Wed, 24 Jul 2024 02:45:47 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20MachineSet=20should=20allow=20sc?= =?UTF-8?q?ale=20down=20operations=20to=20proceed=20when=20templates=20don?= =?UTF-8?q?'t=20exist=20(#10913)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Vince Prignano --- .../machineset/machineset_controller.go | 27 +++- .../machineset/machineset_controller_test.go | 119 +++++++++++++----- 2 files changed, 109 insertions(+), 37 deletions(-) diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index e5d802fcbb28..c4d618e9a0e1 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -240,12 +240,12 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, } // Make sure to reconcile the external infrastructure reference. - if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, &machineSet.Spec.Template.Spec.InfrastructureRef); err != nil { + if err := r.reconcileExternalTemplateReference(ctx, cluster, machineSet, &machineSet.Spec.Template.Spec.InfrastructureRef); err != nil { return ctrl.Result{}, err } // Make sure to reconcile the external bootstrap reference, if any. if machineSet.Spec.Template.Spec.Bootstrap.ConfigRef != nil { - if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, machineSet.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil { + if err := r.reconcileExternalTemplateReference(ctx, cluster, machineSet, machineSet.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil { return ctrl.Result{}, err } } @@ -1139,21 +1139,36 @@ func (r *Reconciler) reconcileUnhealthyMachines(ctx context.Context, cluster *cl return ctrl.Result{}, nil } -func reconcileExternalTemplateReference(ctx context.Context, c client.Client, cluster *clusterv1.Cluster, ref *corev1.ObjectReference) error { +func (r *Reconciler) reconcileExternalTemplateReference(ctx context.Context, cluster *clusterv1.Cluster, ms *clusterv1.MachineSet, ref *corev1.ObjectReference) error { if !strings.HasSuffix(ref.Kind, clusterv1.TemplateSuffix) { return nil } - if err := utilconversion.UpdateReferenceAPIContract(ctx, c, ref); err != nil { + if err := utilconversion.UpdateReferenceAPIContract(ctx, r.Client, ref); err != nil { return err } - obj, err := external.Get(ctx, c, ref, cluster.Namespace) + obj, err := external.Get(ctx, r.Client, ref, cluster.Namespace) if err != nil { + if apierrors.IsNotFound(err) { + if _, ok := ms.Labels[clusterv1.MachineDeploymentNameLabel]; !ok { + // If the MachineSet is not in a MachineDeployment, return the error immediately. + return err + } + // When the MachineSet is part of a MachineDeployment but isn't the current revision, we should + // ignore the not found references and allow the controller to proceed. + owner, err := r.getOwnerMachineDeployment(ctx, ms) + if err != nil { + return err + } + if owner.Annotations[clusterv1.RevisionAnnotation] != ms.Annotations[clusterv1.RevisionAnnotation] { + return nil + } + } return err } - patchHelper, err := patch.NewHelper(obj, c) + patchHelper, err := patch.NewHelper(obj, r.Client) if err != nil { return err } diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index a3e4e60aeae0..edb10c12cc42 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -84,12 +84,71 @@ func TestMachineSetReconciler(t *testing.T) { duration5m := &metav1.Duration{Duration: 5 * time.Minute} replicas := int32(2) version := "v1.14.2" + machineTemplateSpec := clusterv1.MachineTemplateSpec{ + ObjectMeta: clusterv1.ObjectMeta{ + Labels: map[string]string{ + "label-1": "true", + }, + Annotations: map[string]string{ + "annotation-1": "true", + "precedence": "MachineSet", + }, + }, + Spec: clusterv1.MachineSpec{ + ClusterName: testCluster.Name, + Version: &version, + Bootstrap: clusterv1.Bootstrap{ + ConfigRef: &corev1.ObjectReference{ + APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", + Kind: "GenericBootstrapConfigTemplate", + Name: "ms-template", + }, + }, + InfrastructureRef: corev1.ObjectReference{ + APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", + Kind: "GenericInfrastructureMachineTemplate", + Name: "ms-template", + }, + NodeDrainTimeout: duration10m, + NodeDeletionTimeout: duration10m, + NodeVolumeDetachTimeout: duration10m, + }, + } + + machineDeployment := &clusterv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "md-", + Namespace: namespace.Name, + Annotations: map[string]string{ + clusterv1.RevisionAnnotation: "10", + }, + }, + Spec: clusterv1.MachineDeploymentSpec{ + ClusterName: testCluster.Name, + Replicas: &replicas, + Template: machineTemplateSpec, + }, + } + g.Expect(env.Create(ctx, machineDeployment)).To(Succeed()) + instance := &clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "ms-", Namespace: namespace.Name, Labels: map[string]string{ - "label-1": "true", + "label-1": "true", + clusterv1.MachineDeploymentNameLabel: machineDeployment.Name, + }, + Annotations: map[string]string{ + clusterv1.RevisionAnnotation: "10", + }, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineDeployment", + Name: machineDeployment.Name, + UID: machineDeployment.UID, + }, }, }, Spec: clusterv1.MachineSetSpec{ @@ -100,36 +159,7 @@ func TestMachineSetReconciler(t *testing.T) { "label-1": "true", }, }, - Template: clusterv1.MachineTemplateSpec{ - ObjectMeta: clusterv1.ObjectMeta{ - Labels: map[string]string{ - "label-1": "true", - }, - Annotations: map[string]string{ - "annotation-1": "true", - "precedence": "MachineSet", - }, - }, - Spec: clusterv1.MachineSpec{ - ClusterName: testCluster.Name, - Version: &version, - Bootstrap: clusterv1.Bootstrap{ - ConfigRef: &corev1.ObjectReference{ - APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", - Kind: "GenericBootstrapConfigTemplate", - Name: "ms-template", - }, - }, - InfrastructureRef: corev1.ObjectReference{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", - Kind: "GenericInfrastructureMachineTemplate", - Name: "ms-template", - }, - NodeDrainTimeout: duration10m, - NodeDeletionTimeout: duration10m, - NodeVolumeDetachTimeout: duration10m, - }, - }, + Template: machineTemplateSpec, }, } @@ -405,6 +435,33 @@ func TestMachineSetReconciler(t *testing.T) { // Validate that the controller set the cluster name label in selector. g.Expect(instance.Status.Selector).To(ContainSubstring(testCluster.Name)) + + t.Log("Verifying MachineSet can be scaled down when templates don't exist, and MachineSet is not current") + g.Expect(env.CleanupAndWait(ctx, bootstrapTmpl)).To(Succeed()) + g.Expect(env.CleanupAndWait(ctx, infraTmpl)).To(Succeed()) + + t.Log("Updating Replicas on MachineSet") + patchHelper, err = patch.NewHelper(instance, env) + g.Expect(err).ToNot(HaveOccurred()) + instance.SetAnnotations(map[string]string{ + clusterv1.RevisionAnnotation: "9", + }) + instance.Spec.Replicas = ptr.To(int32(1)) + g.Expect(patchHelper.Patch(ctx, instance)).Should(Succeed()) + + // Verify that we have 1 replicas. + g.Eventually(func() (ready int) { + if err := env.List(ctx, machines, client.InNamespace(namespace.Name)); err != nil { + return -1 + } + for _, m := range machines.Items { + if !m.DeletionTimestamp.IsZero() { + continue + } + ready++ + } + return + }, timeout*3).Should(BeEquivalentTo(1)) }) }