From f4153d37e23c0927cb77c293d0c8a879fedf0c53 Mon Sep 17 00:00:00 2001 From: Michael Weibel Date: Tue, 2 Jul 2024 11:40:52 +0200 Subject: [PATCH] fix: ampm remove finalizer and avoid recreation if VMSS is deleting --- azure/scope/machinepool.go | 4 + azure/scope/machinepool_test.go | 20 +++ .../azuremachinepoolmachine_controller.go | 9 +- ...azuremachinepoolmachine_controller_test.go | 146 ++++++++++++++++++ 4 files changed, 177 insertions(+), 2 deletions(-) diff --git a/azure/scope/machinepool.go b/azure/scope/machinepool.go index eef85381c5d..b6222419da2 100644 --- a/azure/scope/machinepool.go +++ b/azure/scope/machinepool.go @@ -412,6 +412,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 index 0f410516f8d..0c3602c6b3c 100644 --- 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 index cb8ba6aeafb..4d4ced208f4 100644 --- a/exp/controllers/azuremachinepoolmachine_controller.go +++ b/exp/controllers/azuremachinepoolmachine_controller.go @@ -214,9 +214,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 index 48e9580f62d..4b604261df1 100644 --- 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} +}