🐛 MachineSet should allow scale down operations to proceed when templates don't exist (#10913)

Signed-off-by: Vince Prignano <[email protected]>

Author: vincepri

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
 	}

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))
 	})
 }