Make disks delete async

api/v1beta1/conditions_consts.go

@@ -106,6 +106,8 @@ const (
 	AvailabilitySetReadyCondition clusterv1.ConditionType = "AvailabilitySetReady"
 	// RoleAssignmentReadyCondition means the role assignment exists and is ready to be used.
 	RoleAssignmentReadyCondition clusterv1.ConditionType = "RoleAssignmentReady"
+	// DisksReadyCondition means the disks exist and are ready to be used.
+	DisksReadyCondition clusterv1.ConditionType = "DisksReady"
 
 	// CreatingReason means the resource is being created.
 	CreatingReason = "Creating"

azure/scope/cluster.go

@@ -600,6 +600,7 @@ func (s *ClusterScope) PatchObject(ctx context.Context) error {
 			infrav1.ResourceGroupReadyCondition,
 			infrav1.NetworkInfrastructureReadyCondition,
 			infrav1.VnetPeeringReadyCondition,
+			infrav1.DisksReadyCondition,
 		),
 	)
 
@@ -611,6 +612,7 @@ func (s *ClusterScope) PatchObject(ctx context.Context) error {
 			infrav1.ResourceGroupReadyCondition,
 			infrav1.NetworkInfrastructureReadyCondition,
 			infrav1.VnetPeeringReadyCondition,
+			infrav1.DisksReadyCondition,
 		}})
 }
 

azure/scope/machine.go

@@ -39,6 +39,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/services/disks"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/resourceskus"
 	"sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines"
 	"sigs.k8s.io/cluster-api-provider-azure/util/futures"
@@ -252,16 +253,20 @@ func (m *MachineScope) NICIDs() []string {
 }
 
 // DiskSpecs returns the disk specs.
-func (m *MachineScope) DiskSpecs() []azure.DiskSpec {
-	disks := make([]azure.DiskSpec, 1+len(m.AzureMachine.Spec.DataDisks))
-	disks[0] = azure.DiskSpec{
-		Name: azure.GenerateOSDiskName(m.Name()),
+func (m *MachineScope) DiskSpecs() []azure.ResourceSpecGetter {
+	diskSpecs := make([]azure.ResourceSpecGetter, 1+len(m.AzureMachine.Spec.DataDisks))
+	diskSpecs[0] = &disks.DiskSpec{
+		Name:          azure.GenerateOSDiskName(m.Name()),
+		ResourceGroup: m.ResourceGroup(),
 	}
 
 	for i, dd := range m.AzureMachine.Spec.DataDisks {
-		disks[i+1] = azure.DiskSpec{Name: azure.GenerateDataDiskName(m.Name(), dd.NameSuffix)}
+		diskSpecs[i+1] = &disks.DiskSpec{
+			Name:          azure.GenerateDataDiskName(m.Name(), dd.NameSuffix),
+			ResourceGroup: m.ResourceGroup(),
+		}
 	}
-	return disks
+	return diskSpecs
 }
 
 // RoleAssignmentSpecs returns the role assignment specs.

azure/scope/machine_test.go

@@ -25,11 +25,13 @@ import (
 	autorestazure "github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/go-autorest/autorest/azure/auth"
 	"github.com/Azure/go-autorest/autorest/to"
+	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/pointer"
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
+	"sigs.k8s.io/cluster-api-provider-azure/azure/services/disks"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 )
 
@@ -1731,7 +1733,7 @@ func TestDiskSpecs(t *testing.T) {
 	testcases := []struct {
 		name         string
 		machineScope MachineScope
-		want         []azure.DiskSpec
+		want         []azure.ResourceSpecGetter
 	}{
 		{
 			name: "only os disk",
@@ -1746,6 +1748,9 @@ func TestDiskSpecs(t *testing.T) {
 						ObjectMeta: metav1.ObjectMeta{
 							Name: "cluster",
 						},
+						Spec: infrav1.AzureClusterSpec{
+							ResourceGroup: "my-rg",
+						},
 					},
 				},
 				AzureMachine: &infrav1.AzureMachine{
@@ -1765,9 +1770,10 @@ func TestDiskSpecs(t *testing.T) {
 					},
 				},
 			},
-			want: []azure.DiskSpec{
-				{
-					Name: "my-azure-machine_OSDisk",
+			want: []azure.ResourceSpecGetter{
+				&disks.DiskSpec{
+					Name:          "my-azure-machine_OSDisk",
+					ResourceGroup: "my-rg",
 				},
 			},
 		},
@@ -1784,6 +1790,9 @@ func TestDiskSpecs(t *testing.T) {
 						ObjectMeta: metav1.ObjectMeta{
 							Name: "cluster",
 						},
+						Spec: infrav1.AzureClusterSpec{
+							ResourceGroup: "my-rg",
+						},
 					},
 				},
 				AzureMachine: &infrav1.AzureMachine{
@@ -1808,12 +1817,14 @@ func TestDiskSpecs(t *testing.T) {
 					},
 				},
 			},
-			want: []azure.DiskSpec{
-				{
-					Name: "my-azure-machine_OSDisk",
+			want: []azure.ResourceSpecGetter{
+				&disks.DiskSpec{
+					Name:          "my-azure-machine_OSDisk",
+					ResourceGroup: "my-rg",
 				},
-				{
-					Name: "my-azure-machine_etcddisk",
+				&disks.DiskSpec{
+					Name:          "my-azure-machine_etcddisk",
+					ResourceGroup: "my-rg",
 				},
 			},
 		}, {
@@ -1829,6 +1840,9 @@ func TestDiskSpecs(t *testing.T) {
 						ObjectMeta: metav1.ObjectMeta{
 							Name: "cluster",
 						},
+						Spec: infrav1.AzureClusterSpec{
+							ResourceGroup: "my-rg",
+						},
 					},
 				},
 				AzureMachine: &infrav1.AzureMachine{
@@ -1856,15 +1870,18 @@ func TestDiskSpecs(t *testing.T) {
 					},
 				},
 			},
-			want: []azure.DiskSpec{
-				{
-					Name: "my-azure-machine_OSDisk",
+			want: []azure.ResourceSpecGetter{
+				&disks.DiskSpec{
+					Name:          "my-azure-machine_OSDisk",
+					ResourceGroup: "my-rg",
 				},
-				{
-					Name: "my-azure-machine_etcddisk",
+				&disks.DiskSpec{
+					Name:          "my-azure-machine_etcddisk",
+					ResourceGroup: "my-rg",
 				},
-				{
-					Name: "my-azure-machine_otherdisk",
+				&disks.DiskSpec{
+					Name:          "my-azure-machine_otherdisk",
+					ResourceGroup: "my-rg",
 				},
 			},
 		},
@@ -1873,11 +1890,11 @@ func TestDiskSpecs(t *testing.T) {
 	for _, tt := range testcases {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
 			t.Parallel()
 			result := tt.machineScope.DiskSpecs()
-			if !reflect.DeepEqual(result, tt.want) {
-				t.Errorf("DiskSpecs(), DiskSpecs = %v, want %v", result, tt.want)
-			}
+			g.Expect(result).To(BeEquivalentTo(tt.want))
 		})
 	}
 }

azure/services/disks/client.go

@@ -21,48 +21,81 @@ import (
 
 	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-04-01/compute"
 	"github.com/Azure/go-autorest/autorest"
+	azureautorest "github.com/Azure/go-autorest/autorest/azure"
+	"github.com/pkg/errors"
+
 	"sigs.k8s.io/cluster-api-provider-azure/azure"
+	"sigs.k8s.io/cluster-api-provider-azure/util/reconciler"
 	"sigs.k8s.io/cluster-api-provider-azure/util/tele"
 )
 
-// Client wraps go-sdk.
+// client wraps go-sdk.
 type client interface {
-	Delete(context.Context, string, string) error
+	DeleteAsync(context.Context, azure.ResourceSpecGetter) (azureautorest.FutureAPI, error)
+	Result(context.Context, azureautorest.FutureAPI, string) (interface{}, error)
+	IsDone(context.Context, azureautorest.FutureAPI) (bool, error)
 }
 
-// AzureClient contains the Azure go-sdk Client.
+// azureClient contains the Azure go-sdk Client.
 type azureClient struct {
 	disks compute.DisksClient
 }
 
 var _ client = (*azureClient)(nil)
 
-// newClient creates a new VM client from subscription ID.
+// newClient creates a new VM Client from subscription ID.
 func newClient(auth azure.Authorizer) *azureClient {
-	c := newDisksClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer())
+	c := NewDisksClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer())
 	return &azureClient{c}
 }
 
-// newDisksClient creates a new disks client from subscription ID.
-func newDisksClient(subscriptionID string, baseURI string, authorizer autorest.Authorizer) compute.DisksClient {
+// NewDisksClient creates a new disks Client from subscription ID.
+func NewDisksClient(subscriptionID string, baseURI string, authorizer autorest.Authorizer) compute.DisksClient {
 	disksClient := compute.NewDisksClientWithBaseURI(baseURI, subscriptionID)
 	azure.SetAutoRestClientDefaults(&disksClient.Client, authorizer)
 	return disksClient
 }
 
-// Delete removes the disk client.
-func (ac *azureClient) Delete(ctx context.Context, resourceGroupName, name string) error {
-	ctx, _, done := tele.StartSpanWithLogger(ctx, "disks.AzureClient.Delete")
+// DeleteAsync deletes a route table asynchronously. DeleteAsync sends a DELETE
+// request to Azure and if accepted without error, the func will return a Future which can be used to track the ongoing
+// progress of the operation.
+func (ac *azureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) {
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "disks.Service.DeleteAsync")
 	defer done()
 
-	future, err := ac.disks.Delete(ctx, resourceGroupName, name)
+	future, err := ac.disks.Delete(ctx, spec.ResourceGroupName(), spec.ResourceName())
 	if err != nil {
-		return err
+		return nil, err
 	}
+
+	ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout)
+	defer cancel()
+
 	err = future.WaitForCompletionRef(ctx, ac.disks.Client)
 	if err != nil {
-		return err
+		// if an error occurs, return the future.
+		// this means the long-running operation didn't finish in the specified timeout.
+		return &future, err
 	}
 	_, err = future.Result(ac.disks)
-	return err
+	// if the operation completed, return a nil future.
+	return nil, err
+}
+
+// Result fetches the result of a long-running operation future.
+func (ac *azureClient) Result(ctx context.Context, futureData azureautorest.FutureAPI, futureType string) (interface{}, error) {
+	return nil, nil
+}
+
+// IsDone returns true if the long-running operation has completed.
+func (ac *azureClient) IsDone(ctx context.Context, future azureautorest.FutureAPI) (bool, error) {
+	ctx, _, done := tele.StartSpanWithLogger(ctx, "natgateways.Service.IsDone")
+	defer done()
+
+	isDone, err := future.DoneWithContext(ctx, ac.disks)
+	if err != nil {
+		return false, errors.Wrap(err, "failed checking if the operation was complete")
+	}
+
+	return isDone, nil
 }

azure/services/disks/disks.go

@@ -20,17 +20,22 @@ import (
 	"context"
 
 	"github.com/go-logr/logr"
-	"github.com/pkg/errors"
 
+	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/services/async"
+	"sigs.k8s.io/cluster-api-provider-azure/util/reconciler"
 	"sigs.k8s.io/cluster-api-provider-azure/util/tele"
 )
 
+const serviceName = "disks"
+
 // DiskScope defines the scope interface for a disk service.
 type DiskScope interface {
 	logr.Logger
 	azure.ClusterDescriber
-	DiskSpecs() []azure.DiskSpec
+	azure.AsyncStatusUpdater
+	DiskSpecs() []azure.ResourceSpecGetter
 }
 
 // Service provides operations on Azure resources.
@@ -52,6 +57,7 @@ func (s *Service) Reconcile(ctx context.Context) error {
 	_, _, done := tele.StartSpanWithLogger(ctx, "disks.Service.Reconcile")
 	defer done()
 
+	// DisksReadyCondition is set in the VM service.
 	return nil
 }
 
@@ -60,18 +66,21 @@ func (s *Service) Delete(ctx context.Context) error {
 	ctx, _, done := tele.StartSpanWithLogger(ctx, "disks.Service.Delete")
 	defer done()
 
+	ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureServiceReconcileTimeout)
+	defer cancel()
+
+	var result error
+
+	// We go through the list of DiskSpecs to delete each one, independently of the result of the previous one.
+	// If multiple errors occur, we return the most pressing one
+	// order of precedence is: error deleting -> deleting in progress -> deleted (no error)
 	for _, diskSpec := range s.Scope.DiskSpecs() {
-		s.Scope.V(2).Info("deleting disk", "disk", diskSpec.Name)
-		err := s.client.Delete(ctx, s.Scope.ResourceGroup(), diskSpec.Name)
-		if err != nil && azure.ResourceNotFound(err) {
-			// already deleted
-			continue
+		if err := async.DeleteResource(ctx, s.Scope, s.client, diskSpec, serviceName); err != nil {
+			if !azure.IsOperationNotDoneError(err) || result == nil {
+				result = err
+			}
 		}
-		if err != nil {
-			return errors.Wrapf(err, "failed to delete disk %s in resource group %s", diskSpec.Name, s.Scope.ResourceGroup())
-		}
-
-		s.Scope.V(2).Info("successfully deleted disk", "disk", diskSpec.Name)
 	}
-	return nil
+	s.Scope.UpdateDeleteStatus(infrav1.DisksReadyCondition, serviceName, result)
+	return result
 }