diff --git a/azure/services/groups/groups_test.go b/azure/services/groups/groups_test.go index d5e1ffa322f..2413f5be50e 100644 --- a/azure/services/groups/groups_test.go +++ b/azure/services/groups/groups_test.go @@ -212,12 +212,13 @@ func TestDeleteGroups(t *testing.T) { } } -func TestShouldDeleteIndividualResources(t *testing.T) { +func TestIsManaged(t *testing.T) { tests := []struct { - name string - objects []client.Object - expect func(s *mock_groups.MockGroupScopeMockRecorder) - expected bool + name string + objects []client.Object + expect func(s *mock_groups.MockGroupScopeMockRecorder) + expected bool + expectedError bool }{ { name: "error checking if group is managed", @@ -226,7 +227,7 @@ func TestShouldDeleteIndividualResources(t *testing.T) { s.GroupSpec().Return(&GroupSpec{}).AnyTimes() s.ClusterName().Return("").AnyTimes() }, - expected: true, + expectedError: true, }, { name: "group is unmanaged", @@ -319,8 +320,12 @@ func TestShouldDeleteIndividualResources(t *testing.T) { scopeMock.EXPECT().GetClient().Return(ctrlClient).AnyTimes() test.expect(scopeMock.EXPECT()) - actual := New(scopeMock).ShouldDeleteIndividualResources(context.Background()) - g.Expect(actual).To(Equal(test.expected)) + actual, err := New(scopeMock).IsManaged(context.Background()) + if test.expectedError { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(actual).To(Equal(test.expected)) + } }) } } diff --git a/controllers/azuremachine_controller.go b/controllers/azuremachine_controller.go index 9b370487ba8..7c26b4552bc 100644 --- a/controllers/azuremachine_controller.go +++ b/controllers/azuremachine_controller.go @@ -28,6 +28,7 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/scope" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/groups" "sigs.k8s.io/cluster-api-provider-azure/pkg/coalescing" "sigs.k8s.io/cluster-api-provider-azure/util/reconciler" "sigs.k8s.io/cluster-api-provider-azure/util/tele" @@ -369,27 +370,32 @@ func (amr *AzureMachineReconciler) reconcileDelete(ctx context.Context, machineS return reconcile.Result{}, err } - ams, err := amr.createAzureMachineService(machineScope) - if err != nil { - return reconcile.Result{}, errors.Wrap(err, "failed to create azure machine service") - } + if ShouldDeleteIndividualResources(ctx, groups.New(clusterScope), clusterScope) { + log.Info("Deleting AzureMachine") + ams, err := amr.createAzureMachineService(machineScope) + if err != nil { + return reconcile.Result{}, errors.Wrap(err, "failed to create azure machine service") + } - if err := ams.Delete(ctx); err != nil { - // Handle transient errors - var reconcileError azure.ReconcileError - if errors.As(err, &reconcileError) { - if reconcileError.IsTransient() { - if azure.IsOperationNotDoneError(reconcileError) { - log.V(2).Info(fmt.Sprintf("AzureMachine delete not done: %s", reconcileError.Error())) - } else { - log.V(2).Info("transient failure to delete AzureMachine, retrying") + if err := ams.Delete(ctx); err != nil { + // Handle transient errors + var reconcileError azure.ReconcileError + if errors.As(err, &reconcileError) { + if reconcileError.IsTransient() { + if azure.IsOperationNotDoneError(reconcileError) { + log.V(2).Info(fmt.Sprintf("AzureMachine delete not done: %s", reconcileError.Error())) + } else { + log.V(2).Info("transient failure to delete AzureMachine, retrying") + } + return reconcile.Result{RequeueAfter: reconcileError.RequeueAfter()}, nil } - return reconcile.Result{RequeueAfter: reconcileError.RequeueAfter()}, nil } - } - amr.Recorder.Eventf(machineScope.AzureMachine, corev1.EventTypeWarning, "Error deleting AzureMachine", errors.Wrapf(err, "error deleting AzureMachine %s/%s", machineScope.Namespace(), machineScope.Name()).Error()) - return reconcile.Result{}, errors.Wrapf(err, "error deleting AzureMachine %s/%s", machineScope.Namespace(), machineScope.Name()) + amr.Recorder.Eventf(machineScope.AzureMachine, corev1.EventTypeWarning, "Error deleting AzureMachine", errors.Wrapf(err, "error deleting AzureMachine %s/%s", machineScope.Namespace(), machineScope.Name()).Error()) + return reconcile.Result{}, errors.Wrapf(err, "error deleting AzureMachine %s/%s", machineScope.Namespace(), machineScope.Name()) + } + } else { + log.Info("Skipping AzureMachine Deletion; will delete whole resource group.") } // we're done deleting this AzureMachine so remove the finalizer. diff --git a/controllers/azuremachine_reconciler.go b/controllers/azuremachine_reconciler.go index 9b43bbb5c08..f1a6d2ff8af 100644 --- a/controllers/azuremachine_reconciler.go +++ b/controllers/azuremachine_reconciler.go @@ -24,7 +24,6 @@ import ( "sigs.k8s.io/cluster-api-provider-azure/azure/scope" "sigs.k8s.io/cluster-api-provider-azure/azure/services/availabilitysets" "sigs.k8s.io/cluster-api-provider-azure/azure/services/disks" - "sigs.k8s.io/cluster-api-provider-azure/azure/services/groups" "sigs.k8s.io/cluster-api-provider-azure/azure/services/inboundnatrules" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces" "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips" @@ -153,24 +152,14 @@ func (s *azureMachineService) pause(ctx context.Context) error { // delete deletes all the services in a predetermined order. func (s *azureMachineService) delete(ctx context.Context) error { - ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.azureMachineService.delete") + ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.azureMachineService.delete") defer done() - groupSvc, err := GetService(groups.ServiceName, s.services) - if err != nil { - return errors.Wrap(err, "failed to get group service") - } - - if ShouldDeleteIndividualResources(ctx, groupSvc, s.scope.ClusterScoper) { - log.V(4).Info("deleting individual resources") - // Delete services in reverse order of creation. - for i := len(s.services) - 1; i >= 0; i-- { - if err := s.services[i].Delete(ctx); err != nil { - return errors.Wrapf(err, "failed to delete AzureMachine service %s", s.services[i].Name()) - } + // Delete services in reverse order of creation. + for i := len(s.services) - 1; i >= 0; i-- { + if err := s.services[i].Delete(ctx); err != nil { + return errors.Wrapf(err, "failed to delete AzureMachine service %s", s.services[i].Name()) } - } else { - log.V(4).Info("skipping individual resource deletion, will delete the entire resource group") } return nil diff --git a/exp/controllers/azuremachinepool_controller.go b/exp/controllers/azuremachinepool_controller.go index 7303dea703f..3139e834970 100644 --- a/exp/controllers/azuremachinepool_controller.go +++ b/exp/controllers/azuremachinepool_controller.go @@ -28,6 +28,7 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/scope" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/groups" infracontroller "sigs.k8s.io/cluster-api-provider-azure/controllers" infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/pkg/coalescing" @@ -211,7 +212,9 @@ func (ampr *AzureMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl. logger = logger.WithValues("cluster", cluster.Name) var clusterScope azure.ClusterScoper - if cluster.Spec.InfrastructureRef.Kind == "AzureCluster" { + var groupScope groups.GroupScope + switch cluster.Spec.InfrastructureRef.Kind { + case "AzureCluster": logger = logger.WithValues("AzureCluster", cluster.Spec.InfrastructureRef.Name) azureClusterName := client.ObjectKey{ Namespace: azMachinePool.Namespace, @@ -224,7 +227,7 @@ func (ampr *AzureMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl. } // Create the cluster scope - clusterScope, err = scope.NewClusterScope(ctx, scope.ClusterScopeParams{ + scoper, err := scope.NewClusterScope(ctx, scope.ClusterScopeParams{ Client: ampr.Client, Cluster: cluster, AzureCluster: azureCluster, @@ -232,7 +235,9 @@ func (ampr *AzureMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl. if err != nil { return reconcile.Result{}, err } - } else if cluster.Spec.InfrastructureRef.Kind == "AzureManagedCluster" { + clusterScope = scoper + groupScope = scoper + case "AzureManagedCluster": logger = logger.WithValues("AzureManagedCluster", cluster.Spec.InfrastructureRef.Name) azureManagedControlPlaneName := client.ObjectKey{ Namespace: azMachinePool.Namespace, @@ -245,7 +250,7 @@ func (ampr *AzureMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl. } // Create the control plane scope - clusterScope, err = scope.NewManagedControlPlaneScope(ctx, scope.ManagedControlPlaneScopeParams{ + scoper, err := scope.NewManagedControlPlaneScope(ctx, scope.ManagedControlPlaneScopeParams{ Client: ampr.Client, Cluster: cluster, ControlPlane: azureManagedControlPlane, @@ -253,7 +258,9 @@ func (ampr *AzureMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl. if err != nil { return reconcile.Result{}, err } - } else { + clusterScope = scoper + groupScope = scoper + default: return reconcile.Result{}, errors.Errorf("unsupported infrastructure type %q", cluster.Spec.InfrastructureRef.Kind) } @@ -283,7 +290,7 @@ func (ampr *AzureMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl. // Handle deleted machine pools if !azMachinePool.ObjectMeta.DeletionTimestamp.IsZero() { - return ampr.reconcileDelete(ctx, machinePoolScope, clusterScope, cluster) + return ampr.reconcileDelete(ctx, machinePoolScope, clusterScope, groupScope) } // Handle non-deleted machine pools @@ -400,18 +407,22 @@ func (ampr *AzureMachinePoolReconciler) reconcilePause(ctx context.Context, mach return reconcile.Result{}, nil } -func (ampr *AzureMachinePoolReconciler) reconcileDelete(ctx context.Context, machinePoolScope *scope.MachinePoolScope, clusterScope azure.ClusterScoper, cluster *clusterv1.Cluster) (reconcile.Result, error) { +func (ampr *AzureMachinePoolReconciler) reconcileDelete(ctx context.Context, machinePoolScope *scope.MachinePoolScope, clusterScope azure.ClusterScoper, groupScope groups.GroupScope) (reconcile.Result, error) { ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.AzureMachinePoolReconciler.reconcileDelete") defer done() log.V(2).Info("handling deleted AzureMachinePool") - amps, err := ampr.createAzureMachinePoolService(machinePoolScope) - if err != nil { - return reconcile.Result{}, errors.Wrap(err, "failed creating a new AzureMachinePoolService") - } - if err := amps.Delete(ctx); err != nil { - return reconcile.Result{}, errors.Wrapf(err, "error deleting AzureMachinePool %s/%s", machinePoolScope.AzureMachinePool.Namespace, machinePoolScope.Name()) + if infracontroller.ShouldDeleteIndividualResources(ctx, groups.New(groupScope), clusterScope) { + amps, err := ampr.createAzureMachinePoolService(machinePoolScope) + if err != nil { + return reconcile.Result{}, errors.Wrap(err, "failed creating a new AzureMachinePoolService") + } + + log.V(4).Info("deleting AzureMachinePool resource individually") + if err := amps.Delete(ctx); err != nil { + return reconcile.Result{}, errors.Wrapf(err, "error deleting AzureMachinePool %s/%s", machinePoolScope.AzureMachinePool.Namespace, machinePoolScope.Name()) + } } // Delete succeeded, remove finalizer diff --git a/exp/controllers/azuremachinepool_reconciler.go b/exp/controllers/azuremachinepool_reconciler.go index 888735fe337..fe5efcadcf2 100644 --- a/exp/controllers/azuremachinepool_reconciler.go +++ b/exp/controllers/azuremachinepool_reconciler.go @@ -22,11 +22,9 @@ import ( "github.com/pkg/errors" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/scope" - "sigs.k8s.io/cluster-api-provider-azure/azure/services/groups" "sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus" "sigs.k8s.io/cluster-api-provider-azure/azure/services/roleassignments" "sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesets" - infracontroller "sigs.k8s.io/cluster-api-provider-azure/controllers" "sigs.k8s.io/cluster-api-provider-azure/util/tele" ) @@ -103,24 +101,14 @@ func (s *azureMachinePoolService) Pause(ctx context.Context) error { // Delete reconciles all the services in pre determined order. func (s *azureMachinePoolService) Delete(ctx context.Context) error { - ctx, log, done := tele.StartSpanWithLogger(ctx, "controllers.azureMachinePoolService.Delete") + ctx, _, done := tele.StartSpanWithLogger(ctx, "controllers.azureMachinePoolService.Delete") defer done() - groupSvc, err := infracontroller.GetService(groups.ServiceName, s.services) - if err != nil { - return errors.Wrap(err, "failed to get group service") - } - - if infracontroller.ShouldDeleteIndividualResources(ctx, groupSvc, s.scope.ClusterScoper) { - log.V(4).Info("deleting individual resources") - // Delete services in reverse order of creation. - for i := len(s.services) - 1; i >= 0; i-- { - if err := s.services[i].Delete(ctx); err != nil { - return errors.Wrapf(err, "failed to delete AzureMachinePool service %s", s.services[i].Name()) - } + // Delete services in reverse order of creation. + for i := len(s.services) - 1; i >= 0; i-- { + if err := s.services[i].Delete(ctx); err != nil { + return errors.Wrapf(err, "failed to delete AzureMachinePool service %s", s.services[i].Name()) } - } else { - log.V(4).Info("skipping individual resource deletion, will delete the entire resource group") } return nil