diff --git a/azure/services/virtualmachines/virtualmachines.go b/azure/services/virtualmachines/virtualmachines.go index f974f98e6da..099588bd695 100644 --- a/azure/services/virtualmachines/virtualmachines.go +++ b/azure/services/virtualmachines/virtualmachines.go @@ -54,6 +54,7 @@ type VMScope interface { // Service provides operations on Azure resources. type Service struct { Scope VMScope + async.AsyncReconciler Client interfacesClient networkinterfaces.Client publicIPsClient publicips.Client @@ -62,12 +63,14 @@ type Service struct { // New creates a new service. func New(scope VMScope) *Service { + Client := NewClient(scope) return &Service{ Scope: scope, - Client: NewClient(scope), + Client: Client, interfacesClient: networkinterfaces.NewClient(scope), publicIPsClient: publicips.NewClient(scope), availabilitySetsClient: availabilitysets.NewClient(scope), + AsyncReconciler: async.New(scope, Client, Client), } } @@ -81,7 +84,7 @@ func (s *Service) Reconcile(ctx context.Context) error { vmSpec := s.Scope.VMSpec() - result, err := async.CreateResource(ctx, s.Scope, s.Client, vmSpec, serviceName) + result, err := s.CreateResource(ctx, vmSpec, serviceName) s.Scope.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, err) if err == nil && result != nil { vm, ok := result.(compute.VirtualMachine) @@ -116,7 +119,7 @@ func (s *Service) Delete(ctx context.Context) error { vmSpec := s.Scope.VMSpec() - err := async.DeleteResource(ctx, s.Scope, s.Client, vmSpec, serviceName) + err := s.DeleteResource(ctx, vmSpec, serviceName) if err != nil { s.Scope.SetVMState(infrav1.Deleting) } else { diff --git a/azure/services/virtualmachines/virtualmachines_test.go b/azure/services/virtualmachines/virtualmachines_test.go index 2054655986c..9166b158f09 100644 --- a/azure/services/virtualmachines/virtualmachines_test.go +++ b/azure/services/virtualmachines/virtualmachines_test.go @@ -18,22 +18,20 @@ package virtualmachines import ( "context" - "fmt" "net/http" "testing" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-04-01/compute" "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network" "github.com/Azure/go-autorest/autorest" - azureautorest "github.com/Azure/go-autorest/autorest/azure" "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" - "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" "k8s.io/klog/v2/klogr" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async" "sigs.k8s.io/cluster-api-provider-azure/azure/services/availabilitysets/mock_availabilitysets" "sigs.k8s.io/cluster-api-provider-azure/azure/services/networkinterfaces/mock_networkinterfaces" "sigs.k8s.io/cluster-api-provider-azure/azure/services/publicips/mock_publicips" @@ -57,6 +55,48 @@ var ( Image: &infrav1.Image{ID: to.StringPtr("fake-image-id")}, BootstrapData: "fake data", } + fakeExistingVM = compute.VirtualMachine{ + ID: to.StringPtr("test-vm-id"), + VirtualMachineProperties: &compute.VirtualMachineProperties{ + ProvisioningState: to.StringPtr("Succeeded"), + NetworkProfile: &compute.NetworkProfile{ + NetworkInterfaces: &[]compute.NetworkInterfaceReference{ + { + ID: to.StringPtr("/subscriptions/123/resourceGroups/test-rg/providers/Microsoft.Network/networkInterfaces/nic-1"), + }, + }, + }, + }, + } + fakeNetworkInterface = network.Interface{ + InterfacePropertiesFormat: &network.InterfacePropertiesFormat{ + IPConfigurations: &[]network.InterfaceIPConfiguration{ + { + InterfaceIPConfigurationPropertiesFormat: &network.InterfaceIPConfigurationPropertiesFormat{ + PrivateIPAddress: to.StringPtr("10.0.0.5"), + PublicIPAddress: &network.PublicIPAddress{ + ID: to.StringPtr("/subscriptions/123/resourceGroups/test-rg/providers/Microsoft.Network/publicIPAddresses/pip-1"), + }, + }, + }, + }, + }, + } + fakePublicIPs = network.PublicIPAddress{ + PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ + IPAddress: to.StringPtr("10.0.0.6"), + }, + } + fakeNodeAddresses = []corev1.NodeAddress{ + { + Type: corev1.NodeInternalIP, + Address: "10.0.0.5", + }, + { + Type: corev1.NodeExternalIP, + Address: "10.0.0.6", + }, + } fakeFuture = infrav1.Future{ Type: infrav1.DeleteFuture, ServiceName: serviceName, @@ -64,81 +104,67 @@ var ( ResourceGroup: "test-group", Data: "eyJtZXRob2QiOiJERUxFVEUiLCJwb2xsaW5nTWV0aG9kIjoiTG9jYXRpb24iLCJscm9TdGF0ZSI6IkluUHJvZ3Jlc3MifQ==", } - internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") - notFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") - errCtxExceeded = errors.New("ctx exceeded") + internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") ) func TestReconcileVM(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder) + expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockAsyncReconcilerMockRecorder) }{ { name: "create vm succeeds", expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockAsyncReconcilerMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) s.VMSpec().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName) - m.CreateOrUpdateAsync(gomockinternal.AContext(), &fakeVMSpec).Return(compute.VirtualMachine{ - ID: to.StringPtr("test-vm-id"), - VirtualMachineProperties: &compute.VirtualMachineProperties{ - ProvisioningState: to.StringPtr("Succeeded"), - NetworkProfile: &compute.NetworkProfile{ - NetworkInterfaces: &[]compute.NetworkInterfaceReference{ - { - ID: to.StringPtr("/subscriptions/123/resourceGroups/test-rg/providers/Microsoft.Network/networkInterfaces/nic-1"), - }, - }, - }, - }, - }, nil, nil) + r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(fakeExistingVM, nil) s.UpdatePutStatus(infrav1.VMRunningCondition, 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{ - InterfacePropertiesFormat: &network.InterfacePropertiesFormat{ - IPConfigurations: &[]network.InterfaceIPConfiguration{ - { - InterfaceIPConfigurationPropertiesFormat: &network.InterfaceIPConfigurationPropertiesFormat{ - PrivateIPAddress: to.StringPtr("10.0.0.5"), - PublicIPAddress: &network.PublicIPAddress{ - ID: to.StringPtr("/subscriptions/123/resourceGroups/test-rg/providers/Microsoft.Network/publicIPAddresses/pip-1"), - }, - }, - }, - }, - }, - }, nil) - mpip.Get(gomockinternal.AContext(), "test-group", "pip-1").Return(network.PublicIPAddress{ - PublicIPAddressPropertiesFormat: &network.PublicIPAddressPropertiesFormat{ - IPAddress: to.StringPtr("10.0.0.6"), - }, - }, nil) - s.SetAddresses([]corev1.NodeAddress{ - { - Type: corev1.NodeInternalIP, - Address: "10.0.0.5", - }, - { - Type: corev1.NodeExternalIP, - Address: "10.0.0.6", - }, - }) + mnic.Get(gomockinternal.AContext(), "test-group", "nic-1").Return(fakeNetworkInterface, nil) + mpip.Get(gomockinternal.AContext(), "test-group", "pip-1").Return(fakePublicIPs, nil) + s.SetAddresses(fakeNodeAddresses) s.SetVMState(infrav1.Succeeded) }, }, { - name: "create vm fails", - expectedError: "failed to create resource test-group/test-vm (service: virtualmachine): #: Internal Server Error: StatusCode=500", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder) { + name: "creating vm fails", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockAsyncReconcilerMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) s.VMSpec().Return(&fakeVMSpec) - 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()))) + r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(nil, internalError) + s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, internalError) + }, + }, + { + name: "create vm succeeds but failed to get network interfaces", + expectedError: "failed to fetch VM addresses: #: Internal Server Error: StatusCode=500", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockAsyncReconcilerMockRecorder) { + s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + s.VMSpec().Return(&fakeVMSpec) + r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(fakeExistingVM, nil) + s.UpdatePutStatus(infrav1.VMRunningCondition, 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{}, internalError) + }, + }, + { + name: "create vm succeeds but failed to get public IPs", + expectedError: "failed to fetch VM addresses: #: Internal Server Error: StatusCode=500", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, mnic *mock_networkinterfaces.MockClientMockRecorder, mpip *mock_publicips.MockClientMockRecorder, r *mock_async.MockAsyncReconcilerMockRecorder) { + s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) + s.VMSpec().Return(&fakeVMSpec) + r.CreateResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(fakeExistingVM, nil) + s.UpdatePutStatus(infrav1.VMRunningCondition, serviceName, nil) + s.SetProviderID("azure://test-vm-id") + s.SetAnnotation("cluster-api-provider-azure", "true") + mnic.Get(gomockinternal.AContext(), "test-group", "nic-1").Return(fakeNetworkInterface, nil) + mpip.Get(gomockinternal.AContext(), "test-group", "pip-1").Return(network.PublicIPAddress{}, internalError) + }, }, } @@ -156,8 +182,9 @@ func TestReconcileVM(t *testing.T) { interfaceMock := mock_networkinterfaces.NewMockClient(mockCtrl) publicIPMock := mock_publicips.NewMockClient(mockCtrl) availabilitySetsMock := mock_availabilitysets.NewMockClient(mockCtrl) + asyncMock := mock_async.NewMockAsyncReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), interfaceMock.EXPECT(), publicIPMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), interfaceMock.EXPECT(), publicIPMock.EXPECT(), asyncMock.EXPECT()) s := &Service{ Scope: scopeMock, @@ -165,6 +192,7 @@ func TestReconcileVM(t *testing.T) { interfacesClient: interfaceMock, publicIPsClient: publicIPMock, availabilitySetsClient: availabilitySetsMock, + AsyncReconciler: asyncMock, } err := s.Reconcile(context.TODO()) @@ -182,79 +210,37 @@ func TestDeleteVM(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) + expect func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockAsyncReconcilerMockRecorder) }{ - { - name: "long running delete operation is done", - expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { - s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Times(2).Return(&fakeFuture) - m.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(true, nil) - m.Result(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{}), infrav1.DeleteFuture).Return(nil, nil) - s.DeleteLongRunningOperationState("test-vm", serviceName) - s.SetVMState(infrav1.Deleted) - s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, nil) - }, - }, - { - name: "long running delete operation is not done", - expectedError: "operation type DELETE on Azure resource test-group/test-vm is not done. Object will be requeued after 15s", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { - s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Times(2).Return(&fakeFuture) - m.IsDone(gomockinternal.AContext(), gomock.AssignableToTypeOf(&azureautorest.Future{})).Return(false, nil) - s.SetVMState(infrav1.Deleting) - s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, gomockinternal.ErrStrEq("operation type DELETE on Azure resource test-group/test-vm is not done. Object will be requeued after 15s")) - }, - }, { name: "vm doesn't exist", expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockAsyncReconcilerMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName) - m.DeleteAsync(gomockinternal.AContext(), &fakeVMSpec).Return(nil, notFoundError) + r.DeleteResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(nil) s.SetVMState(infrav1.Deleted) s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, nil) }, }, { name: "error occurs when deleting vm", - expectedError: "failed to delete resource test-group/test-vm (service: virtualmachine): #: Internal Server Error: StatusCode=500", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { - s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakeVMSpec).Return(nil, internalError) - s.SetVMState(infrav1.Deleting) - s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, gomockinternal.ErrStrEq("failed to delete resource test-group/test-vm (service: virtualmachine): #: Internal Server Error: StatusCode=500")) - }, - }, - { - name: "context deadline exceeded while deleting vm", - expectedError: "operation type DELETE on Azure resource test-group/test-vm is not done. Object will be requeued after 15s", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockAsyncReconcilerMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakeVMSpec).Return(&azureautorest.Future{}, errCtxExceeded) - s.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})) + r.DeleteResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(internalError) s.SetVMState(infrav1.Deleting) - s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, gomockinternal.ErrStrEq("operation type DELETE on Azure resource test-group/test-vm is not done. Object will be requeued after 15s")) + s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, internalError) }, }, { name: "delete the vm successfully", expectedError: "", - expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, m *mock_virtualmachines.MockClientMockRecorder) { + expect: func(s *mock_virtualmachines.MockVMScopeMockRecorder, r *mock_async.MockAsyncReconcilerMockRecorder) { s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) s.VMSpec().AnyTimes().Return(&fakeVMSpec) - s.GetLongRunningOperationState("test-vm", serviceName).Return(nil) - m.DeleteAsync(gomockinternal.AContext(), &fakeVMSpec).Return(nil, nil) + r.DeleteResource(gomockinternal.AContext(), &fakeVMSpec, serviceName).Return(nil) s.SetVMState(infrav1.Deleted) s.UpdateDeleteStatus(infrav1.VMRunningCondition, serviceName, nil) }, @@ -270,12 +256,14 @@ func TestDeleteVM(t *testing.T) { defer mockCtrl.Finish() scopeMock := mock_virtualmachines.NewMockVMScope(mockCtrl) clientMock := mock_virtualmachines.NewMockClient(mockCtrl) + asyncMock := mock_async.NewMockAsyncReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), asyncMock.EXPECT()) s := &Service{ - Scope: scopeMock, - Client: clientMock, + Scope: scopeMock, + Client: clientMock, + AsyncReconciler: asyncMock, } err := s.Delete(context.TODO())