diff --git a/azure/scope/machinepoolmachine.go b/azure/scope/machinepoolmachine.go index a9808a56d75..918004e13cf 100644 --- a/azure/scope/machinepoolmachine.go +++ b/azure/scope/machinepoolmachine.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "reflect" + "strings" "time" "github.com/pkg/errors" @@ -32,7 +33,9 @@ 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/converters" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesetvms" infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" + azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure" "sigs.k8s.io/cluster-api-provider-azure/util/futures" "sigs.k8s.io/cluster-api-provider-azure/util/tele" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -149,6 +152,24 @@ func NewMachinePoolMachineScope(params MachinePoolMachineScopeParams) (*MachineP }, nil } +// ScaleSetVMSpec returns the VMSS VM spec. +func (s *MachinePoolMachineScope) ScaleSetVMSpec() azure.ResourceSpecGetter { + spec := &scalesetvms.ScaleSetVMSpec{ + Name: s.Name(), + InstanceID: s.InstanceID(), + ResourceGroup: s.ResourceGroup(), + ScaleSetName: s.ScaleSetName(), + ProviderID: s.ProviderID(), + IsFlex: s.OrchestrationMode() == infrav1.FlexibleOrchestrationMode, + } + + if spec.IsFlex { + spec.ResourceID = strings.TrimPrefix(spec.ProviderID, azureutil.ProviderIDPrefix) + } + + return spec +} + // Name is the name of the Machine Pool Machine. func (s *MachinePoolMachineScope) Name() string { return s.AzureMachinePoolMachine.Name @@ -226,6 +247,13 @@ func (s *MachinePoolMachineScope) SetVMSSVM(instance *azure.VMSSVM) { s.instance = instance } +// SetVMSSVMState update the scope with the current provisioning state of the VMSS VM. +func (s *MachinePoolMachineScope) SetVMSSVMState(state infrav1.ProvisioningState) { + if s.instance != nil { + s.instance.State = state + } +} + // ProvisioningState returns the AzureMachinePoolMachine provisioning state. func (s *MachinePoolMachineScope) ProvisioningState() infrav1.ProvisioningState { if s.AzureMachinePoolMachine.Status.ProvisioningState != nil { diff --git a/azure/scope/machinepoolmachine_test.go b/azure/scope/machinepoolmachine_test.go index 464072c0535..1773f422025 100644 --- a/azure/scope/machinepoolmachine_test.go +++ b/azure/scope/machinepoolmachine_test.go @@ -18,8 +18,10 @@ package scope import ( "context" + "reflect" "testing" + "github.com/google/go-cmp/cmp" . "github.com/onsi/gomega" "github.com/pkg/errors" "go.uber.org/mock/gomock" @@ -27,9 +29,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/ptr" + 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/mock_azure" mock_scope "sigs.k8s.io/cluster-api-provider-azure/azure/scope/mocks" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesetvms" infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" gomock2 "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -134,6 +138,124 @@ func TestNewMachinePoolMachineScope(t *testing.T) { } } +func TestMachinePoolMachineScope_ScaleSetVMSpecs(t *testing.T) { + tests := []struct { + name string + machinePoolMachineScope MachinePoolMachineScope + want azure.ResourceSpecGetter + }{ + { + name: "return vmss vm spec for uniform vmss", + machinePoolMachineScope: MachinePoolMachineScope{ + MachinePool: &expv1.MachinePool{}, + AzureMachinePool: &infrav1exp.AzureMachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool-name", + }, + Spec: infrav1exp.AzureMachinePoolSpec{ + Template: infrav1exp.AzureMachinePoolMachineTemplate{ + OSDisk: infrav1.OSDisk{ + OSType: "Linux", + }, + }, + OrchestrationMode: infrav1.UniformOrchestrationMode, + }, + }, + AzureMachinePoolMachine: &infrav1exp.AzureMachinePoolMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepoolmachine-name", + }, + Spec: infrav1exp.AzureMachinePoolMachineSpec{ + ProviderID: "azure:///subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachineScaleSets/machinepool-name/virtualMachines/0", + InstanceID: "0", + }, + }, + ClusterScoper: &ClusterScope{ + AzureCluster: &infrav1.AzureCluster{ + Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", + }, + }, + }, + MachinePoolScope: &MachinePoolScope{ + AzureMachinePool: &infrav1exp.AzureMachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool-name", + }, + }, + }, + }, + want: &scalesetvms.ScaleSetVMSpec{ + Name: "machinepoolmachine-name", + InstanceID: "0", + ResourceGroup: "my-rg", + ScaleSetName: "machinepool-name", + ProviderID: "azure:///subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachineScaleSets/machinepool-name/virtualMachines/0", + IsFlex: false, + ResourceID: "", + }, + }, + { + name: "return vmss vm spec for vmss flex", + machinePoolMachineScope: MachinePoolMachineScope{ + MachinePool: &expv1.MachinePool{}, + AzureMachinePool: &infrav1exp.AzureMachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool-name", + }, + Spec: infrav1exp.AzureMachinePoolSpec{ + Template: infrav1exp.AzureMachinePoolMachineTemplate{ + OSDisk: infrav1.OSDisk{ + OSType: "Linux", + }, + }, + OrchestrationMode: infrav1.FlexibleOrchestrationMode, + }, + }, + AzureMachinePoolMachine: &infrav1exp.AzureMachinePoolMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepoolmachine-name", + }, + Spec: infrav1exp.AzureMachinePoolMachineSpec{ + ProviderID: "azure:///subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachineScaleSets/machinepool-name/virtualMachines/0", + InstanceID: "0", + }, + }, + ClusterScoper: &ClusterScope{ + AzureCluster: &infrav1.AzureCluster{ + Spec: infrav1.AzureClusterSpec{ + ResourceGroup: "my-rg", + }, + }, + }, + MachinePoolScope: &MachinePoolScope{ + AzureMachinePool: &infrav1exp.AzureMachinePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machinepool-name", + }, + }, + }, + }, + want: &scalesetvms.ScaleSetVMSpec{ + Name: "machinepoolmachine-name", + InstanceID: "0", + ResourceGroup: "my-rg", + ScaleSetName: "machinepool-name", + ProviderID: "azure:///subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachineScaleSets/machinepool-name/virtualMachines/0", + IsFlex: true, + ResourceID: "/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachineScaleSets/machinepool-name/virtualMachines/0", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.machinePoolMachineScope.ScaleSetVMSpec(); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Diff between expected result and actual result: %+v", cmp.Diff(tt.want, got)) + } + }) + } +} + func TestMachineScope_UpdateNodeStatus(t *testing.T) { scheme := runtime.NewScheme() _ = expv1.AddToScheme(scheme) diff --git a/azure/services/scalesetvms/client.go b/azure/services/scalesetvms/client.go index 5311e9eef31..f6341d254d2 100644 --- a/azure/services/scalesetvms/client.go +++ b/azure/services/scalesetvms/client.go @@ -18,42 +18,27 @@ package scalesetvms import ( "context" - "encoding/base64" - "encoding/json" - "time" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute" "github.com/Azure/go-autorest/autorest" - "github.com/pkg/errors" + azureautorest "github.com/Azure/go-autorest/autorest/azure" "k8s.io/utils/ptr" - 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/converters" + "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 { - Get(context.Context, string, string, string) (compute.VirtualMachineScaleSetVM, error) - GetResultIfDone(ctx context.Context, future *infrav1.Future) (compute.VirtualMachineScaleSetVM, error) - DeleteAsync(context.Context, string, string, string) (*infrav1.Future, error) + Get(context.Context, azure.ResourceSpecGetter) (interface{}, error) + DeleteAsync(context.Context, azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) + IsDone(ctx context.Context, future azureautorest.FutureAPI) (isDone bool, err error) } -type ( - // azureClient contains the Azure go-sdk Client. - azureClient struct { - scalesetvms compute.VirtualMachineScaleSetVMsClient - } - - genericScaleSetVMFuture interface { - DoneWithContext(ctx context.Context, sender autorest.Sender) (done bool, err error) - Result(client compute.VirtualMachineScaleSetVMsClient) (vmss compute.VirtualMachineScaleSetVM, err error) - } - - deleteFutureAdapter struct { - compute.VirtualMachineScaleSetVMsDeleteFuture - } -) +// azureClient contains the Azure go-sdk Client. +type azureClient struct { + scalesetvms compute.VirtualMachineScaleSetVMsClient +} var _ client = &azureClient{} @@ -74,79 +59,56 @@ func newVirtualMachineScaleSetVMsClient(subscriptionID string, baseURI string, a } // Get retrieves the Virtual Machine Scale Set Virtual Machine. -func (ac *azureClient) Get(ctx context.Context, resourceGroupName, vmssName, instanceID string) (compute.VirtualMachineScaleSetVM, error) { +func (ac *azureClient) Get(ctx context.Context, spec azure.ResourceSpecGetter) (interface{}, error) { ctx, _, done := tele.StartSpanWithLogger(ctx, "scalesetvms.azureClient.Get") defer done() - return ac.scalesetvms.Get(ctx, resourceGroupName, vmssName, instanceID, "") + return ac.scalesetvms.Get(ctx, spec.ResourceGroupName(), spec.OwnerResourceName(), spec.ResourceName(), "") } -// GetResultIfDone fetches the result of a long-running operation future if it is done. -func (ac *azureClient) GetResultIfDone(ctx context.Context, future *infrav1.Future) (compute.VirtualMachineScaleSetVM, error) { - ctx, _, spanDone := tele.StartSpanWithLogger(ctx, "scalesetvms.azureClient.GetResultIfDone") - defer spanDone() +// CreateOrUpdateAsync is a dummy implementation to fulfill the async.Reconciler interface. +func (ac *azureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, parameters interface{}) (result interface{}, future azureautorest.FutureAPI, err error) { + _, _, done := tele.StartSpanWithLogger(ctx, "scalesets.AzureClient.CreateOrUpdateAsync") + defer done() - var genericFuture genericScaleSetVMFuture - futureData, err := base64.URLEncoding.DecodeString(future.Data) - if err != nil { - return compute.VirtualMachineScaleSetVM{}, errors.Wrapf(err, "failed to base64 decode future data") - } + return nil, nil, nil +} - switch future.Type { - case infrav1.DeleteFuture: - var future compute.VirtualMachineScaleSetVMsDeleteFuture - if err := json.Unmarshal(futureData, &future); err != nil { - return compute.VirtualMachineScaleSetVM{}, errors.Wrap(err, "failed to unmarshal future data") - } - - genericFuture = &deleteFutureAdapter{ - VirtualMachineScaleSetVMsDeleteFuture: future, - } - default: - return compute.VirtualMachineScaleSetVM{}, errors.Errorf("unknown future type %q", future.Type) - } +// DeleteAsync deletes a virtual machine scale set instance 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) (future azureautorest.FutureAPI, err error) { + ctx, _, done := tele.StartSpanWithLogger(ctx, "scalesetvms.AzureClient.DeleteAsync") + defer done() - done, err := genericFuture.DoneWithContext(ctx, ac.scalesetvms) + deleteFuture, err := ac.scalesetvms.Delete(ctx, spec.ResourceGroupName(), spec.OwnerResourceName(), spec.ResourceName(), ptr.To(false)) if err != nil { - return compute.VirtualMachineScaleSetVM{}, errors.Wrapf(err, "failed checking if the operation was complete") + return nil, err } - if !done { - return compute.VirtualMachineScaleSetVM{}, azure.WithTransientError(azure.NewOperationNotDoneError(future), 15*time.Second) - } + ctx, cancel := context.WithTimeout(ctx, reconciler.DefaultAzureCallTimeout) + defer cancel() - vm, err := genericFuture.Result(ac.scalesetvms) + err = deleteFuture.WaitForCompletionRef(ctx, ac.scalesetvms.Client) if err != nil { - return vm, errors.Wrapf(err, "failed fetching the result of operation for vmss") + // if an error occurs, return the future. + // this means the long-running operation didn't finish in the specified timeout. + return &deleteFuture, err } + _, err = deleteFuture.Result(ac.scalesetvms) + // if the operation completed, return a nil future. + return nil, err +} - return vm, nil +// Result fetches the result of a long-running operation future. +func (ac *azureClient) Result(ctx context.Context, future azureautorest.FutureAPI, futureType string) (result interface{}, err error) { + return nil, nil } -// DeleteAsync is the operation to delete a virtual machine scale set instance 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. -// -// Parameters: -// -// resourceGroupName - the name of the resource group. -// vmssName - the name of the VM scale set to create or update. parameters - the scale set object. -// instanceID - the ID of the VM scale set VM. -func (ac *azureClient) DeleteAsync(ctx context.Context, resourceGroupName, vmssName, instanceID string) (*infrav1.Future, error) { - ctx, _, done := tele.StartSpanWithLogger(ctx, "scalesetvms.azureClient.DeleteAsync") +// 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, "scalesetvms.AzureClient.IsDone") defer done() - future, err := ac.scalesetvms.Delete(ctx, resourceGroupName, vmssName, instanceID, ptr.To(false)) - if err != nil { - return nil, errors.Wrapf(err, "failed deleting vmss named %q", vmssName) - } - - return converters.SDKToFuture(&future, infrav1.DeleteFuture, serviceName, instanceID, resourceGroupName) -} - -// Result wraps the delete result so that we can treat it generically. The only thing we care about is if the delete -// was successful. If it wasn't, an error will be returned. -func (da *deleteFutureAdapter) Result(client compute.VirtualMachineScaleSetVMsClient) (compute.VirtualMachineScaleSetVM, error) { - _, err := da.VirtualMachineScaleSetVMsDeleteFuture.Result(client) - return compute.VirtualMachineScaleSetVM{}, err + return future.DoneWithContext(ctx, ac.scalesetvms) } diff --git a/azure/services/scalesetvms/mock_scalesetvms/client_mock.go b/azure/services/scalesetvms/mock_scalesetvms/client_mock.go index b2ffd5b5702..02648449b20 100644 --- a/azure/services/scalesetvms/mock_scalesetvms/client_mock.go +++ b/azure/services/scalesetvms/mock_scalesetvms/client_mock.go @@ -24,10 +24,9 @@ import ( context "context" reflect "reflect" - compute "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute" - autorest "github.com/Azure/go-autorest/autorest" + azure "github.com/Azure/go-autorest/autorest/azure" gomock "go.uber.org/mock/gomock" - v1beta1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" + azure0 "sigs.k8s.io/cluster-api-provider-azure/azure" ) // Mockclient is a mock of client interface. @@ -54,99 +53,46 @@ func (m *Mockclient) EXPECT() *MockclientMockRecorder { } // DeleteAsync mocks base method. -func (m *Mockclient) DeleteAsync(arg0 context.Context, arg1, arg2, arg3 string) (*v1beta1.Future, error) { +func (m *Mockclient) DeleteAsync(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (azure.FutureAPI, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteAsync", arg0, arg1, arg2, arg3) - ret0, _ := ret[0].(*v1beta1.Future) + ret := m.ctrl.Call(m, "DeleteAsync", arg0, arg1) + ret0, _ := ret[0].(azure.FutureAPI) ret1, _ := ret[1].(error) return ret0, ret1 } // DeleteAsync indicates an expected call of DeleteAsync. -func (mr *MockclientMockRecorder) DeleteAsync(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { +func (mr *MockclientMockRecorder) DeleteAsync(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAsync", reflect.TypeOf((*Mockclient)(nil).DeleteAsync), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAsync", reflect.TypeOf((*Mockclient)(nil).DeleteAsync), arg0, arg1) } // Get mocks base method. -func (m *Mockclient) Get(arg0 context.Context, arg1, arg2, arg3 string) (compute.VirtualMachineScaleSetVM, error) { +func (m *Mockclient) Get(arg0 context.Context, arg1 azure0.ResourceSpecGetter) (interface{}, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2, arg3) - ret0, _ := ret[0].(compute.VirtualMachineScaleSetVM) + ret := m.ctrl.Call(m, "Get", arg0, arg1) + ret0, _ := ret[0].(interface{}) ret1, _ := ret[1].(error) return ret0, ret1 } // Get indicates an expected call of Get. -func (mr *MockclientMockRecorder) Get(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { +func (mr *MockclientMockRecorder) Get(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*Mockclient)(nil).Get), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*Mockclient)(nil).Get), arg0, arg1) } -// GetResultIfDone mocks base method. -func (m *Mockclient) GetResultIfDone(ctx context.Context, future *v1beta1.Future) (compute.VirtualMachineScaleSetVM, error) { +// IsDone mocks base method. +func (m *Mockclient) IsDone(ctx context.Context, future azure.FutureAPI) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetResultIfDone", ctx, future) - ret0, _ := ret[0].(compute.VirtualMachineScaleSetVM) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetResultIfDone indicates an expected call of GetResultIfDone. -func (mr *MockclientMockRecorder) GetResultIfDone(ctx, future interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetResultIfDone", reflect.TypeOf((*Mockclient)(nil).GetResultIfDone), ctx, future) -} - -// MockgenericScaleSetVMFuture is a mock of genericScaleSetVMFuture interface. -type MockgenericScaleSetVMFuture struct { - ctrl *gomock.Controller - recorder *MockgenericScaleSetVMFutureMockRecorder -} - -// MockgenericScaleSetVMFutureMockRecorder is the mock recorder for MockgenericScaleSetVMFuture. -type MockgenericScaleSetVMFutureMockRecorder struct { - mock *MockgenericScaleSetVMFuture -} - -// NewMockgenericScaleSetVMFuture creates a new mock instance. -func NewMockgenericScaleSetVMFuture(ctrl *gomock.Controller) *MockgenericScaleSetVMFuture { - mock := &MockgenericScaleSetVMFuture{ctrl: ctrl} - mock.recorder = &MockgenericScaleSetVMFutureMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockgenericScaleSetVMFuture) EXPECT() *MockgenericScaleSetVMFutureMockRecorder { - return m.recorder -} - -// DoneWithContext mocks base method. -func (m *MockgenericScaleSetVMFuture) DoneWithContext(ctx context.Context, sender autorest.Sender) (bool, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DoneWithContext", ctx, sender) + ret := m.ctrl.Call(m, "IsDone", ctx, future) ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } -// DoneWithContext indicates an expected call of DoneWithContext. -func (mr *MockgenericScaleSetVMFutureMockRecorder) DoneWithContext(ctx, sender interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DoneWithContext", reflect.TypeOf((*MockgenericScaleSetVMFuture)(nil).DoneWithContext), ctx, sender) -} - -// Result mocks base method. -func (m *MockgenericScaleSetVMFuture) Result(client compute.VirtualMachineScaleSetVMsClient) (compute.VirtualMachineScaleSetVM, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Result", client) - ret0, _ := ret[0].(compute.VirtualMachineScaleSetVM) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Result indicates an expected call of Result. -func (mr *MockgenericScaleSetVMFutureMockRecorder) Result(client interface{}) *gomock.Call { +// IsDone indicates an expected call of IsDone. +func (mr *MockclientMockRecorder) IsDone(ctx, future interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Result", reflect.TypeOf((*MockgenericScaleSetVMFuture)(nil).Result), client) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsDone", reflect.TypeOf((*Mockclient)(nil).IsDone), ctx, future) } diff --git a/azure/services/scalesetvms/mock_scalesetvms/scalesetvms_mock.go b/azure/services/scalesetvms/mock_scalesetvms/scalesetvms_mock.go index 4b016be599e..0db1e2de147 100644 --- a/azure/services/scalesetvms/mock_scalesetvms/scalesetvms_mock.go +++ b/azure/services/scalesetvms/mock_scalesetvms/scalesetvms_mock.go @@ -275,20 +275,6 @@ func (mr *MockScaleSetVMScopeMockRecorder) HashKey() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HashKey", reflect.TypeOf((*MockScaleSetVMScope)(nil).HashKey)) } -// InstanceID mocks base method. -func (m *MockScaleSetVMScope) InstanceID() string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "InstanceID") - ret0, _ := ret[0].(string) - return ret0 -} - -// InstanceID indicates an expected call of InstanceID. -func (mr *MockScaleSetVMScopeMockRecorder) InstanceID() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InstanceID", reflect.TypeOf((*MockScaleSetVMScope)(nil).InstanceID)) -} - // Location mocks base method. func (m *MockScaleSetVMScope) Location() string { m.ctrl.T.Helper() @@ -303,34 +289,6 @@ func (mr *MockScaleSetVMScopeMockRecorder) Location() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Location", reflect.TypeOf((*MockScaleSetVMScope)(nil).Location)) } -// OrchestrationMode mocks base method. -func (m *MockScaleSetVMScope) OrchestrationMode() v1beta1.OrchestrationModeType { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "OrchestrationMode") - ret0, _ := ret[0].(v1beta1.OrchestrationModeType) - return ret0 -} - -// OrchestrationMode indicates an expected call of OrchestrationMode. -func (mr *MockScaleSetVMScopeMockRecorder) OrchestrationMode() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "OrchestrationMode", reflect.TypeOf((*MockScaleSetVMScope)(nil).OrchestrationMode)) -} - -// ProviderID mocks base method. -func (m *MockScaleSetVMScope) ProviderID() string { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ProviderID") - ret0, _ := ret[0].(string) - return ret0 -} - -// ProviderID indicates an expected call of ProviderID. -func (mr *MockScaleSetVMScopeMockRecorder) ProviderID() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ProviderID", reflect.TypeOf((*MockScaleSetVMScope)(nil).ProviderID)) -} - // ResourceGroup mocks base method. func (m *MockScaleSetVMScope) ResourceGroup() string { m.ctrl.T.Helper() @@ -345,18 +303,18 @@ func (mr *MockScaleSetVMScopeMockRecorder) ResourceGroup() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResourceGroup", reflect.TypeOf((*MockScaleSetVMScope)(nil).ResourceGroup)) } -// ScaleSetName mocks base method. -func (m *MockScaleSetVMScope) ScaleSetName() string { +// ScaleSetVMSpec mocks base method. +func (m *MockScaleSetVMScope) ScaleSetVMSpec() azure.ResourceSpecGetter { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ScaleSetName") - ret0, _ := ret[0].(string) + ret := m.ctrl.Call(m, "ScaleSetVMSpec") + ret0, _ := ret[0].(azure.ResourceSpecGetter) return ret0 } -// ScaleSetName indicates an expected call of ScaleSetName. -func (mr *MockScaleSetVMScopeMockRecorder) ScaleSetName() *gomock.Call { +// ScaleSetVMSpec indicates an expected call of ScaleSetVMSpec. +func (mr *MockScaleSetVMScopeMockRecorder) ScaleSetVMSpec() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ScaleSetName", reflect.TypeOf((*MockScaleSetVMScope)(nil).ScaleSetName)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ScaleSetVMSpec", reflect.TypeOf((*MockScaleSetVMScope)(nil).ScaleSetVMSpec)) } // SetLongRunningOperationState mocks base method. @@ -383,6 +341,18 @@ func (mr *MockScaleSetVMScopeMockRecorder) SetVMSSVM(vmssvm interface{}) *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetVMSSVM", reflect.TypeOf((*MockScaleSetVMScope)(nil).SetVMSSVM), vmssvm) } +// SetVMSSVMState mocks base method. +func (m *MockScaleSetVMScope) SetVMSSVMState(state v1beta1.ProvisioningState) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetVMSSVMState", state) +} + +// SetVMSSVMState indicates an expected call of SetVMSSVMState. +func (mr *MockScaleSetVMScopeMockRecorder) SetVMSSVMState(state interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetVMSSVMState", reflect.TypeOf((*MockScaleSetVMScope)(nil).SetVMSSVMState), state) +} + // SubscriptionID mocks base method. func (m *MockScaleSetVMScope) SubscriptionID() string { m.ctrl.T.Helper() diff --git a/azure/services/scalesetvms/scalesetvms.go b/azure/services/scalesetvms/scalesetvms.go index 4ca7ffab084..9efa3a7604f 100644 --- a/azure/services/scalesetvms/scalesetvms.go +++ b/azure/services/scalesetvms/scalesetvms.go @@ -19,14 +19,14 @@ package scalesetvms import ( "context" "fmt" - "strings" "time" - "github.com/go-logr/logr" + "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute" "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/converters" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/async" "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines" azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure" "sigs.k8s.io/cluster-api-provider-azure/util/tele" @@ -39,27 +39,27 @@ type ( ScaleSetVMScope interface { azure.ClusterDescriber azure.AsyncStatusUpdater - InstanceID() string - ProviderID() string - ScaleSetName() string - OrchestrationMode() infrav1.OrchestrationModeType + ScaleSetVMSpec() azure.ResourceSpecGetter SetVMSSVM(vmssvm *azure.VMSSVM) + SetVMSSVMState(state infrav1.ProvisioningState) } // Service provides operations on Azure resources. Service struct { - Client client - VMClient virtualmachines.Client - Scope ScaleSetVMScope + Scope ScaleSetVMScope + async.Reconciler + VMReconciler async.Reconciler } ) // NewService creates a new service. func NewService(scope ScaleSetVMScope) *Service { + client := newClient(scope) + vmClient := virtualmachines.NewClient(scope) return &Service{ - Client: newClient(scope), - VMClient: virtualmachines.NewClient(scope), - Scope: scope, + Reconciler: async.New(scope, client, client), + VMReconciler: async.New(scope, vmClient, vmClient), + Scope: scope, } } @@ -73,211 +73,96 @@ func (s *Service) Reconcile(ctx context.Context) error { ctx, log, done := tele.StartSpanWithLogger(ctx, "scalesetvms.Service.Reconcile") defer done() - var ( - resourceGroup = s.Scope.ResourceGroup() - vmssName = s.Scope.ScaleSetName() - instanceID = s.Scope.InstanceID() - providerID = s.Scope.ProviderID() - isFlex = s.Scope.OrchestrationMode() == infrav1.FlexibleOrchestrationMode - ) + spec := s.Scope.ScaleSetVMSpec() + scaleSetVMSpec, ok := spec.(*ScaleSetVMSpec) + if !ok { + return errors.Errorf("%T is not of type ScaleSetVMSpec", spec) + } + reconciler := s.Reconciler + var getter azure.ResourceSpecGetter = scaleSetVMSpec + var result interface{} + var err error // Fetch the latest instance or VM data. AzureMachinePoolReconciler handles model mutations. - if isFlex { - resourceID := strings.TrimPrefix(providerID, azureutil.ProviderIDPrefix) - log.V(4).Info("VMSS is flex", "vmssName", vmssName, "providerID", providerID, "resourceID", resourceID) - // Using VMSS Flex, so fetch by resource ID. - vm, err := s.VMClient.GetByID(ctx, resourceID) + if scaleSetVMSpec.IsFlex { + log.V(4).Info("VMSS is flex", "vmssName", scaleSetVMSpec.Name, "providerID", scaleSetVMSpec.ProviderID, "resourceID", scaleSetVMSpec.ResourceID) + getter, err = scaleSetVMSpecToVMSpec(*scaleSetVMSpec) if err != nil { - if azure.ResourceNotFound(err) { - return azure.WithTransientError(errors.New("vm does not exist yet"), 30*time.Second) - } - return errors.Wrap(err, "failed getting vm") + return errors.Wrap(err, "failed to convert scaleSetVMSpec to vmSpec") } - s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm, infrav1.FlexibleOrchestrationMode)) - return nil + reconciler = s.VMReconciler + } else { + log.V(4).Info("VMSS is uniform", "vmssName", scaleSetVMSpec.Name, "providerID", scaleSetVMSpec.ProviderID, "instanceID", scaleSetVMSpec.InstanceID) } - log.V(4).Info("VMSS is uniform", "vmssName", vmssName, "providerID", providerID, "instanceID", instanceID) - // Using VMSS Uniform, so fetch by instance ID. - instance, err := s.Client.Get(ctx, resourceGroup, vmssName, instanceID) + // We only want to get the resource if it exists and handle the not found error. + // We're using CreateOrUpdateResource() to do so but it doesn't actually create or update anything since getter.Parameters() always returns nil. + result, err = reconciler.CreateOrUpdateResource(ctx, getter, serviceName) if err != nil { - if azure.ResourceNotFound(err) { - return azure.WithTransientError(errors.New("instance does not exist yet"), 30*time.Second) + return err + } else if result == nil { + return azure.WithTransientError(fmt.Errorf("instance does not exist yet"), time.Second*30) + } + + if scaleSetVMSpec.IsFlex { + vm, ok := result.(compute.VirtualMachine) + if !ok { + return errors.Errorf("%T is not of type compute.VirtualMachine", result) } - return errors.Wrap(err, "failed getting instance") + s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm, infrav1.FlexibleOrchestrationMode)) + } else { + instance, ok := result.(compute.VirtualMachineScaleSetVM) + if !ok { + return errors.Errorf("%T is not of type compute.VirtualMachineScaleSetVM", result) + } + s.Scope.SetVMSSVM(converters.SDKToVMSSVM(instance)) } - s.Scope.SetVMSSVM(converters.SDKToVMSSVM(instance)) return nil } // Delete deletes a scaleset instance asynchronously returning a future which encapsulates the long-running operation. func (s *Service) Delete(ctx context.Context) error { - var ( - resourceGroup = s.Scope.ResourceGroup() - vmssName = s.Scope.ScaleSetName() - instanceID = s.Scope.InstanceID() - providerID = s.Scope.ProviderID() - isFlex = s.Scope.OrchestrationMode() == infrav1.FlexibleOrchestrationMode - ) + ctx, _, done := tele.StartSpanWithLogger(ctx, "scalesetvms.Service.Delete") - ctx, log, done := tele.StartSpanWithLogger( - ctx, - "scalesetvms.Service.Delete", - tele.KVP("resourceGroup", resourceGroup), - tele.KVP("scaleset", vmssName), - tele.KVP("instanceID", instanceID), - ) defer done() - if isFlex { - return s.deleteVMSSFlexVM(ctx, strings.TrimPrefix(providerID, azureutil.ProviderIDPrefix)) + spec := s.Scope.ScaleSetVMSpec() + scaleSetVMSpec, ok := spec.(*ScaleSetVMSpec) + if !ok { + return errors.Errorf("%T is not of type ScaleSetVMSpec", spec) } - return s.deleteVMSSUniformInstance(ctx, resourceGroup, vmssName, instanceID, log) -} - -func (s *Service) deleteVMSSFlexVM(ctx context.Context, resourceID string) error { - ctx, log, done := tele.StartSpanWithLogger(ctx, "scalesetvms.Service.deleteVMSSFlexVM") - defer done() - - defer func() { - if vm, err := s.VMClient.GetByID(ctx, resourceID); err == nil && vm.VirtualMachineProperties != nil { - log.V(4).Info("vmss vm delete in progress", "state", vm.ProvisioningState) - s.Scope.SetVMSSVM(converters.SDKVMToVMSSVM(vm, s.Scope.OrchestrationMode())) - } - }() - - parsed, err := azureutil.ParseResourceID(resourceID) - if err != nil { - return errors.Wrap(err, fmt.Sprintf("failed to parse resource id %q", resourceID)) - } - resourceGroup, resourceName := parsed.ResourceGroupName, parsed.Name - log.V(4).Info("entering delete") - future := s.Scope.GetLongRunningOperationState(resourceName, serviceName, infrav1.DeleteFuture) - if future != nil { - if future.Type != infrav1.DeleteFuture { - return azure.WithTransientError(errors.New("attempting to delete, non-delete operation in progress"), 30*time.Second) - } - - log.V(4).Info("checking if the vm is done deleting") - if _, err := s.VMClient.GetResultIfDone(ctx, future); err != nil { - // fetch vm to update status - return errors.Wrap(err, "failed to get result of long running operation") + reconciler := s.Reconciler + var getter azure.ResourceSpecGetter = scaleSetVMSpec + var err error + if scaleSetVMSpec.IsFlex { + getter, err = scaleSetVMSpecToVMSpec(*scaleSetVMSpec) + if err != nil { + return errors.Wrap(err, "failed to convert scaleSetVMSpec to vmSpec") } - - // there was no error in fetching the result, the future has been completed - log.V(4).Info("successfully deleted the vm") - s.Scope.DeleteLongRunningOperationState(resourceName, serviceName, infrav1.DeleteFuture) - return nil - } - // since the future was nil, there is no ongoing activity; start deleting the vm - log.V(4).Info("vmss delete vm future is nil") // This is always true - - vmGetter := &VMSSFlexVMGetter{ - Name: resourceName, - ResourceGroup: resourceGroup, + reconciler = s.VMReconciler } - sdkFuture, err := s.VMClient.DeleteAsync(ctx, vmGetter) + err = reconciler.DeleteResource(ctx, getter, serviceName) if err != nil { - if azure.ResourceNotFound(err) { - // already deleted - return nil - } - return errors.Wrapf(err, "failed to delete vm %s/%s", resourceGroup, resourceName) + s.Scope.SetVMSSVMState(infrav1.Deleting) + } else { + s.Scope.SetVMSSVMState(infrav1.Deleted) } - if sdkFuture != nil { - future, err = converters.SDKToFuture(sdkFuture, infrav1.DeleteFuture, serviceName, vmGetter.ResourceName(), vmGetter.ResourceGroupName()) - if err != nil { - return errors.Wrapf(err, "failed to convert SDK to Future %s/%s", resourceGroup, resourceName) - } - s.Scope.SetLongRunningOperationState(future) - return nil - } - - s.Scope.DeleteLongRunningOperationState(resourceName, serviceName, infrav1.DeleteFuture) - return nil + return err } -func (s *Service) deleteVMSSUniformInstance(ctx context.Context, resourceGroup string, vmssName string, instanceID string, log logr.Logger) error { - ctx, log, done := tele.StartSpanWithLogger(ctx, "scalesetvms.Service.deleteVMSSUniformInstance") - defer done() - - defer func() { - if instance, err := s.Client.Get(ctx, resourceGroup, vmssName, instanceID); err == nil && instance.VirtualMachineScaleSetVMProperties != nil { - log.V(4).Info("updating vmss vm state", "state", instance.ProvisioningState) - s.Scope.SetVMSSVM(converters.SDKToVMSSVM(instance)) - } - }() - - log.V(4).Info("entering delete") - future := s.Scope.GetLongRunningOperationState(instanceID, serviceName, infrav1.DeleteFuture) - if future != nil { - if future.Type != infrav1.DeleteFuture { - return azure.WithTransientError(errors.New("attempting to delete, non-delete operation in progress"), 30*time.Second) - } - - log.V(4).Info("checking if the instance is done deleting") - if _, err := s.Client.GetResultIfDone(ctx, future); err != nil { - // fetch instance to update status - return errors.Wrap(err, "failed to get result of long running operation") - } - - // there was no error in fetching the result, the future has been completed - log.V(4).Info("successfully deleted the instance") - s.Scope.DeleteLongRunningOperationState(instanceID, serviceName, infrav1.DeleteFuture) - return nil - } - - // since the future was nil, there is no ongoing activity; start deleting the instance - future, err := s.Client.DeleteAsync(ctx, resourceGroup, vmssName, instanceID) +func scaleSetVMSpecToVMSpec(scaleSetVMSpec ScaleSetVMSpec) (*VMSSFlexGetter, error) { + parsed, err := azureutil.ParseResourceID(scaleSetVMSpec.ResourceID) if err != nil { - if azure.ResourceNotFound(err) { - // already deleted - return nil - } - return errors.Wrapf(err, "failed to delete instance %s/%s", vmssName, instanceID) - } - - s.Scope.SetLongRunningOperationState(future) - - log.V(4).Info("checking if the instance is done deleting") - if _, err := s.Client.GetResultIfDone(ctx, future); err != nil { - // fetch instance to update status - return errors.Wrap(err, "failed to get result of long running operation") + return nil, errors.Wrap(err, fmt.Sprintf("failed to parse resource id %q", scaleSetVMSpec.ResourceID)) } + resourceGroup, resourceName := parsed.ResourceGroupName, parsed.Name - s.Scope.DeleteLongRunningOperationState(instanceID, serviceName, infrav1.DeleteFuture) - return nil -} - -// VMSSFlexVMGetter gets the information required to create, update, or delete an Azure resource. -type VMSSFlexVMGetter struct { - Name string - ResourceGroup string -} - -// ResourceName returns the name of the resource. -func (vm *VMSSFlexVMGetter) ResourceName() string { - return vm.Name -} - -// OwnerResourceName returns the name of the resource that owns this Azure subresource. -func (vm *VMSSFlexVMGetter) OwnerResourceName() string { - return "" -} - -// ResourceGroupName returns the name of the resource group the resource is in. -func (vm *VMSSFlexVMGetter) ResourceGroupName() string { - return vm.ResourceGroup -} - -// Parameters takes the existing resource and returns the desired parameters of the resource. -// If the resource does not exist, or we do not care about existing parameters to update the resource, existing should be `nil`. -// If no update is needed on the resource, Parameters should return `nil`. -// NOTE: Not yet implemented, see kubernetes-sigs/cluster-api-provider-azure#2720. -func (vm *VMSSFlexVMGetter) Parameters(ctx context.Context, existing interface{}) (params interface{}, err error) { - return nil, nil + return &VMSSFlexGetter{ + Name: resourceName, + ResourceGroup: resourceGroup, + }, nil } diff --git a/azure/services/scalesetvms/scalesetvms_test.go b/azure/services/scalesetvms/scalesetvms_test.go index 8d44fb0002f..648c9dad947 100644 --- a/azure/services/scalesetvms/scalesetvms_test.go +++ b/azure/services/scalesetvms/scalesetvms_test.go @@ -18,313 +18,213 @@ package scalesetvms import ( "context" - "fmt" "net/http" "testing" - "time" "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute" "github.com/Azure/go-autorest/autorest" . "github.com/onsi/gomega" - "github.com/pkg/errors" "go.uber.org/mock/gomock" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/ptr" 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/converters" - "sigs.k8s.io/cluster-api-provider-azure/azure/scope" + "sigs.k8s.io/cluster-api-provider-azure/azure/services/async/mock_async" "sigs.k8s.io/cluster-api-provider-azure/azure/services/scalesetvms/mock_scalesetvms" - "sigs.k8s.io/cluster-api-provider-azure/azure/services/virtualmachines/mock_virtualmachines" - infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" - gomock2 "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" - "sigs.k8s.io/controller-runtime/pkg/client/fake" + gomockinternal "sigs.k8s.io/cluster-api-provider-azure/internal/test/matchers/gomock" ) var ( - autorest404 = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: http.StatusNotFound}, "Not Found") -) - -func TestNewService(t *testing.T) { - g := NewGomegaWithT(t) - scheme := runtime.NewScheme() - _ = clusterv1.AddToScheme(scheme) - _ = expv1.AddToScheme(scheme) - _ = infrav1.AddToScheme(scheme) - _ = infrav1exp.AddToScheme(scheme) + uniformScaleSetVMSpec = &ScaleSetVMSpec{ + Name: "my-vmss", + InstanceID: "0", + ResourceGroup: "my-rg", + ScaleSetName: "my-vmss", + ProviderID: "azure:///subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachineScaleSets/my-vmss/virtualMachines/0", + ResourceID: "/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachineScaleSets/my-vmss/virtualMachines/0", + IsFlex: false, + } + uniformScaleSetVM = compute.VirtualMachineScaleSetVM{ + ID: &uniformScaleSetVMSpec.ResourceID, + } - cluster := &clusterv1.Cluster{ - ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"}, + flexScaleSetVMSpec = &ScaleSetVMSpec{ + Name: "my-vmss", + InstanceID: "0", + ResourceGroup: "my-rg", + ScaleSetName: "my-vmss", + ProviderID: "azure:///subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachineScaleSets/my-vmss/virtualMachines/my-vmss", + ResourceID: "/subscriptions/123/resourceGroups/my-rg/providers/Microsoft.Compute/virtualMachineScaleSets/my-vmss/virtualMachines/my-vmss", + IsFlex: true, + } + flexGetter = &VMSSFlexGetter{ + Name: flexScaleSetVMSpec.Name, + ResourceGroup: flexScaleSetVMSpec.ResourceGroup, + } + flexScaleSetVM = compute.VirtualMachine{ + Name: &uniformScaleSetVMSpec.Name, } - client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cluster).Build() - s, err := scope.NewClusterScope(context.Background(), scope.ClusterScopeParams{ - AzureClients: scope.AzureClients{ - Authorizer: autorest.NullAuthorizer{}, - }, - Client: client, - Cluster: cluster, - AzureCluster: &infrav1.AzureCluster{ - Spec: infrav1.AzureClusterSpec{ - AzureClusterClassSpec: infrav1.AzureClusterClassSpec{ - Location: "test-location", - SubscriptionID: "123", - }, - ResourceGroup: "my-rg", - NetworkSpec: infrav1.NetworkSpec{ - Vnet: infrav1.VnetSpec{Name: "my-vnet", ResourceGroup: "my-rg"}, - }, - }, - }, - }) - g.Expect(err).NotTo(HaveOccurred()) - mpms, err := scope.NewMachinePoolMachineScope(scope.MachinePoolMachineScopeParams{ - Client: client, - MachinePool: new(expv1.MachinePool), - AzureMachinePool: new(infrav1exp.AzureMachinePool), - AzureMachinePoolMachine: new(infrav1exp.AzureMachinePoolMachine), - ClusterScope: s, - }) - g.Expect(err).NotTo(HaveOccurred()) - actual := NewService(mpms) - g.Expect(actual).NotTo(BeNil()) -} + errInternal = autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: http.StatusInternalServerError}, "Internal Server Error") +) -func TestService_Reconcile(t *testing.T) { - cases := []struct { - Name string - Setup func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder) - Err error - CheckIsErr bool +func TestReconcileVMSS(t *testing.T) { + testcases := []struct { + name string + expect func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) + expectedError string }{ { - Name: "should reconcile successfully", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder) { - s.ResourceGroup().Return("rg") - s.InstanceID().Return("0") - s.ProviderID().Return("foo") - s.ScaleSetName().Return("scaleset") - vm := compute.VirtualMachineScaleSetVM{ - InstanceID: ptr.To("0"), - } - m.Get(gomock2.AContext(), "rg", "scaleset", "0").Return(vm, nil) - s.SetVMSSVM(converters.SDKToVMSSVM(vm)) + name: "get a uniform vmss vm", + expectedError: "", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(uniformScaleSetVMSpec) + r.CreateOrUpdateResource(gomockinternal.AContext(), uniformScaleSetVMSpec, serviceName).Return(uniformScaleSetVM, nil) + s.SetVMSSVM(converters.SDKToVMSSVM(uniformScaleSetVM)) }, }, { - Name: "if 404, then should respond with transient error", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder) { - s.ResourceGroup().Return("rg") - s.InstanceID().Return("0") - s.ProviderID().Return("foo") - s.ScaleSetName().Return("scaleset") - m.Get(gomock2.AContext(), "rg", "scaleset", "0").Return(compute.VirtualMachineScaleSetVM{}, autorest404) + name: "get a vmss flex vm", + expectedError: "", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(flexScaleSetVMSpec) + v.CreateOrUpdateResource(gomockinternal.AContext(), flexGetter, serviceName).Return(flexScaleSetVM, nil) + s.SetVMSSVM(converters.SDKVMToVMSSVM(flexScaleSetVM, infrav1.FlexibleOrchestrationMode)) }, - Err: azure.WithTransientError(errors.New("instance does not exist yet"), 30*time.Second), - CheckIsErr: true, }, { - Name: "if other error, then should respond with error", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder) { - s.ResourceGroup().Return("rg") - s.InstanceID().Return("0") - s.ProviderID().Return("foo") - s.ScaleSetName().Return("scaleset") - m.Get(gomock2.AContext(), "rg", "scaleset", "0").Return(compute.VirtualMachineScaleSetVM{}, errors.New("boom")) + name: "uniform vmss vm doesn't exist yet", + expectedError: "instance does not exist yet. Object will be requeued after 30s", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(uniformScaleSetVMSpec) + r.CreateOrUpdateResource(gomockinternal.AContext(), uniformScaleSetVMSpec, serviceName).Return(nil, nil) + }, + }, + { + name: "vmss flex vm doesn't exist yet", + expectedError: "instance does not exist yet. Object will be requeued after 30s", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(flexScaleSetVMSpec) + v.CreateOrUpdateResource(gomockinternal.AContext(), flexGetter, serviceName).Return(nil, nil) + }, + }, + { + name: "error getting uniform vmss vm", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(uniformScaleSetVMSpec) + r.CreateOrUpdateResource(gomockinternal.AContext(), uniformScaleSetVMSpec, serviceName).Return(nil, errInternal) + }, + }, + { + name: "error getting vmss flex vm", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(flexScaleSetVMSpec) + v.CreateOrUpdateResource(gomockinternal.AContext(), flexGetter, serviceName).Return(nil, errInternal) }, - Err: errors.Wrap(errors.New("boom"), "failed getting instance"), }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { - var ( - g = NewWithT(t) - mockCtrl = gomock.NewController(t) - scopeMock = mock_scalesetvms.NewMockScaleSetVMScope(mockCtrl) - clientMock = mock_scalesetvms.NewMockclient(mockCtrl) - ) + 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.EXPECT().SubscriptionID().Return("subID").AnyTimes() - scopeMock.EXPECT().BaseURI().Return("https://localhost/").AnyTimes() - scopeMock.EXPECT().Authorizer().Return(nil).AnyTimes() - scopeMock.EXPECT().OrchestrationMode().Return(infrav1.UniformOrchestrationMode).AnyTimes() + scopeMock := mock_scalesetvms.NewMockScaleSetVMScope(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) + vmAsyncMock := mock_async.NewMockReconciler(mockCtrl) - service := NewService(scopeMock) - service.Client = clientMock - c.Setup(scopeMock.EXPECT(), clientMock.EXPECT()) + tc.expect(g, scopeMock.EXPECT(), asyncMock.EXPECT(), vmAsyncMock.EXPECT()) - if err := service.Reconcile(context.TODO()); c.Err == nil { - g.Expect(err).To(Succeed()) - } else { + s := &Service{ + Scope: scopeMock, + Reconciler: asyncMock, + VMReconciler: vmAsyncMock, + } + + err := s.Reconcile(context.TODO()) + if tc.expectedError != "" { g.Expect(err).To(HaveOccurred()) - g.Expect(err).To(MatchError(c.Err.Error())) - if c.CheckIsErr { - g.Expect(errors.Is(err, c.Err)).To(BeTrue()) - } + g.Expect(err).To(MatchError(tc.expectedError), err.Error()) + } else { + g.Expect(err).NotTo(HaveOccurred()) } }) } } -func TestService_Delete(t *testing.T) { - cases := []struct { - Name string - Setup func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder, v *mock_virtualmachines.MockClientMockRecorder) - Err error - CheckIsErr bool +func TestDeleteVMSS(t *testing.T) { + testcases := []struct { + name string + expect func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) + expectedError string }{ { - Name: "should start deleting successfully if no long running operation is active", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder, v *mock_virtualmachines.MockClientMockRecorder) { - s.ResourceGroup().Return("rg") - s.InstanceID().Return("0") - s.ProviderID().Return("foo") - s.ScaleSetName().Return("scaleset") - s.OrchestrationMode().Return(infrav1.UniformOrchestrationMode) - s.GetLongRunningOperationState("0", serviceName, infrav1.DeleteFuture).Return(nil) - future := &infrav1.Future{ - Type: infrav1.DeleteFuture, - } - m.DeleteAsync(gomock2.AContext(), "rg", "scaleset", "0").Return(future, nil) - s.SetLongRunningOperationState(future) - m.GetResultIfDone(gomock2.AContext(), future).Return(compute.VirtualMachineScaleSetVM{}, azure.WithTransientError(azure.NewOperationNotDoneError(future), 15*time.Second)) - m.Get(gomock2.AContext(), "rg", "scaleset", "0").Return(compute.VirtualMachineScaleSetVM{}, nil) - }, - CheckIsErr: true, - Err: errors.Wrap(azure.WithTransientError(azure.NewOperationNotDoneError(&infrav1.Future{ - Type: infrav1.DeleteFuture, - }), 15*time.Second), "failed to get result of long running operation"), - }, - { - Name: "should finish deleting successfully when there's a long running operation that has completed", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder, v *mock_virtualmachines.MockClientMockRecorder) { - s.ResourceGroup().Return("rg") - s.InstanceID().Return("0") - s.ProviderID().Return("foo") - s.ScaleSetName().Return("scaleset") - s.OrchestrationMode().Return(infrav1.UniformOrchestrationMode) - future := &infrav1.Future{ - Type: infrav1.DeleteFuture, - } - s.GetLongRunningOperationState("0", serviceName, infrav1.DeleteFuture).Return(future) - m.GetResultIfDone(gomock2.AContext(), future).Return(compute.VirtualMachineScaleSetVM{}, nil) - s.DeleteLongRunningOperationState("0", serviceName, infrav1.DeleteFuture) - m.Get(gomock2.AContext(), "rg", "scaleset", "0").Return(compute.VirtualMachineScaleSetVM{}, nil) - }, - }, - { - Name: "should not error when deleting, but resource is 404", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder, v *mock_virtualmachines.MockClientMockRecorder) { - s.ResourceGroup().Return("rg") - s.InstanceID().Return("0") - s.ProviderID().Return("foo") - s.ScaleSetName().Return("scaleset") - s.OrchestrationMode().Return(infrav1.UniformOrchestrationMode) - s.GetLongRunningOperationState("0", serviceName, infrav1.DeleteFuture).Return(nil) - m.DeleteAsync(gomock2.AContext(), "rg", "scaleset", "0").Return(nil, autorest404) - m.Get(gomock2.AContext(), "rg", "scaleset", "0").Return(compute.VirtualMachineScaleSetVM{}, nil) - }, - }, - { - Name: "should error when deleting, but a non-404 error is returned from DELETE call", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder, v *mock_virtualmachines.MockClientMockRecorder) { - s.ResourceGroup().Return("rg") - s.InstanceID().Return("0") - s.ProviderID().Return("foo") - s.ScaleSetName().Return("scaleset") - s.OrchestrationMode().Return(infrav1.UniformOrchestrationMode) - s.GetLongRunningOperationState("0", serviceName, infrav1.DeleteFuture).Return(nil) - m.DeleteAsync(gomock2.AContext(), "rg", "scaleset", "0").Return(nil, errors.New("boom")) - m.Get(gomock2.AContext(), "rg", "scaleset", "0").Return(compute.VirtualMachineScaleSetVM{}, nil) + name: "delete a uniform vmss vm", + expectedError: "", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(uniformScaleSetVMSpec) + r.DeleteResource(gomockinternal.AContext(), uniformScaleSetVMSpec, serviceName).Return(nil) + s.SetVMSSVMState(infrav1.Deleted) }, - Err: errors.Wrap(errors.New("boom"), "failed to delete instance scaleset/0"), }, { - Name: "should return error when a long running operation is active and getting the result returns an error", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder, v *mock_virtualmachines.MockClientMockRecorder) { - s.ResourceGroup().Return("rg") - s.InstanceID().Return("0") - s.ProviderID().Return("foo") - s.ScaleSetName().Return("scaleset") - s.OrchestrationMode().Return(infrav1.UniformOrchestrationMode) - future := &infrav1.Future{ - Type: infrav1.DeleteFuture, - } - s.GetLongRunningOperationState("0", serviceName, infrav1.DeleteFuture).Return(future) - m.GetResultIfDone(gomock2.AContext(), future).Return(compute.VirtualMachineScaleSetVM{}, errors.New("boom")) - m.Get(gomock2.AContext(), "rg", "scaleset", "0").Return(compute.VirtualMachineScaleSetVM{}, nil) + name: "delete a vmss flex vm", + expectedError: "", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(flexScaleSetVMSpec) + v.DeleteResource(gomockinternal.AContext(), flexGetter, serviceName).Return(nil) + s.SetVMSSVMState(infrav1.Deleted) }, - Err: errors.Wrap(errors.New("boom"), "failed to get result of long running operation"), }, { - Name: "(flex) should delete successfully if no long-running operation is active", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder, v *mock_virtualmachines.MockClientMockRecorder) { - s.ResourceGroup().Return("my-cluster") - s.ScaleSetName().Return("scaleset") - s.InstanceID().Return("0") - s.ProviderID().Return("azure:///subscriptions/1234-5678/resourceGroups/my-cluster/providers/Microsoft.Compute/virtualMachines/my-cluster_1234abcd") - s.OrchestrationMode().Return(infrav1.FlexibleOrchestrationMode) - s.GetLongRunningOperationState("my-cluster_1234abcd", serviceName, infrav1.DeleteFuture).Return(nil) - vmGetter := &VMSSFlexVMGetter{ - Name: "my-cluster_1234abcd", - ResourceGroup: "my-cluster", - } - future := &infrav1.Future{ - Type: infrav1.DeleteFuture, - } - sdkFuture, _ := converters.FutureToSDK(*future) - v.DeleteAsync(gomock2.AContext(), vmGetter).Return(sdkFuture, nil) - s.DeleteLongRunningOperationState("my-cluster_1234abcd", serviceName, infrav1.DeleteFuture) - v.GetByID(gomock2.AContext(), "/subscriptions/1234-5678/resourceGroups/my-cluster/providers/Microsoft.Compute/virtualMachines/my-cluster_1234abcd").Return(compute.VirtualMachine{}, nil) + name: "error when deleting a uniform vmss vm", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(uniformScaleSetVMSpec) + r.DeleteResource(gomockinternal.AContext(), uniformScaleSetVMSpec, serviceName).Return(errInternal) + s.SetVMSSVMState(infrav1.Deleting) }, }, { - Name: "(flex) should error if providerID is invalid", - Setup: func(s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, m *mock_scalesetvms.MockclientMockRecorder, v *mock_virtualmachines.MockClientMockRecorder) { - s.ResourceGroup().Return("my-cluster") - s.ScaleSetName().Return("scaleset") - s.InstanceID().Return("0") - s.ProviderID().Return("foo") - s.OrchestrationMode().Return(infrav1.FlexibleOrchestrationMode) - v.GetByID(gomock2.AContext(), "foo").Return(compute.VirtualMachine{}, nil) + name: "error when deleting a vmss flex vm", + expectedError: "#: Internal Server Error: StatusCode=500", + expect: func(g *WithT, s *mock_scalesetvms.MockScaleSetVMScopeMockRecorder, r *mock_async.MockReconcilerMockRecorder, v *mock_async.MockReconcilerMockRecorder) { + s.ScaleSetVMSpec().Return(flexScaleSetVMSpec) + v.DeleteResource(gomockinternal.AContext(), flexGetter, serviceName).Return(errInternal) + s.SetVMSSVMState(infrav1.Deleting) }, - Err: errors.Wrap(fmt.Errorf("invalid resource ID: resource id '%s' must start with '/'", "foo"), fmt.Sprintf("failed to parse resource id %q", "foo")), }, } - for _, c := range cases { - t.Run(c.Name, func(t *testing.T) { - var ( - g = NewWithT(t) - mockCtrl = gomock.NewController(t) - scopeMock = mock_scalesetvms.NewMockScaleSetVMScope(mockCtrl) - clientMock = mock_scalesetvms.NewMockclient(mockCtrl) - vmClientMock = mock_virtualmachines.NewMockClient(mockCtrl) - ) + 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.EXPECT().SubscriptionID().Return("subID").AnyTimes() - scopeMock.EXPECT().BaseURI().Return("https://localhost/").AnyTimes() - scopeMock.EXPECT().Authorizer().Return(nil).AnyTimes() + scopeMock := mock_scalesetvms.NewMockScaleSetVMScope(mockCtrl) + asyncMock := mock_async.NewMockReconciler(mockCtrl) + vmAsyncMock := mock_async.NewMockReconciler(mockCtrl) - service := NewService(scopeMock) - service.Client = clientMock - service.VMClient = vmClientMock - c.Setup(scopeMock.EXPECT(), clientMock.EXPECT(), vmClientMock.EXPECT()) + tc.expect(g, scopeMock.EXPECT(), asyncMock.EXPECT(), vmAsyncMock.EXPECT()) - if err := service.Delete(context.TODO()); c.Err == nil { - g.Expect(err).To(Succeed()) - } else { + s := &Service{ + Scope: scopeMock, + Reconciler: asyncMock, + VMReconciler: vmAsyncMock, + } + + err := s.Delete(context.TODO()) + if tc.expectedError != "" { g.Expect(err).To(HaveOccurred()) - g.Expect(err).To(MatchError(c.Err.Error())) - if c.CheckIsErr { - g.Expect(errors.Is(err, c.Err)).To(BeTrue()) - } + g.Expect(err).To(MatchError(tc.expectedError), err.Error()) + } else { + g.Expect(err).NotTo(HaveOccurred()) } }) } diff --git a/azure/services/scalesetvms/spec.go b/azure/services/scalesetvms/spec.go new file mode 100644 index 00000000000..2dbba5cbe1f --- /dev/null +++ b/azure/services/scalesetvms/spec.go @@ -0,0 +1,78 @@ +/* +Copyright 2023 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 scalesetvms + +import ( + "context" +) + +// ScaleSetVMSpec defines the specification for a VMSS VM. +type ScaleSetVMSpec struct { + Name string + InstanceID string + ResourceGroup string + ScaleSetName string + ProviderID string + ResourceID string + IsFlex bool +} + +// ResourceName returns the instance ID of the VMSS VM. This is because the it is identified by the instance ID in Azure instead of the name. +func (s *ScaleSetVMSpec) ResourceName() string { + return s.InstanceID +} + +// ResourceGroupName returns the name of the resource group the VMSS that owns this VM. +func (s *ScaleSetVMSpec) ResourceGroupName() string { + return s.ResourceGroup +} + +// OwnerResourceName returns the name of the VMSS that owns this VM. +func (s *ScaleSetVMSpec) OwnerResourceName() string { + return s.ScaleSetName +} + +// Parameters is a no-op for VMSS VMs as this spec is only used to Get(). +func (s *ScaleSetVMSpec) Parameters(ctx context.Context, existing interface{}) (params interface{}, err error) { + return nil, nil +} + +// VMSSFlexGetter defines the specification for a VMSS flex VM. +type VMSSFlexGetter struct { + Name string + ResourceGroup string +} + +// ResourceName returns the name of the flex VM. +func (s *VMSSFlexGetter) ResourceName() string { + return s.Name +} + +// ResourceGroupName returns the name of the flex VM. +func (s *VMSSFlexGetter) ResourceGroupName() string { + return s.ResourceGroup +} + +// OwnerResourceName is a no-op for flex VMs. +func (s *VMSSFlexGetter) OwnerResourceName() string { + return "" +} + +// Parameters is a no-op for flex VMs as this spec is only used to Get(). +func (s *VMSSFlexGetter) Parameters(ctx context.Context, existing interface{}) (params interface{}, err error) { + return nil, nil +}