From 6a5db52aafa062aeac6da0ef6b6b8bc6b7dce87a Mon Sep 17 00:00:00 2001 From: Jonathan Tong Date: Mon, 13 Dec 2021 16:19:04 -0800 Subject: [PATCH] Refactor Delete logic --- .../availabilitysets/availabilitysets.go | 52 ++++++++----------- azure/services/availabilitysets/spec.go | 15 +++--- azure/services/availabilitysets/spec_test.go | 23 +++++++- 3 files changed, 51 insertions(+), 39 deletions(-) diff --git a/azure/services/availabilitysets/availabilitysets.go b/azure/services/availabilitysets/availabilitysets.go index 12d6c762d71..17416a9f21f 100644 --- 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 index c87b51da25b..1f884500897 100644 --- 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 index 20f9b77c07e..818ff532987 100644 --- 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",