Skip to content

Commit

Permalink
fix some more unit tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@CecileRobertMichon
CecileRobertMichon committed Oct 3, 2023
1 parent 0f1e1cd commit 2cbbee5
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 71 deletions.
21 changes: 13 additions & 8 deletions azure/services/groups/groups_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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))
}
})
}
}
40 changes: 23 additions & 17 deletions controllers/azuremachine_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down
21 changes: 5 additions & 16 deletions controllers/azuremachine_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
37 changes: 24 additions & 13 deletions exp/controllers/azuremachinepool_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand All @@ -224,15 +227,17 @@ 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,
})
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,
Expand All @@ -245,15 +250,17 @@ 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,
})
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)
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
22 changes: 5 additions & 17 deletions exp/controllers/azuremachinepool_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 2cbbee5

Please sign in to comment.