Merge pull request #4441 from Azure/dbizau/disable_vmss_repairs_before_cleanup

disable automatic vmss repairs before cleanup and add retry

Author: mociarain

pkg/deploy/deploy.go

@@ -9,11 +9,13 @@ import (
 	"fmt"
 	"reflect"
 	"strings"
+	"time"
 
 	"github.com/jongio/azidext/go/azidext"
 	"github.com/sirupsen/logrus"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	mgmtcompute "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2020-06-01/compute"
 	mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features"
 	"github.com/Azure/go-autorest/autorest/azure"
 
@@ -29,6 +31,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/features"
 	"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/msi"
 	"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/storage"
+	"github.com/Azure/ARO-RP/pkg/util/pointerutils"
 )
 
 var _ Deployer = (*deployer)(nil)
@@ -266,3 +269,61 @@ func (d *deployer) checkForKnownError(serviceErr *azure.ServiceError, deployAtte
 
 	return "", nil
 }
+
+// disableAutomaticRepairsOnVMSS disables automatic repairs on a VMSS to prevent
+// race conditions during teardown when stopping services causes health probe failures.
+// This is a best-effort operation - we log errors as warnings but don't fail the
+// teardown, allowing cleanup to proceed even if we can't disable repairs.
+func (d *deployer) disableAutomaticRepairsOnVMSS(ctx context.Context, resourceGroupName, vmssName string) error {
+	d.log.Printf("disabling automatic repairs on scaleset %s", vmssName)
+	start := time.Now()
+	err := d.vmss.UpdateAndWait(ctx, resourceGroupName, vmssName, mgmtcompute.VirtualMachineScaleSetUpdate{
+		VirtualMachineScaleSetUpdateProperties: &mgmtcompute.VirtualMachineScaleSetUpdateProperties{
+			AutomaticRepairsPolicy: &mgmtcompute.AutomaticRepairsPolicy{
+				Enabled: pointerutils.ToPtr(false),
+			},
+		},
+	})
+	d.log.Printf("disabling automatic repairs on %s took %v", vmssName, time.Since(start))
+
+	if err != nil {
+		// Log as warning but don't fail the teardown - we want to proceed with
+		// stopping services and deleting the VMSS even if we couldn't disable repairs
+		d.log.Warnf("failed to disable automatic repairs on %s, proceeding with teardown anyway: %v", vmssName, err)
+		return nil
+	}
+	return nil
+}
+
+// runCommandWithRetry runs a command on a VMSS instance with retry logic for
+// OperationPreempted errors that can occur when Azure repair operations race
+// with our shutdown commands.
+func (d *deployer) runCommandWithRetry(ctx context.Context, resourceGroupName, vmssName, instanceID string, input mgmtcompute.RunCommandInput) error {
+	const maxRetries = 3
+	const retryDelaySecs = 10
+
+	var err error
+	for i := 0; i < maxRetries; i++ {
+		err = d.vmssvms.RunCommandAndWait(ctx, resourceGroupName, vmssName, instanceID, input)
+		if err == nil || !isOperationPreemptedError(err) {
+			break
+		}
+		if i == maxRetries-1 {
+			break
+		}
+		d.log.Printf("RunCommand preempted on instance %s, retrying (%d/%d)", instanceID, i+1, maxRetries)
+		timer := time.NewTimer(retryDelaySecs * time.Second)
+		select {
+		case <-ctx.Done():
+			if !timer.Stop() {
+				<-timer.C
+			}
+			if ctxErr := ctx.Err(); ctxErr != nil {
+				return ctxErr
+			}
+			return err
+		case <-timer.C:
+		}
+	}
+	return err
+}

pkg/deploy/deploy_test.go

@@ -5,17 +5,22 @@ package deploy
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/sirupsen/logrus"
 	"go.uber.org/mock/gomock"
 
+	mgmtcompute "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2020-06-01/compute"
 	mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features"
+	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/azure"
 
 	"github.com/Azure/ARO-RP/pkg/util/arm"
+	mock_compute "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/compute"
 	mock_features "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/features"
 	mock_vmsscleaner "github.com/Azure/ARO-RP/pkg/util/mocks/vmsscleaner"
 	"github.com/Azure/ARO-RP/pkg/util/pointerutils"
@@ -379,3 +384,199 @@ func TestGetParameters(t *testing.T) {
 		})
 	}
 }
+
+func TestDisableAutomaticRepairsOnVMSS(t *testing.T) {
+	ctx := context.Background()
+	resourceGroupName := "rg"
+	vmssName := "vmss"
+
+	for _, tt := range []struct {
+		name      string
+		updateErr error
+	}{
+		{
+			name: "success",
+		},
+		{
+			name:      "azure error returns nil",
+			updateErr: errors.New("update failed"),
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+
+			mockVMSS := mock_compute.NewMockVirtualMachineScaleSetsClient(controller)
+
+			mockVMSS.EXPECT().UpdateAndWait(ctx, resourceGroupName, vmssName, gomock.AssignableToTypeOf(mgmtcompute.VirtualMachineScaleSetUpdate{})).DoAndReturn(
+				func(_ context.Context, rg, name string, update mgmtcompute.VirtualMachineScaleSetUpdate) error {
+					if update.VirtualMachineScaleSetUpdateProperties == nil {
+						t.Fatalf("expected VirtualMachineScaleSetUpdateProperties to be set")
+					}
+					policy := update.AutomaticRepairsPolicy
+					if policy == nil || policy.Enabled == nil {
+						t.Fatalf("expected AutomaticRepairsPolicy.Enabled to be set")
+					}
+					if *policy.Enabled {
+						t.Fatalf("expected AutomaticRepairsPolicy.Enabled to be false")
+					}
+					return tt.updateErr
+				},
+			)
+
+			d := deployer{
+				log:  logrus.NewEntry(logrus.StandardLogger()),
+				vmss: mockVMSS,
+			}
+
+			if err := d.disableAutomaticRepairsOnVMSS(ctx, resourceGroupName, vmssName); err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+		})
+	}
+}
+
+func TestRunCommandWithRetrySuccess(t *testing.T) {
+	ctx := context.Background()
+	controller := gomock.NewController(t)
+	defer controller.Finish()
+
+	input := mgmtcompute.RunCommandInput{}
+	resourceGroupName := "rg"
+	vmssName := "vmss"
+	instanceID := "1"
+
+	mockVMSSVMs := mock_compute.NewMockVirtualMachineScaleSetVMsClient(controller)
+	mockVMSSVMs.EXPECT().RunCommandAndWait(ctx, resourceGroupName, vmssName, instanceID, input).Return(nil)
+
+	d := deployer{
+		log:     logrus.NewEntry(logrus.StandardLogger()),
+		vmssvms: mockVMSSVMs,
+	}
+
+	if err := d.runCommandWithRetry(ctx, resourceGroupName, vmssName, instanceID, input); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestRunCommandWithRetryRetriesOnOperationPreempted(t *testing.T) {
+	ctx := context.Background()
+	controller := gomock.NewController(t)
+	defer controller.Finish()
+
+	input := mgmtcompute.RunCommandInput{}
+	resourceGroupName := "rg"
+	vmssName := "vmss"
+	instanceID := "1"
+
+	mockVMSSVMs := mock_compute.NewMockVirtualMachineScaleSetVMsClient(controller)
+	gomock.InOrder(
+		mockVMSSVMs.EXPECT().RunCommandAndWait(ctx, resourceGroupName, vmssName, instanceID, input).Return(newOperationPreemptedError()),
+		mockVMSSVMs.EXPECT().RunCommandAndWait(ctx, resourceGroupName, vmssName, instanceID, input).Return(nil),
+	)
+
+	d := deployer{
+		log:     logrus.NewEntry(logrus.StandardLogger()),
+		vmssvms: mockVMSSVMs,
+	}
+
+	start := time.Now()
+	if err := d.runCommandWithRetry(ctx, resourceGroupName, vmssName, instanceID, input); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	duration := time.Since(start)
+	if duration < 10*time.Second {
+		t.Fatalf("expected retry delay of at least 10s, got %v", duration)
+	}
+}
+
+func TestRunCommandWithRetryReturnsLastError(t *testing.T) {
+	ctx := context.Background()
+	controller := gomock.NewController(t)
+	defer controller.Finish()
+
+	input := mgmtcompute.RunCommandInput{}
+	resourceGroupName := "rg"
+	vmssName := "vmss"
+	instanceID := "1"
+	wantErr := newOperationPreemptedError()
+
+	mockVMSSVMs := mock_compute.NewMockVirtualMachineScaleSetVMsClient(controller)
+	mockVMSSVMs.EXPECT().RunCommandAndWait(ctx, resourceGroupName, vmssName, instanceID, input).Return(wantErr).Times(3)
+
+	d := deployer{
+		log:     logrus.NewEntry(logrus.StandardLogger()),
+		vmssvms: mockVMSSVMs,
+	}
+
+	start := time.Now()
+	err := d.runCommandWithRetry(ctx, resourceGroupName, vmssName, instanceID, input)
+	duration := time.Since(start)
+	if err != wantErr {
+		t.Fatalf("expected %v, got %v", wantErr, err)
+	}
+	if duration < 20*time.Second {
+		t.Fatalf("expected retry delay of at least 20s (2 retries), got %v", duration)
+	}
+}
+
+func TestRunCommandWithRetryReturnsNonRetryableError(t *testing.T) {
+	ctx := context.Background()
+	controller := gomock.NewController(t)
+	defer controller.Finish()
+
+	input := mgmtcompute.RunCommandInput{}
+	resourceGroupName := "rg"
+	vmssName := "vmss"
+	instanceID := "1"
+	wantErr := errors.New("boom")
+
+	mockVMSSVMs := mock_compute.NewMockVirtualMachineScaleSetVMsClient(controller)
+	mockVMSSVMs.EXPECT().RunCommandAndWait(ctx, resourceGroupName, vmssName, instanceID, input).Return(wantErr)
+
+	d := deployer{
+		log:     logrus.NewEntry(logrus.StandardLogger()),
+		vmssvms: mockVMSSVMs,
+	}
+
+	err := d.runCommandWithRetry(ctx, resourceGroupName, vmssName, instanceID, input)
+	if err != wantErr {
+		t.Fatalf("expected %v, got %v", wantErr, err)
+	}
+}
+
+func TestRunCommandWithRetryContextCancelled(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	controller := gomock.NewController(t)
+	defer controller.Finish()
+
+	input := mgmtcompute.RunCommandInput{}
+	resourceGroupName := "rg"
+	vmssName := "vmss"
+	instanceID := "1"
+
+	mockVMSSVMs := mock_compute.NewMockVirtualMachineScaleSetVMsClient(controller)
+	mockVMSSVMs.EXPECT().RunCommandAndWait(ctx, resourceGroupName, vmssName, instanceID, input).DoAndReturn(
+		func(context.Context, string, string, string, mgmtcompute.RunCommandInput) error {
+			// Cancel context during the first call to simulate cancellation during retry
+			cancel()
+			return newOperationPreemptedError()
+		},
+	)
+
+	d := deployer{
+		log:     logrus.NewEntry(logrus.StandardLogger()),
+		vmssvms: mockVMSSVMs,
+	}
+
+	err := d.runCommandWithRetry(ctx, resourceGroupName, vmssName, instanceID, input)
+	if !errors.Is(err, context.Canceled) {
+		t.Fatalf("expected context.Canceled error, got %v", err)
+	}
+}
+
+func newOperationPreemptedError() error {
+	return &autorest.DetailedError{
+		Original: &azure.ServiceError{Code: OperationPreemptedCode},
+	}
+}

pkg/deploy/error.go

@@ -4,10 +4,14 @@ package deploy
 // Licensed under the Apache License 2.0.
 
 import (
+	"strings"
+
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/azure"
 )
 
+const OperationPreemptedCode = "OperationPreempted"
+
 func isDeploymentNotFoundError(err error) bool {
 	if detailedErr, ok := err.(autorest.DetailedError); ok {
 		if requestErr, ok := detailedErr.Original.(*azure.RequestError); ok &&
@@ -18,3 +22,19 @@ func isDeploymentNotFoundError(err error) bool {
 	}
 	return false
 }
+
+func isOperationPreemptedError(err error) bool {
+	if detailedErr, ok := err.(autorest.DetailedError); ok {
+		if requestErr, ok := detailedErr.Original.(*azure.RequestError); ok &&
+			requestErr.ServiceError != nil &&
+			requestErr.ServiceError.Code == OperationPreemptedCode {
+			return true
+		}
+		if serviceErr, ok := detailedErr.Original.(*azure.ServiceError); ok &&
+			serviceErr.Code == OperationPreemptedCode {
+			return true
+		}
+	}
+	// Also check error message for OperationPreempted
+	return err != nil && strings.Contains(err.Error(), OperationPreemptedCode)
+}

pkg/deploy/upgrade_gateway.go

@@ -30,7 +30,10 @@ func (d *deployer) UpgradeGateway(ctx context.Context) error {
 		return err
 	}
 
-	return d.gatewayRemoveOldScalesets(ctx)
+	// Create a separate timeout for cleanup phase
+	cleanupCtx, cleanupCancel := context.WithTimeout(ctx, 30*time.Minute)
+	defer cleanupCancel()
+	return d.gatewayRemoveOldScalesets(cleanupCtx)
 }
 
 func (d *deployer) gatewayWaitForReadiness(ctx context.Context, vmssName string) error {
@@ -73,6 +76,12 @@ func (d *deployer) gatewayRemoveOldScalesets(ctx context.Context) error {
 }
 
 func (d *deployer) gatewayRemoveOldScaleset(ctx context.Context, vmssName string) error {
+	// Disable automatic repairs on the old VMSS to prevent race conditions during teardown
+	err := d.disableAutomaticRepairsOnVMSS(ctx, d.config.GatewayResourceGroupName, vmssName)
+	if err != nil {
+		return err
+	}
+
 	scalesetVMs, err := d.vmssvms.List(ctx, d.config.GatewayResourceGroupName, vmssName, "", "", "")
 	if err != nil {
 		return err
@@ -84,7 +93,7 @@ func (d *deployer) gatewayRemoveOldScaleset(ctx context.Context, vmssName string
 		if d.isVMInstanceHealthy(ctx, d.config.GatewayResourceGroupName, vmssName, *vm.InstanceID) {
 			d.log.Printf("stopping gateway service on %s", *vm.Name)
 			go func(id string) {
-				errors <- d.vmssvms.RunCommandAndWait(ctx, d.config.GatewayResourceGroupName, vmssName, id, mgmtcompute.RunCommandInput{
+				errors <- d.runCommandWithRetry(ctx, d.config.GatewayResourceGroupName, vmssName, id, mgmtcompute.RunCommandInput{
 					CommandID: pointerutils.ToPtr("RunShellScript"),
 					Script:    &[]string{"systemctl stop aro-gateway"},
 				})