-
Notifications
You must be signed in to change notification settings - Fork 470
Make scalesetvms delete async #3799
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 check out the disks client for a similar example
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I think this will act the same way in |
||
| 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.
|
||
| _, _, 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) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.