From dd31469ef63dc91c01d561d91f27473a2a779f4f Mon Sep 17 00:00:00 2001 From: Jonathan Tong Date: Mon, 8 Nov 2021 16:37:34 -0500 Subject: [PATCH] Add async disk service and generate code --- api/v1beta1/conditions_consts.go | 2 + azure/scope/machine.go | 14 ++-- azure/services/disks/client.go | 68 +++++++++++----- azure/services/disks/disks.go | 42 ++++++---- .../services/disks/mock_disks/client_mock.go | 71 ++++++++++++----- azure/services/disks/mock_disks/disks_mock.go | 79 ++++++++++++++++++- azure/services/disks/spec.go | 40 ++++++++++ azure/types.go | 5 -- 8 files changed, 254 insertions(+), 67 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/machine.go b/azure/scope/machine.go index 27ee6646703..2461fb21336 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,17 @@ 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)} } - return disks + return diskSpecs } // RoleAssignmentSpecs returns the role assignment specs. diff --git a/azure/services/disks/client.go b/azure/services/disks/client.go index 9e5d725f556..ee78c373cd5 100644 --- a/azure/services/disks/client.go +++ b/azure/services/disks/client.go @@ -21,48 +21,80 @@ 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") - defer done() +// 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, span := tele.Tracer().Start(ctx, "disks.AzureClient.Delete") + defer span.End() - 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 +} + +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, span := tele.Tracer().Start(ctx, "disks.AzureClient.IsDone") + defer span.End() + + done, err := future.DoneWithContext(ctx, ac.disks) + if err != nil { + return false, errors.Wrap(err, "failed checking if the operation was complete") + } + + return done, nil } diff --git a/azure/services/disks/disks.go b/azure/services/disks/disks.go index 9bbe9aa4ab4..4b4a3fc0934 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), } } @@ -57,21 +62,24 @@ func (s *Service) Reconcile(ctx context.Context) error { // Delete deletes the disk associated with a VM. func (s *Service) Delete(ctx context.Context) error { - ctx, _, done := tele.StartSpanWithLogger(ctx, "disks.Service.Delete") - defer done() + ctx, span := tele.Tracer().Start(ctx, "disks.Service.Delete") + defer span.End() + + 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 erros 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/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..b109a5e2009 --- /dev/null +++ b/azure/services/disks/spec.go @@ -0,0 +1,40 @@ +/* +Copyright 2021 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 virtual network peering. +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 virtual network peerings. +func (s *DiskSpec) OwnerResourceName() string { + return "" +} + +// Parameters returns the parameters for the virtual network peering. +func (s *DiskSpec) Parameters(existing interface{}) (interface{}, error) { + return nil, nil +} 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