Refactor Delete logic

kubernetes-sigs · Dec 14, 2021 · 6a5db52 · 6a5db52
1 parent 988fe15
commit 6a5db52
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 39 deletions.
diff --git a/azure/services/availabilitysets/availabilitysets.go b/azure/services/availabilitysets/availabilitysets.go
@@ -86,40 +86,30 @@ func (s *Service) Delete(ctx context.Context) error {
 	defer cancel()
 
 	setSpec := s.Scope.AvailabilitySetSpec()
+	var resultingErr error
 	if setSpec == nil {
 		log.V(2).Info("skip deletion when no availability set spec is found")
-		s.Scope.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil)
-		return nil
-	}
-
-	existingSet, err := s.Client.Get(ctx, setSpec)
-	if err != nil {
-		if azure.ResourceNotFound(err) {
-			// already deleted
-			s.Scope.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil)
-			return nil
+	} else {
+		existingSet, err := s.Client.Get(ctx, setSpec)
+		if err != nil {
+			if !azure.ResourceNotFound(err) {
+				resultingErr = errors.Wrapf(err, "failed to get availability set %s in resource group %s", setSpec.ResourceName(), setSpec.ResourceGroupName())
+			}
+		} else {
+			availabilitySet, ok := existingSet.(compute.AvailabilitySet)
+			if !ok {
+				resultingErr = errors.Errorf("%T is not a compute.AvailabilitySet", existingSet)
+			} else {
+				// only delete when the availability set does not have any vms
+				if availabilitySet.AvailabilitySetProperties != nil && availabilitySet.VirtualMachines != nil && len(*availabilitySet.VirtualMachines) > 0 {
+					log.V(2).Info("skip deleting availability set with VMs", "availability set", setSpec.ResourceName())
+				} else {
+					resultingErr = s.DeleteResource(ctx, setSpec, serviceName)
+				}
+			}
 		}
-		resultingErr := errors.Wrapf(err, "failed to get availability set %s in resource group %s", setSpec.ResourceName(), setSpec.ResourceGroupName())
-
-		s.Scope.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, resultingErr)
-		return resultingErr
 	}
-	availabilitySet, ok := existingSet.(compute.AvailabilitySet)
-	if !ok {
-		err = errors.Errorf("%T is not a compute.AvailabilitySet", existingSet)
 
-		s.Scope.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, err)
-		return err
-	}
-
-	// only delete when the availability set does not have any vms
-	if availabilitySet.AvailabilitySetProperties != nil && availabilitySet.VirtualMachines != nil && len(*availabilitySet.VirtualMachines) > 0 {
-		log.V(2).Info("skip deleting availability set with VMs", "availability set", setSpec.ResourceName())
-		s.Scope.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil)
-		return nil
-	}
-
-	err = s.DeleteResource(ctx, setSpec, serviceName)
-	s.Scope.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, err)
-	return err
+	s.Scope.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, resultingErr)
+	return resultingErr
 }
diff --git a/azure/services/availabilitysets/spec.go b/azure/services/availabilitysets/spec.go
@@ -63,18 +63,19 @@ func (s *AvailabilitySetSpec) Parameters(existing interface{}) (interface{}, err
 	}
 
 	if s.SKU == nil {
-		return nil, errors.New("unable to find required availability set SKU from machine cache")
+		return nil, errors.New("unable to get required availability set SKU from machine cache")
 	}
 
 	var faultDomainCount *int32
 	faultDomainCountStr, ok := s.SKU.GetCapability(resourceskus.MaximumPlatformFaultDomainCount)
-	if ok {
-		count, err := strconv.ParseInt(faultDomainCountStr, 10, 32)
-		if err != nil {
-			return nil, errors.Wrapf(err, "unable to parse availability set fault domain count")
-		}
-		faultDomainCount = to.Int32Ptr(int32(count))
+	if !ok {
+		return nil, errors.Errorf("unable to get required availability set SKU capability %s", resourceskus.MaximumPlatformFaultDomainCount)
+	}
+	count, err := strconv.ParseInt(faultDomainCountStr, 10, 32)
+	if err != nil {
+		return nil, errors.Wrapf(err, "unable to parse availability set fault domain count")
 	}
+	faultDomainCount = to.Int32Ptr(int32(count))
 
 	asParams := compute.AvailabilitySet{
 		Sku: &compute.Sku{

diff --git a/azure/services/availabilitysets/spec_test.go b/azure/services/availabilitysets/spec_test.go
@@ -22,6 +22,18 @@ import (
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-04-01/compute"
 	"github.com/Azure/go-autorest/autorest/to"
 	. "github.com/onsi/gomega"
+	"sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus"
+)
+
+var (
+	fakeSetSpecMissingCap = AvailabilitySetSpec{
+		Name:           "test-as",
+		ResourceGroup:  "test-rg",
+		ClusterName:    "test-cluster",
+		Location:       "test-location",
+		SKU:            &resourceskus.SKU{},
+		AdditionalTags: map[string]string{},
+	}
 )
 
 func TestParameters(t *testing.T) {
@@ -39,7 +51,16 @@ func TestParameters(t *testing.T) {
 			expect: func(g *WithT, result interface{}) {
 				g.Expect(result).To(BeNil())
 			},
-			expectedError: "unable to find required availability set SKU from machine cache",
+			expectedError: "unable to get required availability set SKU from machine cache",
+		},
+		{
+			name:     "error when SKU capability is missing",
+			spec:     &fakeSetSpecMissingCap,
+			existing: nil,
+			expect: func(g *WithT, result interface{}) {
+				g.Expect(result).To(BeNil())
+			},
+			expectedError: "unable to get required availability set SKU capability MaximumPlatformFaultDomainCount",
 		},
 		{
 			name:     "get parameters when all values are present",