Merge pull request #4959 from helio/fix-ampm-delete-recreate

AzureMachinePoolMachine remove finalizer and avoid recreation if VMSS is deleting
kubernetes-sigs · Jul 30, 2024 · 57ea5b8 · 57ea5b8
2 parents 4150054 + f4153d3
commit 57ea5b8
Show file tree

Hide file tree

Showing 4 changed files with 177 additions and 2 deletions.
diff --git a/azure/scope/machinepool.go b/azure/scope/machinepool.go
@@ -413,6 +413,10 @@ func (m *MachinePoolScope) applyAzureMachinePoolMachines(ctx context.Context) er
 	// determine which machines need to be created to reflect the current state in Azure
 	azureMachinesByProviderID := m.vmssState.InstancesByProviderID(m.AzureMachinePool.Spec.OrchestrationMode)
 	for key, val := range azureMachinesByProviderID {
+		if val.State == infrav1.Deleting || val.State == infrav1.Deleted {
+			log.V(4).Info("not recreating AzureMachinePoolMachine because VMSS VM is deleting", "providerID", key)
+			continue
+		}
 		if _, ok := existingMachinesByProviderID[key]; !ok {
 			log.V(4).Info("creating AzureMachinePoolMachine", "providerID", key)
 			if err := m.createMachine(ctx, val); err != nil {

diff --git a/azure/scope/machinepool_test.go b/azure/scope/machinepool_test.go
@@ -1449,6 +1449,26 @@ func TestMachinePoolScope_applyAzureMachinePoolMachines(t *testing.T) {
 				g.Expect(err).To(HaveOccurred())
 			},
 		},
+		{
+			Name: "if existing MachinePool is present but in deleting state, do not recreate AzureMachinePoolMachines",
+			Setup: func(mp *expv1.MachinePool, amp *infrav1exp.AzureMachinePool, vmssState *azure.VMSS, cb *fake.ClientBuilder) {
+				mp.Spec.Replicas = ptr.To[int32](1)
+
+				vmssState.Instances = []azure.VMSSVM{
+					{
+						ID:    "/subscriptions/123/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/vm",
+						Name:  "vm",
+						State: infrav1.Deleting,
+					},
+				}
+			},
+			Verify: func(g *WithT, amp *infrav1exp.AzureMachinePool, c client.Client, err error) {
+				g.Expect(err).NotTo(HaveOccurred())
+				list := infrav1exp.AzureMachinePoolMachineList{}
+				g.Expect(c.List(ctx, &list)).NotTo(HaveOccurred())
+				g.Expect(list.Items).Should(BeEmpty())
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.Name, func(t *testing.T) {

diff --git a/exp/controllers/azuremachinepoolmachine_controller.go b/exp/controllers/azuremachinepoolmachine_controller.go
@@ -207,9 +207,14 @@ func (ampmr *AzureMachinePoolMachineController) Reconcile(ctx context.Context, r
 		return reconcile.Result{}, err
 	}
 
-	if machine != nil {
+	switch {
+	case machine != nil:
 		logger = logger.WithValues("machine", machine.Name)
-	} else {
+	case !azureMachinePool.ObjectMeta.DeletionTimestamp.IsZero():
+		logger.Info("AzureMachinePool is being deleted, removing finalizer")
+		controllerutil.RemoveFinalizer(azureMachine, infrav1exp.AzureMachinePoolMachineFinalizer)
+		return reconcile.Result{}, ampmr.Client.Update(ctx, azureMachine)
+	default:
 		logger.Info("Waiting for Machine Controller to set OwnerRef on AzureMachinePoolMachine")
 		return reconcile.Result{}, nil
 	}

diff --git a/exp/controllers/azuremachinepoolmachine_controller_test.go b/exp/controllers/azuremachinepoolmachine_controller_test.go
@@ -89,6 +89,24 @@ func TestAzureMachinePoolMachineReconciler_Reconcile(t *testing.T) {
 				g.Expect(err.Error()).Should(ContainSubstring("not found"))
 			},
 		},
+		{
+			Name: "should remove finalizer if Machine is not found and AzureMachinePool has deletionTimestamp set",
+			Setup: func(cb *fake.ClientBuilder, reconciler *mock_azure.MockReconcilerMockRecorder) {
+				objects := getDeletingMachinePoolObjects()
+				cb.WithObjects(objects...)
+			},
+			Verify: func(g *WithT, c client.Client, result ctrl.Result, err error) {
+				g.Expect(err).NotTo(HaveOccurred())
+
+				ampm := &infrav1exp.AzureMachinePoolMachine{}
+				err = c.Get(context.Background(), types.NamespacedName{
+					Name:      "ampm1",
+					Namespace: "default",
+				}, ampm)
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).Should(ContainSubstring("not found"))
+			},
+		},
 	}
 
 	for _, c := range cases {
@@ -282,3 +300,131 @@ func getReadyMachinePoolMachineClusterObjects(ampmIsDeleting bool, ampmProvision
 
 	return []client.Object{cluster, azCluster, mp, amp, ma, ampm, fakeIdentity, fakeSecret}
 }
+
+func getDeletingMachinePoolObjects() []client.Object {
+	azCluster := &infrav1.AzureCluster{
+		TypeMeta: metav1.TypeMeta{
+			Kind: "AzureCluster",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "azCluster1",
+			Namespace: "default",
+		},
+		Spec: infrav1.AzureClusterSpec{
+			AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
+				SubscriptionID: "subID",
+				IdentityRef: &corev1.ObjectReference{
+					Name:      "fake-identity",
+					Namespace: "default",
+					Kind:      "AzureClusterIdentity",
+				},
+			},
+		},
+	}
+
+	cluster := &clusterv1.Cluster{
+		TypeMeta: metav1.TypeMeta{
+			Kind: "Cluster",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "cluster1",
+			Namespace: "default",
+		},
+		Spec: clusterv1.ClusterSpec{
+			InfrastructureRef: &corev1.ObjectReference{
+				Name:      azCluster.Name,
+				Namespace: "default",
+				Kind:      "AzureCluster",
+			},
+		},
+		Status: clusterv1.ClusterStatus{
+			InfrastructureReady: true,
+		},
+	}
+
+	mp := &expv1.MachinePool{
+		TypeMeta: metav1.TypeMeta{
+			Kind: "MachinePool",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "mp1",
+			Namespace:  "default",
+			Finalizers: []string{"test"},
+			DeletionTimestamp: &metav1.Time{
+				Time: time.Now(),
+			},
+			Labels: map[string]string{
+				"cluster.x-k8s.io/cluster-name": cluster.Name,
+			},
+		},
+	}
+
+	amp := &infrav1exp.AzureMachinePool{
+		TypeMeta: metav1.TypeMeta{
+			Kind: "AzureMachinePool",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "amp1",
+			Namespace:  "default",
+			Finalizers: []string{"test"},
+			DeletionTimestamp: &metav1.Time{
+				Time: time.Now(),
+			},
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					Name:       mp.Name,
+					Kind:       "MachinePool",
+					APIVersion: expv1.GroupVersion.String(),
+				},
+			},
+		},
+	}
+
+	ampm := &infrav1exp.AzureMachinePoolMachine{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "ampm1",
+			Namespace: "default",
+			DeletionTimestamp: &metav1.Time{
+				Time: time.Now(),
+			},
+			Finalizers: []string{infrav1exp.AzureMachinePoolMachineFinalizer},
+			Labels: map[string]string{
+				clusterv1.ClusterNameLabel: cluster.Name,
+			},
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					Name:       amp.Name,
+					Kind:       infrav1.AzureMachinePoolKind,
+					APIVersion: infrav1exp.GroupVersion.String(),
+				},
+			},
+		},
+	}
+
+	fakeIdentity := &infrav1.AzureClusterIdentity{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "fake-identity",
+			Namespace: "default",
+		},
+		Spec: infrav1.AzureClusterIdentitySpec{
+			Type: infrav1.ServicePrincipal,
+			ClientSecret: corev1.SecretReference{
+				Name:      "fooSecret",
+				Namespace: "default",
+			},
+			TenantID: "fake-tenantid",
+		},
+	}
+
+	fakeSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "fooSecret",
+			Namespace: "default",
+		},
+		Data: map[string][]byte{
+			"clientSecret": []byte("fooSecret"),
+		},
+	}
+
+	return []client.Object{cluster, azCluster, mp, amp, ampm, fakeIdentity, fakeSecret}
+}