Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make scalesetvms delete async #3799

Merged
merged 1 commit into from
Sep 8, 2023
Merged

Make scalesetvms delete async #3799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions azure/scope/machinepoolmachine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"reflect"
"strings"
"time"

"github.com/pkg/errors"
Expand All @@ -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"
Expand Down Expand Up @@ -149,6 +152,24 @@ func NewMachinePoolMachineScope(params MachinePoolMachineScopeParams) (*MachineP
}, nil
}

// ScaleSetVMSpec returns the VMSS VM spec.
func (s *MachinePoolMachineScope) ScaleSetVMSpec() azure.ResourceSpecGetter {
Jont828 marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -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 {
Expand Down
122 changes: 122 additions & 0 deletions azure/scope/machinepoolmachine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,22 @@ 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"
corev1 "k8s.io/api/core/v1"
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"
Expand Down Expand Up @@ -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)
Expand Down
122 changes: 42 additions & 80 deletions azure/services/scalesetvms/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}

Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you shouldn't need to do this, you should be able to do async.New(scope, nil, client), when creating the service in scalesetvms.go so this client doesn't need to implement the Deleter interface

check out the disks client for a similar example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, in the disks service we don't attempt to call CreateOrUpdateResource() so we can pass in a nil to the Creator interface. In this service, we want to get the resource if it exists and so I'm trying to leverage the CreateOrUpdateResource() in the async interface to fetch the resource and handle the not found/transient errors. Alternatively, I could try to construct the client as well, but I felt that it would be clunky to declare a Reconciler and a client for each type of VM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm thinking about this more, I wonder if it might make more sense to add a Getter (https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/azure/services/async/interfaces.go#L41) in the Service and simply call getter.Get() in the Reconcile func instead of using CreateOrUpdateResource in a way it's not meant to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was to be able to take advantage of the error handling in this block of CreateOrUpdateResource() so we don't have to duplicate that logic. Maybe we could add a Get() function to the Reconciler iface to give us the error handling?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mboersma what do you think? I'm concerned this won't work the same way with the async poller framework when we try to migrade scaleset vms to sdk v2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think this will act the same way in asyncpoller; the CreateOrUpdateResource code isn't really changed. I think we can merge this as-is and it won't present problems for refactoring SDKv2 on top of it.

func (ac *azureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.ResourceSpecGetter, parameters interface{}) (result interface{}, future azureautorest.FutureAPI, err error) {
CecileRobertMichon marked this conversation as resolved.
Show resolved Hide resolved
_, _, 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)
}
Loading