diff --git a/azure/services/availabilitysets/availabilitysets.go b/azure/services/availabilitysets/availabilitysets.go index 71b6a2a4f86..7c080415d6c 100644 --- a/azure/services/availabilitysets/availabilitysets.go +++ b/azure/services/availabilitysets/availabilitysets.go @@ -69,6 +69,9 @@ func (s *Service) Reconcile(ctx context.Context) error { if err != nil { s.Scope.UpdatePutStatus(infrav1.AvailabilitySetReadyCondition, serviceName, err) return err + } else if setSpec == nil { + s.Scope.UpdatePutStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil) + return nil } _, err = s.CreateResource(ctx, setSpec, serviceName) @@ -88,28 +91,39 @@ func (s *Service) Delete(ctx context.Context) error { if err != nil { s.Scope.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, err) return err + } else if setSpec == nil { + 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 } - return errors.Wrapf(err, "failed to get availability set %s in resource group %s", setSpec.ResourceName(), setSpec.ResourceGroupName()) + 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 { - return errors.Errorf("%T is not a compute.AvailabilitySet", existingSet) + 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", "availability set", setSpec.ResourceName(), "when it has VMs") + 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.VMRunningCondition, serviceName, err) + s.Scope.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, err) return err } diff --git a/azure/services/availabilitysets/availabilitysets_test.go b/azure/services/availabilitysets/availabilitysets_test.go index dbcec5d1cc9..931dade8483 100644 --- a/azure/services/availabilitysets/availabilitysets_test.go +++ b/azure/services/availabilitysets/availabilitysets_test.go @@ -18,16 +18,18 @@ package availabilitysets 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/compute/mgmt/2021-04-01/compute" "github.com/Azure/go-autorest/autorest" + "github.com/Azure/go-autorest/autorest/to" + // "github.com/Azure/go-autorest/autorest/to" "github.com/golang/mock/gomock" . "github.com/onsi/gomega" 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" gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) @@ -41,140 +43,104 @@ var ( FaultDomainCount: 3, AdditionalTags: map[string]string{}, } - internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") - notFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") - // fakeSpecError = errors.New("Error getting availability set spec") + internalError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error") + notFoundError = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not Found") + fakeSetWithVMs = compute.AvailabilitySet{ + AvailabilitySetProperties: &compute.AvailabilitySetProperties{ + VirtualMachines: &[]compute.SubResource{ + {ID: to.StringPtr("vm-id")}, + }, + }, + } ) -// func TestReconcileAvailabilitySets(t *testing.T) { -// testcases := []struct { -// name string -// expectedError string -// expect func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) -// setupSKUs func(svc *Service) -// }{ -// { -// name: "create or update availability set", -// expectedError: "", -// expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) { -// s.V(gomock.AssignableToTypeOf(2)).MinTimes(2).Return(klogr.New()) -// s.AvailabilitySet().Return("as-name", true) -// s.ResourceGroup().Return("my-rg") -// s.ClusterName().Return("cl-name") -// s.AdditionalTags().Return(map[string]string{}) -// s.Location().Return("test-location") -// m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "as-name", -// compute.AvailabilitySet{ -// Sku: &compute.Sku{Name: to.StringPtr("Aligned")}, -// AvailabilitySetProperties: &compute.AvailabilitySetProperties{ -// PlatformFaultDomainCount: pointer.Int32Ptr(3), -// }, -// Tags: map[string]*string{"sigs.k8s.io_cluster-api-provider-azure_cluster_cl-name": to.StringPtr("owned"), -// "sigs.k8s.io_cluster-api-provider-azure_role": to.StringPtr("common"), "Name": to.StringPtr("as-name")}, -// Location: to.StringPtr("test-location"), -// }).Return(compute.AvailabilitySet{}, nil) -// }, -// setupSKUs: func(svc *Service) { -// skus := []compute.ResourceSku{ -// { -// Name: to.StringPtr("Aligned"), -// Kind: to.StringPtr(string(resourceskus.AvailabilitySets)), -// Capabilities: &[]compute.ResourceSkuCapabilities{ -// { -// Name: to.StringPtr(resourceskus.MaximumPlatformFaultDomainCount), -// Value: to.StringPtr("3"), -// }, -// }, -// }, -// } -// resourceSkusCache := resourceskus.NewStaticCache(skus, "") -// svc.resourceSKUCache = resourceSkusCache -// }, -// }, -// { -// name: "noop if the machine does not need to be assigned an availability set (machines without a deployment)", -// expectedError: "", -// expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) { -// s.AvailabilitySet().Return("as-name", false) -// }, -// setupSKUs: func(svc *Service) { -// }, -// }, -// { -// name: "return error", -// expectedError: "failed to create availability set as-name: something went wrong", -// expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) { -// s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) -// s.AvailabilitySet().Return("as-name", true) -// s.ResourceGroup().Return("my-rg") -// s.ClusterName().Return("cl-name") -// s.AdditionalTags().Return(map[string]string{}) -// s.Location().Return("test-location") -// m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "as-name", -// gomock.AssignableToTypeOf(compute.AvailabilitySet{})).Return(compute.AvailabilitySet{}, errors.New("something went wrong")) -// }, -// setupSKUs: func(svc *Service) { -// skus := []compute.ResourceSku{ -// { -// Name: to.StringPtr("Aligned"), -// Kind: to.StringPtr(string(resourceskus.AvailabilitySets)), -// Capabilities: &[]compute.ResourceSkuCapabilities{ -// { -// Name: to.StringPtr(resourceskus.MaximumPlatformFaultDomainCount), -// Value: to.StringPtr("3"), -// }, -// }, -// }, -// } -// resourceSkusCache := resourceskus.NewStaticCache(skus, "") -// svc.resourceSKUCache = resourceSkusCache -// }, -// }, -// } -// for _, tc := range testcases { -// tc := tc -// t.Run(tc.name, func(t *testing.T) { -// g := NewWithT(t) +func TestReconcileAvailabilitySets(t *testing.T) { + testcases := []struct { + name string + expectedError string + expect func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) + // setupSKUs func(svc *Service) + }{ + { + name: "create or update availability set", + expectedError: "", + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.AvailabilitySetSpec().Return(&fakeSetSpec, nil) + r.CreateResource(gomockinternal.AContext(), &fakeSetSpec, serviceName).Return(nil, nil) + s.UpdatePutStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil) + }, + }, + { + name: "noop if availability set is not enabled or has no name", + expectedError: "", + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.AvailabilitySetSpec().Return(nil, nil) + s.UpdatePutStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil) + }, + }, + { + name: "error in getting availability set spec", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.AvailabilitySetSpec().Return(nil, internalError) + s.UpdatePutStatus(infrav1.AvailabilitySetReadyCondition, serviceName, internalError) + }, + }, + { + name: "error in creating availability set", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.AvailabilitySetSpec().Return(&fakeSetSpec, nil) + r.CreateResource(gomockinternal.AContext(), &fakeSetSpec, serviceName).Return(nil, internalError) + s.UpdatePutStatus(infrav1.AvailabilitySetReadyCondition, serviceName, internalError) + }, + }, + } + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) -// t.Parallel() -// mockCtrl := gomock.NewController(t) -// defer mockCtrl.Finish() -// scopeMock := mock_availabilitysets.NewMockAvailabilitySetScope(mockCtrl) -// clientMock := mock_availabilitysets.NewMockClient(mockCtrl) + t.Parallel() + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + scopeMock := mock_availabilitysets.NewMockAvailabilitySetScope(mockCtrl) + clientMock := mock_availabilitysets.NewMockClient(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) -// tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), asyncMock.EXPECT()) -// s := &Service{ -// Scope: scopeMock, -// Client: clientMock, -// } -// tc.setupSKUs(s) + s := &Service{ + Scope: scopeMock, + Client: clientMock, + Reconciler: asyncMock, + } -// err := s.Reconcile(context.TODO()) -// if tc.expectedError != "" { -// g.Expect(err).To(HaveOccurred()) -// g.Expect(err).To(MatchError(tc.expectedError)) -// } else { -// g.Expect(err).NotTo(HaveOccurred()) -// } -// }) -// } -// } + err := s.Reconcile(context.TODO()) + if tc.expectedError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(tc.expectedError)) + } else { + g.Expect(err).NotTo(HaveOccurred()) + } + }) + } +} func TestDeleteAvailabilitySets(t *testing.T) { testcases := []struct { name string expectedError string - expect func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) + expect func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) }{ { name: "deletes availability set", expectedError: "", - expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) { + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.AvailabilitySetSpec().Return(&fakeSetSpec, nil) gomock.InOrder( - s.GetLongRunningOperationState("test-as", serviceName), - m.DeleteAsync(gomockinternal.AContext(), &fakeSetSpec).Return(nil, nil), + m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(compute.AvailabilitySet{}, nil), + r.DeleteResource(gomockinternal.AContext(), &fakeSetSpec, serviceName).Return(nil), s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil), ) }, @@ -182,49 +148,72 @@ func TestDeleteAvailabilitySets(t *testing.T) { { name: "noop if AvailabilitySetSpec returns nil", expectedError: "", - expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) { + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.AvailabilitySetSpec().Return(nil, nil) s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil) }, }, - // { - // name: "noop if availability set has vms", - // expectedError: "", - // expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) { - // s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New()) - // s.AvailabilitySetSpec().Return(&fakeSetSpec, nil) - // gomock.InOrder( - // s.GetLongRunningOperationState("test-as", serviceName), - // m.DeleteAsync(gomockinternal.AContext(), &fakeSetSpec).Return(nil, nil), - // s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil), - // ) - - // // m.Get(gomockinternal.AContext(), "my-rg", "as-name").Return(compute.AvailabilitySet{ - // // AvailabilitySetProperties: &compute.AvailabilitySetProperties{VirtualMachines: &[]compute.SubResource{ - // // {ID: to.StringPtr("vm-id")}}}}, nil) - // }, - // }, { - name: "noop if availability set is already deleted - delete returns 404", + name: "error in getting availability set spec", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.AvailabilitySetSpec().Return(nil, internalError) + s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, internalError) + }, + }, + { + name: "noop if availability set has vms", expectedError: "", - expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) { + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.AvailabilitySetSpec().Return(&fakeSetSpec, nil) gomock.InOrder( - s.GetLongRunningOperationState("test-as", serviceName), - m.DeleteAsync(gomockinternal.AContext(), &fakeSetSpec).Return(nil, notFoundError), + m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(fakeSetWithVMs, nil), s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil), ) }, }, { - name: "returns error when availability set delete fails", - expectedError: "failed to delete resource test-rg/test-as (service: availabilitysets): #: Internal Server Error: StatusCode=500", - expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder) { + name: "availability set not found", + expectedError: "", + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.AvailabilitySetSpec().Return(&fakeSetSpec, nil) + gomock.InOrder( + m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(nil, notFoundError), + s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, nil), + ) + }, + }, + { + name: "error in getting availability set", + expectedError: "failed to get availability set test-as in resource group test-rg: #: Internal Server Error: StatusCode=500", + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.AvailabilitySetSpec().Return(&fakeSetSpec, nil) + gomock.InOrder( + m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(nil, internalError), + s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, gomockinternal.ErrStrEq("failed to get availability set test-as in resource group test-rg: #: Internal Server Error: StatusCode=500")), + ) + }, + }, + { + name: "availability set get result is not an availability set", + expectedError: "string is not a compute.AvailabilitySet", + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { + s.AvailabilitySetSpec().Return(&fakeSetSpec, nil) + gomock.InOrder( + m.Get(gomockinternal.AContext(), &fakeSetSpec).Return("not an availability set", nil), + s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, gomockinternal.ErrStrEq("string is not a compute.AvailabilitySet")), + ) + }, + }, + { + name: "error in deleting availability set", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(s *mock_availabilitysets.MockAvailabilitySetScopeMockRecorder, m *mock_availabilitysets.MockClientMockRecorder, r *mock_async.MockReconcilerMockRecorder) { s.AvailabilitySetSpec().Return(&fakeSetSpec, nil) gomock.InOrder( - s.GetLongRunningOperationState("test-as", serviceName), - m.DeleteAsync(gomockinternal.AContext(), &fakeSetSpec).Return(nil, internalError), - s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, gomockinternal.ErrStrEq(fmt.Sprintf("failed to delete resource test-rg/test-as (service: availabilitysets): %s", internalError.Error()))), + m.Get(gomockinternal.AContext(), &fakeSetSpec).Return(compute.AvailabilitySet{}, nil), + r.DeleteResource(gomockinternal.AContext(), &fakeSetSpec, serviceName).Return(internalError), + s.UpdateDeleteStatus(infrav1.AvailabilitySetReadyCondition, serviceName, internalError), ) }, }, @@ -239,12 +228,14 @@ func TestDeleteAvailabilitySets(t *testing.T) { defer mockCtrl.Finish() scopeMock := mock_availabilitysets.NewMockAvailabilitySetScope(mockCtrl) clientMock := mock_availabilitysets.NewMockClient(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) - tc.expect(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(scopeMock.EXPECT(), clientMock.EXPECT(), asyncMock.EXPECT()) s := &Service{ - Scope: scopeMock, - Client: clientMock, + Scope: scopeMock, + Client: clientMock, + Reconciler: asyncMock, } err := s.Delete(context.TODO()) diff --git a/azure/services/availabilitysets/spec.go b/azure/services/availabilitysets/spec.go index 3066dc9f734..0240fc3dc97 100644 --- a/azure/services/availabilitysets/spec.go +++ b/azure/services/availabilitysets/spec.go @@ -59,7 +59,7 @@ func (s *AvailabilitySetSpec) Parameters(existing interface{}) (interface{}, err return nil, nil } - asParams := compute.AvailabilitySet{ + asParams := &compute.AvailabilitySet{ Sku: &compute.Sku{ Name: to.StringPtr(string(compute.AvailabilitySetSkuTypesAligned)), },