From 70fe32277f1bfe45f3ad32fa82d015266ca2dd60 Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Fri, 5 Nov 2021 13:29:44 -0700 Subject: [PATCH 1/2] Return existing VM as result if VM does not need to be updated --- azure/services/virtualmachines/client.go | 56 ++++++++---------------- 1 file changed, 19 insertions(+), 37 deletions(-) diff --git a/azure/services/virtualmachines/client.go b/azure/services/virtualmachines/client.go index 8140413f10c..d4400f02c4c 100644 --- a/azure/services/virtualmachines/client.go +++ b/azure/services/virtualmachines/client.go @@ -78,15 +78,29 @@ func (ac *AzureClient) CreateOrUpdateAsync(ctx context.Context, spec azure.Resou ctx, _, done := tele.StartSpanWithLogger(ctx, "virtualmachines.AzureClient.CreateOrUpdate") defer done() - vm, err := ac.vmParams(ctx, spec) + var existingVM interface{} + + if existing, err := ac.Get(ctx, spec.ResourceGroupName(), spec.ResourceName()); err != nil && !azure.ResourceNotFound(err) { + return nil, nil, errors.Wrapf(err, "failed to get virtual machine %s in %s", spec.ResourceName(), spec.ResourceGroupName()) + } else if err == nil { + existingVM = existing + } + + params, err := spec.Parameters(existingVM) if err != nil { return nil, nil, errors.Wrapf(err, "failed to get desired parameters for virtual machine %s", spec.ResourceName()) - } else if vm == nil { - // nothing to do here. - return nil, nil, nil } - future, err := ac.virtualmachines.CreateOrUpdate(ctx, spec.ResourceGroupName(), spec.ResourceName(), *vm) + vm, ok := params.(compute.VirtualMachine) + if !ok { + if params == nil { + // nothing to do here. + return existingVM, nil, nil + } + return nil, nil, errors.Errorf("%T is not a compute.VirtualMachine", params) + } + + future, err := ac.virtualmachines.CreateOrUpdate(ctx, spec.ResourceGroupName(), spec.ResourceName(), vm) if err != nil { return nil, nil, err } @@ -186,35 +200,3 @@ func (ac *AzureClient) Result(ctx context.Context, futureData azureautorest.Futu return result(ac.virtualmachines) } - -// vmParams creates a VirtualMachine object from the given spec. -func (ac *AzureClient) vmParams(ctx context.Context, spec azure.ResourceSpecGetter) (*compute.VirtualMachine, error) { - ctx, span := tele.Tracer().Start(ctx, "virtualmachines.AzureClient.vmParams") - defer span.End() - - var params interface{} - var existing interface{} - - if existingVM, err := ac.Get(ctx, spec.ResourceGroupName(), spec.ResourceName()); err != nil && !azure.ResourceNotFound(err) { - return nil, errors.Wrapf(err, "failed to get virtual machine %s in %s", spec.ResourceName(), spec.ResourceGroupName()) - } else if err == nil { - // virtual machine already exists - existing = existingVM - } - - params, err := spec.Parameters(existing) - if err != nil { - return nil, err - } - - vm, ok := params.(compute.VirtualMachine) - if !ok { - if params == nil { - // nothing to do here. - return nil, nil - } - return nil, errors.Errorf("%T is not a compute.VirtualMachine", params) - } - - return &vm, nil -} From 8a4b3575e90e2ddb353c289d9b72094067a467dc Mon Sep 17 00:00:00 2001 From: Cecile Robert-Michon Date: Fri, 5 Nov 2021 13:34:37 -0700 Subject: [PATCH 2/2] Check if a future is returned regardless of error for async create and delete --- azure/services/async/async.go | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/azure/services/async/async.go b/azure/services/async/async.go index 9c211a561bc..5977d7c375e 100644 --- a/azure/services/async/async.go +++ b/azure/services/async/async.go @@ -85,16 +85,14 @@ func CreateResource(ctx context.Context, scope FutureScope, client Creator, spec // No long running operation is active, so create the resource. scope.V(2).Info("creating resource", "service", serviceName, "resource", resourceName, "resourceGroup", rgName) result, sdkFuture, err := client.CreateOrUpdateAsync(ctx, spec) - if err != nil { - if sdkFuture != nil { - future, err := converters.SDKToFuture(sdkFuture, infrav1.PutFuture, serviceName, resourceName, rgName) - if err != nil { - return nil, errors.Wrapf(err, "failed to create resource %s/%s (service: %s)", rgName, resourceName, serviceName) - } - scope.SetLongRunningOperationState(future) - return nil, azure.WithTransientError(azure.NewOperationNotDoneError(future), retryAfter(sdkFuture)) + if sdkFuture != nil { + future, err := converters.SDKToFuture(sdkFuture, infrav1.PutFuture, serviceName, resourceName, rgName) + if err != nil { + return nil, errors.Wrapf(err, "failed to create resource %s/%s (service: %s)", rgName, resourceName, serviceName) } - + scope.SetLongRunningOperationState(future) + return nil, azure.WithTransientError(azure.NewOperationNotDoneError(future), retryAfter(sdkFuture)) + } else if err != nil { return nil, errors.Wrapf(err, "failed to create resource %s/%s (service: %s)", rgName, resourceName, serviceName) } @@ -120,19 +118,18 @@ func DeleteResource(ctx context.Context, scope FutureScope, client Deleter, spec // No long running operation is active, so delete the resource. scope.V(2).Info("deleting resource", "service", serviceName, "resource", resourceName, "resourceGroup", rgName) sdkFuture, err := client.DeleteAsync(ctx, spec) - if err != nil { + if sdkFuture != nil { + future, err := converters.SDKToFuture(sdkFuture, infrav1.DeleteFuture, serviceName, resourceName, rgName) + if err != nil { + return errors.Wrapf(err, "failed to delete resource %s/%s (service: %s)", rgName, resourceName, serviceName) + } + scope.SetLongRunningOperationState(future) + return azure.WithTransientError(azure.NewOperationNotDoneError(future), retryAfter(sdkFuture)) + } else if err != nil { if azure.ResourceNotFound(err) { // already deleted return nil - } else if sdkFuture != nil { - future, err := converters.SDKToFuture(sdkFuture, infrav1.DeleteFuture, serviceName, resourceName, rgName) - if err != nil { - return errors.Wrapf(err, "failed to delete resource %s/%s (service: %s)", rgName, resourceName, serviceName) - } - scope.SetLongRunningOperationState(future) - return azure.WithTransientError(azure.NewOperationNotDoneError(future), retryAfter(sdkFuture)) } - return errors.Wrapf(err, "failed to delete resource %s/%s (service: %s)", rgName, resourceName, serviceName) }