From 31ae18fcdbb121572ca467cbc8b5c5fb8a443d80 Mon Sep 17 00:00:00 2001 From: Jonathan Tong Date: Mon, 8 Nov 2021 16:37:34 -0500 Subject: [PATCH] Make disk delete async --- api/v1beta1/conditions_consts.go | 2 + azure/scope/cluster.go | 2 + azure/scope/machine.go | 17 ++-- azure/scope/machine_test.go | 55 +++++++---- azure/services/disks/client.go | 67 ++++++++++---- azure/services/disks/disks.go | 39 +++++--- azure/services/disks/disks_test.go | 92 +++++++++++-------- .../services/disks/mock_disks/client_mock.go | 71 ++++++++++---- azure/services/disks/mock_disks/disks_mock.go | 79 +++++++++++++++- azure/services/disks/spec.go | 43 +++++++++ .../virtualmachines/virtualmachines.go | 2 + .../virtualmachines/virtualmachines_test.go | 2 + azure/types.go | 5 - 13 files changed, 354 insertions(+), 122 deletions(-) create mode 100644 azure/services/disks/spec.go diff --git a/api/v1beta1/conditions_consts.go b/api/v1beta1/conditions_consts.go index d11ffc8ec68..dfa792e46c1 100644 --- a/api/v1beta1/conditions_consts.go +++ b/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" diff --git a/azure/scope/cluster.go b/azure/scope/cluster.go index c2760667a06..00b7212d50c 100644 --- a/azure/scope/cluster.go +++ b/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, }}) } diff --git a/azure/scope/machine.go b/azure/scope/machine.go index 27ee6646703..e2216f63499 100644 --- a/azure/scope/machine.go +++ b/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. diff --git a/azure/scope/machine_test.go b/azure/scope/machine_test.go index 0141d6e0b12..5db946275c8 100644 --- a/azure/scope/machine_test.go +++ b/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)) }) } } diff --git a/azure/services/disks/client.go b/azure/services/disks/client.go index 9e5d725f556..04d0ecedc1a 100644 --- a/azure/services/disks/client.go +++ b/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. -type client interface { - Delete(context.Context, string, string) error +type Client interface { + 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. -type azureClient struct { +type AzureClient struct { disks compute.DisksClient } -var _ client = (*azureClient)(nil) +var _ Client = (*AzureClient)(nil) -// newClient creates a new VM client from subscription ID. -func newClient(auth azure.Authorizer) *azureClient { - c := newDisksClient(auth.SubscriptionID(), auth.BaseURI(), auth.Authorizer()) - return &azureClient{c} +// NewClient creates a new VM Client from subscription ID. +func NewClient(auth azure.Authorizer) *AzureClient { + 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 } diff --git a/azure/services/disks/disks.go b/azure/services/disks/disks.go index 9bbe9aa4ab4..a8d81aad68f 100644 --- a/azure/services/disks/disks.go +++ b/azure/services/disks/disks.go @@ -20,30 +20,35 @@ 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. type Service struct { Scope DiskScope - client + Client } // New creates a new disks service. func New(scope DiskScope) *Service { return &Service{ Scope: scope, - client: newClient(scope), + Client: NewClient(scope), } } @@ -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 } diff --git a/azure/services/disks/disks_test.go b/azure/services/disks/disks_test.go index ff0427d8f66..bb966c87191 100644 --- a/azure/services/disks/disks_test.go +++ b/azure/services/disks/disks_test.go @@ -18,6 +18,7 @@ package disks import ( "context" + "fmt" "net/http" "testing" @@ -25,68 +26,81 @@ import ( "github.com/golang/mock/gomock" . "github.com/onsi/gomega" "k8s.io/klog/v2/klogr" + 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/mock_disks" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) +var ( + diskSpec1 = DiskSpec{ + Name: "my-disk-1", + ResourceGroup: "my-group", + } + + diskSpec2 = DiskSpec{ + Name: "my-disk-2", + ResourceGroup: "my-group", + } + + fakeDiskSpecs = []azure.ResourceSpecGetter{ + &diskSpec1, + &diskSpec2, + } + + internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") + notFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") +) + func TestDeleteDisk(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) + expect func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder) }{ { name: "delete the disk", expectedError: "", - expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) { + expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.DiskSpecs().Return([]azure.DiskSpec{ - { - Name: "my-disk-1", - }, - { - Name: "honk-disk", - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - m.Delete(gomockinternal.AContext(), "my-rg", "my-disk-1") - m.Delete(gomockinternal.AContext(), "my-rg", "honk-disk") + s.DiskSpecs().Return(fakeDiskSpecs) + gomock.InOrder( + s.GetLongRunningOperationState("my-disk-1", serviceName), + m.DeleteAsync(gomockinternal.AContext(), &diskSpec1).Return(nil, nil), + s.GetLongRunningOperationState("my-disk-2", serviceName), + m.DeleteAsync(gomockinternal.AContext(), &diskSpec2).Return(nil, nil), + s.UpdateDeleteStatus(infrav1.DisksReadyCondition, serviceName, nil), + ) }, }, { name: "disk already deleted", expectedError: "", - expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) { + expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.DiskSpecs().Return([]azure.DiskSpec{ - { - Name: "my-disk-1", - }, - { - Name: "my-disk-2", - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - m.Delete(gomockinternal.AContext(), "my-rg", "my-disk-1").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found")) - m.Delete(gomockinternal.AContext(), "my-rg", "my-disk-2").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found")) + s.DiskSpecs().Return(fakeDiskSpecs) + gomock.InOrder( + s.GetLongRunningOperationState("my-disk-1", serviceName), + m.DeleteAsync(gomockinternal.AContext(), &diskSpec1).Return(nil, notFoundError), + s.GetLongRunningOperationState("my-disk-2", serviceName), + m.DeleteAsync(gomockinternal.AContext(), &diskSpec2).Return(nil, nil), + s.UpdateDeleteStatus(infrav1.DisksReadyCondition, serviceName, nil), + ) }, }, { name: "error while trying to delete the disk", - expectedError: "failed to delete disk my-disk-1 in resource group my-rg: #: Internal Server Error: StatusCode=500", - expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockclientMockRecorder) { + expectedError: "failed to delete resource my-group/my-disk-1 (service: disks): #: Internal Server Error: StatusCode=500", + expect: func(s *mock_disks.MockDiskScopeMockRecorder, m *mock_disks.MockClientMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.DiskSpecs().Return([]azure.DiskSpec{ - { - Name: "my-disk-1", - }, - { - Name: "my-disk-2", - }, - }) - s.ResourceGroup().AnyTimes().Return("my-rg") - m.Delete(gomockinternal.AContext(), "my-rg", "my-disk-1").Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error")) + s.DiskSpecs().Return(fakeDiskSpecs) + gomock.InOrder( + s.GetLongRunningOperationState("my-disk-1", serviceName), + m.DeleteAsync(gomockinternal.AContext(), &diskSpec1).Return(nil, internalError), + s.GetLongRunningOperationState("my-disk-2", serviceName), + m.DeleteAsync(gomockinternal.AContext(), &diskSpec2).Return(nil, nil), + s.UpdateDeleteStatus(infrav1.DisksReadyCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to delete resource my-group/my-disk-1 (service: disks): %s", internalError.Error()))), + ) }, }, } @@ -100,13 +114,13 @@ func TestDeleteDisk(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() scopeMock := mock_disks.NewMockDiskScope(mockCtrl) - clientMock := mock_disks.NewMockclient(mockCtrl) + clientMock := mock_disks.NewMockClient(mockCtrl) tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) s := &Service{ Scope: scopeMock, - client: clientMock, + Client: clientMock, } err := s.Delete(context.TODO()) diff --git a/azure/services/disks/mock_disks/client_mock.go b/azure/services/disks/mock_disks/client_mock.go index 019e04700f6..a5552fc4c55 100644 --- a/azure/services/disks/mock_disks/client_mock.go +++ b/azure/services/disks/mock_disks/client_mock.go @@ -24,42 +24,75 @@ import ( context "context" reflect "reflect" + azure "github.com/Azure/go-autorest/autorest/azure" gomock "github.com/golang/mock/gomock" + azure0 "sigs.k8s.io/cluster-api-provider-azure/azure" ) -// Mockclient is a mock of client interface. -type Mockclient struct { +// MockClient is a mock of Client interface. +type MockClient struct { ctrl *gomock.Controller - recorder *MockclientMockRecorder + recorder *MockClientMockRecorder } -// MockclientMockRecorder is the mock recorder for Mockclient. -type MockclientMockRecorder struct { - mock *Mockclient +// MockClientMockRecorder is the mock recorder for MockClient. +type MockClientMockRecorder struct { + mock *MockClient } -// NewMockclient creates a new mock instance. -func NewMockclient(ctrl *gomock.Controller) *Mockclient { - mock := &Mockclient{ctrl: ctrl} - mock.recorder = &MockclientMockRecorder{mock} +// NewMockClient creates a new mock instance. +func NewMockClient(ctrl *gomock.Controller) *MockClient { + mock := &MockClient{ctrl: ctrl} + mock.recorder = &MockClientMockRecorder{mock} return mock } // EXPECT returns an object that allows the caller to indicate expected use. -func (m *Mockclient) EXPECT() *MockclientMockRecorder { +func (m *MockClient) EXPECT() *MockClientMockRecorder { return m.recorder } -// Delete mocks base method. -func (m *Mockclient) Delete(arg0 context.Context, arg1, arg2 string) error { +// DeleteAsync mocks base method. +func (m *MockClient) DeleteAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (azure.FutureAPI, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Delete", arg0, arg1, arg2) - ret0, _ := ret[0].(error) - return ret0 + ret := m.ctrl.Call(m, "DeleteAsync", arg0, arg1) + ret0, _ := ret[0].(azure.FutureAPI) + ret1, _ := ret[1].(error) + return ret0, ret1 } -// Delete indicates an expected call of Delete. -func (mr *MockclientMockRecorder) Delete(arg0, arg1, arg2 interface{}) *gomock.Call { +// DeleteAsync indicates an expected call of DeleteAsync. +func (mr *MockClientMockRecorder) DeleteAsync(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*Mockclient)(nil).Delete), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAsync", reflect.TypeOf((*MockClient)(nil).DeleteAsync), arg0, arg1) +} + +// IsDone mocks base method. +func (m *MockClient) IsDone(arg0 context.Context, arg1 azure.FutureAPI) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsDone", arg0, arg1) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// IsDone indicates an expected call of IsDone. +func (mr *MockClientMockRecorder) IsDone(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsDone", reflect.TypeOf((*MockClient)(nil).IsDone), arg0, arg1) +} + +// Result mocks base method. +func (m *MockClient) Result(arg0 context.Context, arg1 azure.FutureAPI, arg2 string) (interface{}, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Result", arg0, arg1, arg2) + ret0, _ := ret[0].(interface{}) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Result indicates an expected call of Result. +func (mr *MockClientMockRecorder) Result(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Result", reflect.TypeOf((*MockClient)(nil).Result), arg0, arg1, arg2) } diff --git a/azure/services/disks/mock_disks/disks_mock.go b/azure/services/disks/mock_disks/disks_mock.go index 24ce5d5ae53..f037c1bfc60 100644 --- a/azure/services/disks/mock_disks/disks_mock.go +++ b/azure/services/disks/mock_disks/disks_mock.go @@ -28,6 +28,7 @@ import ( gomock "github.com/golang/mock/gomock" v1beta1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" azure "sigs.k8s.io/cluster-api-provider-azure/azure" + v1beta10 "sigs.k8s.io/cluster-api/api/v1beta1" ) // MockDiskScope is a mock of DiskScope interface. @@ -179,11 +180,23 @@ func (mr *MockDiskScopeMockRecorder) ClusterName() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ClusterName", reflect.TypeOf((*MockDiskScope)(nil).ClusterName)) } +// DeleteLongRunningOperationState mocks base method. +func (m *MockDiskScope) DeleteLongRunningOperationState(arg0, arg1 string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "DeleteLongRunningOperationState", arg0, arg1) +} + +// DeleteLongRunningOperationState indicates an expected call of DeleteLongRunningOperationState. +func (mr *MockDiskScopeMockRecorder) DeleteLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLongRunningOperationState", reflect.TypeOf((*MockDiskScope)(nil).DeleteLongRunningOperationState), arg0, arg1) +} + // DiskSpecs mocks base method. -func (m *MockDiskScope) DiskSpecs() []azure.DiskSpec { +func (m *MockDiskScope) DiskSpecs() []azure.ResourceSpecGetter { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "DiskSpecs") - ret0, _ := ret[0].([]azure.DiskSpec) + ret0, _ := ret[0].([]azure.ResourceSpecGetter) return ret0 } @@ -238,6 +251,20 @@ func (mr *MockDiskScopeMockRecorder) FailureDomains() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FailureDomains", reflect.TypeOf((*MockDiskScope)(nil).FailureDomains)) } +// GetLongRunningOperationState mocks base method. +func (m *MockDiskScope) GetLongRunningOperationState(arg0, arg1 string) *v1beta1.Future { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetLongRunningOperationState", arg0, arg1) + ret0, _ := ret[0].(*v1beta1.Future) + return ret0 +} + +// GetLongRunningOperationState indicates an expected call of GetLongRunningOperationState. +func (mr *MockDiskScopeMockRecorder) GetLongRunningOperationState(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLongRunningOperationState", reflect.TypeOf((*MockDiskScope)(nil).GetLongRunningOperationState), arg0, arg1) +} + // HashKey mocks base method. func (m *MockDiskScope) HashKey() string { m.ctrl.T.Helper() @@ -297,6 +324,18 @@ func (mr *MockDiskScopeMockRecorder) ResourceGroup() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResourceGroup", reflect.TypeOf((*MockDiskScope)(nil).ResourceGroup)) } +// SetLongRunningOperationState mocks base method. +func (m *MockDiskScope) SetLongRunningOperationState(arg0 *v1beta1.Future) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetLongRunningOperationState", arg0) +} + +// SetLongRunningOperationState indicates an expected call of SetLongRunningOperationState. +func (mr *MockDiskScopeMockRecorder) SetLongRunningOperationState(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetLongRunningOperationState", reflect.TypeOf((*MockDiskScope)(nil).SetLongRunningOperationState), arg0) +} + // SubscriptionID mocks base method. func (m *MockDiskScope) SubscriptionID() string { m.ctrl.T.Helper() @@ -325,6 +364,42 @@ func (mr *MockDiskScopeMockRecorder) TenantID() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TenantID", reflect.TypeOf((*MockDiskScope)(nil).TenantID)) } +// UpdateDeleteStatus mocks base method. +func (m *MockDiskScope) UpdateDeleteStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdateDeleteStatus", arg0, arg1, arg2) +} + +// UpdateDeleteStatus indicates an expected call of UpdateDeleteStatus. +func (mr *MockDiskScopeMockRecorder) UpdateDeleteStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateDeleteStatus", reflect.TypeOf((*MockDiskScope)(nil).UpdateDeleteStatus), arg0, arg1, arg2) +} + +// UpdatePatchStatus mocks base method. +func (m *MockDiskScope) UpdatePatchStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePatchStatus", arg0, arg1, arg2) +} + +// UpdatePatchStatus indicates an expected call of UpdatePatchStatus. +func (mr *MockDiskScopeMockRecorder) UpdatePatchStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePatchStatus", reflect.TypeOf((*MockDiskScope)(nil).UpdatePatchStatus), arg0, arg1, arg2) +} + +// UpdatePutStatus mocks base method. +func (m *MockDiskScope) UpdatePutStatus(arg0 v1beta10.ConditionType, arg1 string, arg2 error) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "UpdatePutStatus", arg0, arg1, arg2) +} + +// UpdatePutStatus indicates an expected call of UpdatePutStatus. +func (mr *MockDiskScopeMockRecorder) UpdatePutStatus(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdatePutStatus", reflect.TypeOf((*MockDiskScope)(nil).UpdatePutStatus), arg0, arg1, arg2) +} + // V mocks base method. func (m *MockDiskScope) V(level int) logr.Logger { m.ctrl.T.Helper() diff --git a/azure/services/disks/spec.go b/azure/services/disks/spec.go new file mode 100644 index 00000000000..09298bf8b35 --- /dev/null +++ b/azure/services/disks/spec.go @@ -0,0 +1,43 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package disks + +// DiskSpec defines the specification for a disk. +type DiskSpec struct { + Name string + ResourceGroup string +} + +// ResourceName returns the name of the disk. +func (s *DiskSpec) ResourceName() string { + return s.Name +} + +// ResourceGroupName returns the name of the resource group. +func (s *DiskSpec) ResourceGroupName() string { + return s.ResourceGroup +} + +// OwnerResourceName is a no-op for disks. +func (s *DiskSpec) OwnerResourceName() string { + return "" +} + +// Parameters is a no-op for disks. +func (s *DiskSpec) Parameters(existing interface{}) (interface{}, error) { + return nil, nil +} diff --git a/azure/services/virtualmachines/virtualmachines.go b/azure/services/virtualmachines/virtualmachines.go index f974f98e6da..9a8bf78cfae 100644 --- a/azure/services/virtualmachines/virtualmachines.go +++ b/azure/services/virtualmachines/virtualmachines.go @@ -83,6 +83,8 @@ func (s *Service) Reconcile(ctx context.Context) error { result, err := async.CreateResource(ctx, s.Scope, s.Client, vmSpec, serviceName) s.Scope.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, err) + // Set the DiskReady condition here since the disk gets created with the VM. + s.Scope.UpdatePutStatus(infrav1.DisksReadyCondition, serviceName, err) if err == nil && result != nil { vm, ok := result.(compute.VirtualMachine) if !ok { diff --git a/azure/services/virtualmachines/virtualmachines_test.go b/azure/services/virtualmachines/virtualmachines_test.go index 2054655986c..8a437e2a4b8 100644 --- a/azure/services/virtualmachines/virtualmachines_test.go +++ b/azure/services/virtualmachines/virtualmachines_test.go @@ -96,6 +96,7 @@ func TestReconcileVM(t *testing.T) { }, }, nil, nil) s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, nil) + s.UpdatePutStatus(infrav1.DisksReadyCondition, serviceName, nil) s.SetProviderID("azure://test-vm-id") s.SetAnnotation("cluster-api-provider-azure", "true") mnic.Get(gomockinternal.AContext(), "test-group", "nic-1").Return(network.Interface{ @@ -139,6 +140,7 @@ func TestReconcileVM(t *testing.T) { s.GetLongRunningOperationState("test-vm", serviceName) m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeVMSpec).Return(nil, nil, internalError) s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to create resource test-group/test-vm (service: virtualmachine): %s", internalError.Error()))) + s.UpdatePutStatus(infrav1.DisksReadyCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to create resource test-group/test-vm (service: virtualmachine): %s", internalError.Error()))) }, }, } diff --git a/azure/types.go b/azure/types.go index 0cf5e361969..2de2840aa6c 100644 --- a/azure/types.go +++ b/azure/types.go @@ -51,11 +51,6 @@ type NICSpec struct { EnableIPForwarding bool } -// DiskSpec defines the specification for a Disk. -type DiskSpec struct { - Name string -} - // LBSpec defines the specification for a Load Balancer. type LBSpec struct { Name string