pkg/cluster: extend retryable coverage to deployments and private endpoint delete

Author: ventifus

pkg/cluster/clusterserviceprincipal.go

@@ -54,10 +54,10 @@ func (m *manager) createOrUpdateClusterServicePrincipalRBAC(ctx context.Context)
 
 	for _, assignment := range toDelete {
 		m.log.Infof("deleting role assignment %s", *assignment.Name)
-		err := m.retryableDelete("deleting role assignment "+*assignment.Name, func() error {
+		err := arm.RetryableDelete(func() error {
 			_, e := m.roleAssignments.Delete(ctx, *assignment.Scope, *assignment.Name)
 			return e
-		})
+		}, m.log, "deleting role assignment "+*assignment.Name)
 		if err != nil {
 			return err
 		}
@@ -75,7 +75,9 @@ func (m *manager) createOrUpdateClusterServicePrincipalRBAC(ctx context.Context)
 			ContentVersion: "1.0.0.0",
 			Resources:      []*arm.Resource{m.clusterServicePrincipalRBAC()},
 		}
-		err = arm.DeployTemplate(ctx, m.log, m.deployments, resourceGroup, "clustersp", t, nil)
+		err = arm.Retryable(func() error {
+			return arm.DeployTemplate(ctx, m.log, m.deployments, resourceGroup, "clustersp", t, nil)
+		}, m.log, "deploying cluster service principal RBAC")
 		if err != nil {
 			return err
 		}

pkg/cluster/delete.go

@@ -14,7 +14,7 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/wait"
 
-	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
+	armsdk "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6"
 	mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features"
@@ -25,6 +25,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/database/cosmosdb"
 	"github.com/Azure/ARO-RP/pkg/env"
+	"github.com/Azure/ARO-RP/pkg/util/arm"
 	"github.com/Azure/ARO-RP/pkg/util/azureclient"
 	"github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/azcertificates"
 	"github.com/Azure/ARO-RP/pkg/util/azureerrors"
@@ -58,16 +59,16 @@ func (m *manager) deleteNic(ctx context.Context, nicName string) error {
 
 	if *nic.Properties.ProvisioningState == armnetwork.ProvisioningStateFailed {
 		m.log.Printf("NIC '%s' is in a Failed provisioning state, attempting to reconcile prior to deletion.", *nic.ID)
-		err := m.armInterfaces.CreateOrUpdateAndWait(ctx, resourceGroup, *nic.Name, nic.Interface, nil)
+		err := arm.Retryable(func() error {
+			return m.armInterfaces.CreateOrUpdateAndWait(ctx, resourceGroup, *nic.Name, nic.Interface, nil)
+		}, m.log, "reconciling NIC "+nicName)
 		if err != nil {
 			return err
 		}
 	}
-	err = m.armInterfaces.DeleteAndWait(ctx, resourceGroup, *nic.Name, nil)
-	if azureerrors.IsNotFoundError(err) {
-		return nil
-	}
-	return err
+	return arm.RetryableDelete(func() error {
+		return m.armInterfaces.DeleteAndWait(ctx, resourceGroup, *nic.Name, nil)
+	}, m.log, "deleting NIC "+nicName)
 }
 
 func (m *manager) disconnectSecurityGroup(ctx context.Context, resourceID string) error {
@@ -90,7 +91,7 @@ func (m *manager) disconnectSecurityGroup(ctx context.Context, resourceID string
 		// Note: subnet only has value in the ID field,
 		// so we have to make another API request to get full subnet struct
 		// TODO: there is probably an undesirable race condition here - check if etags can help.
-		r, err := arm.ParseResourceID(*subnet.ID)
+		r, err := armsdk.ParseResourceID(*subnet.ID)
 		if err != nil {
 			return &api.CloudError{
 				StatusCode: http.StatusBadRequest,
@@ -130,7 +131,9 @@ func (m *manager) disconnectSecurityGroup(ctx context.Context, resourceID string
 		s.Properties.NetworkSecurityGroup = nil
 
 		m.log.Printf("disconnecting network security group from subnet %s", *s.ID)
-		err = m.armSubnets.CreateOrUpdateAndWait(ctx, r.ResourceGroupName, r.Parent.Name, r.Name, s, nil)
+		err = arm.Retryable(func() error {
+			return m.armSubnets.CreateOrUpdateAndWait(ctx, r.ResourceGroupName, r.Parent.Name, r.Name, s, nil)
+		}, m.log, "disconnecting network security group from subnet "+*s.ID)
 		if err != nil {
 			b, _ := json.Marshal(err)
 
@@ -238,11 +241,11 @@ func (m *manager) deleteResources(ctx context.Context) error {
 			}
 
 			var future mgmtfeatures.ResourcesDeleteByIDFuture
-			err = m.retryable("deleting "+*resource.ID, func() error {
+			err = arm.Retryable(func() error {
 				var deleteErr error
 				future, deleteErr = m.resources.DeleteByID(ctx, *resource.ID, apiVersion)
 				return deleteErr
-			})
+			}, m.log, "deleting "+*resource.ID)
 			if err != nil {
 				return deleteByIdCloudError(err)
 			}
@@ -309,10 +312,10 @@ func (m *manager) deleteRoleAssignments(ctx context.Context) error {
 		}
 
 		m.log.Infof("deleting role assignment %s", *assignment.Name)
-		err := m.retryableDelete("deleting role assignment "+*assignment.Name, func() error {
+		err := arm.RetryableDelete(func() error {
 			_, e := m.roleAssignments.Delete(ctx, *assignment.Scope, *assignment.Name)
 			return e
-		})
+		}, m.log, "deleting role assignment "+*assignment.Name)
 		if err != nil {
 			return err
 		}
@@ -337,10 +340,10 @@ func (m *manager) deleteRoleDefinition(ctx context.Context) error {
 		}
 
 		m.log.Infof("deleting role definition %s", *definition.Name)
-		err := m.retryableDelete("deleting role definition "+*definition.Name, func() error {
+		err := arm.RetryableDelete(func() error {
 			_, e := m.roleDefinitions.Delete(ctx, (*definition.AssignableScopes)[0], *definition.Name)
 			return e
-		})
+		}, m.log, "deleting role definition "+*definition.Name)
 		if err != nil {
 			return err
 		}
@@ -462,19 +465,16 @@ func (m *manager) deleteFederatedCredentials(ctx context.Context) error {
 				*federatedCredential.Properties.Issuer != string(*m.doc.OpenShiftCluster.Properties.ClusterProfile.OIDCIssuer):
 				continue
 			default:
-				_, err = m.clusterMsiFederatedIdentityCredentials.Delete(
-					ctx,
-					identityResourceId.ResourceGroup,
-					identityResourceId.ResourceName,
-					*federatedCredential.Name,
-					&armmsi.FederatedIdentityCredentialsClientDeleteOptions{},
-				)
-<<<<<<< HEAD
-=======
-				if azureerrors.IsNotFoundError(err) {
-					err = nil
-				}
->>>>>>> dbd78aace (fixup! pkg/cluster: wrap autorest ARM write call sites with transient retry)
+				err = arm.RetryableDelete(func() error {
+					_, e := m.clusterMsiFederatedIdentityCredentials.Delete(
+						ctx,
+						identityResourceId.ResourceGroup,
+						identityResourceId.ResourceName,
+						*federatedCredential.Name,
+						&armmsi.FederatedIdentityCredentialsClientDeleteOptions{},
+					)
+					return e
+				}, m.log, "deleting federated identity credential "+*federatedCredential.Name)
 				if err != nil {
 					if azureerrors.IsStatusNotFoundError(err) {
 						m.log.Infof("federated identity credentials not found for %s: %v", identity.ResourceID, err.Error())
@@ -528,9 +528,9 @@ func (m *manager) shouldDeleteResourceGroup(ctx context.Context, name string) (b
 }
 
 func (m *manager) deleteResourceGroup(ctx context.Context, name string) error {
-	err := m.retryable("deleting resource group "+name, func() error {
+	err := arm.Retryable(func() error {
 		return m.resourceGroups.DeleteAndWait(ctx, name)
-	})
+	}, m.log, "deleting resource group "+name)
 
 	detailedErr, isDetailedErr := err.(autorest.DetailedError)
 	if azureerrors.ResourceGroupNotFound(err) || (isDetailedErr && (detailedErr.StatusCode == http.StatusNotFound)) {
@@ -553,15 +553,10 @@ func (m *manager) Delete(ctx context.Context) error {
 	}
 
 	m.log.Print("deleting private endpoint")
-	err = m.armFPPrivateEndpoints.DeleteAndWait(ctx, m.env.ResourceGroup(), env.RPPrivateEndpointPrefix+m.doc.ID, nil)
-<<<<<<< HEAD
-	if err != nil && !azureerrors.IsNotFoundError(err) {
-=======
-	if azureerrors.IsNotFoundError(err) {
-		err = nil
-	}
+	err = arm.RetryableDelete(func() error {
+		return m.armFPPrivateEndpoints.DeleteAndWait(ctx, m.env.ResourceGroup(), env.RPPrivateEndpointPrefix+m.doc.ID, nil)
+	}, m.log, "deleting private endpoint "+env.RPPrivateEndpointPrefix+m.doc.ID)
 	if err != nil {
->>>>>>> dbd78aace (fixup! pkg/cluster: wrap autorest ARM write call sites with transient retry)
 		return err
 	}
 

pkg/cluster/delete_test.go

@@ -13,7 +13,6 @@ import (
 	"time"
 
 	"github.com/sirupsen/logrus"
-	logrustest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
@@ -30,6 +29,7 @@ import (
 	"github.com/Azure/msi-dataplane/pkg/dataplane"
 
 	"github.com/Azure/ARO-RP/pkg/api"
+	"github.com/Azure/ARO-RP/pkg/util/arm"
 	mock_armmsi "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/azuresdk/armmsi"
 	mock_armnetwork "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/azuresdk/armnetwork"
 	mock_azsecrets "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/azuresdk/azsecrets"
@@ -407,135 +407,61 @@ func TestRetryableDelete(t *testing.T) {
 	for _, tt := range []struct {
 		name    string
 		err     error
-		wantErr bool
+		wantErr error
 	}{
 		{
 			name:    "404 from inner f() is swallowed and nil returned",
 			err:     autorest.DetailedError{StatusCode: http.StatusNotFound},
-			wantErr: false,
+			wantErr: nil,
 		},
 		{
 			name:    "non-404 error propagates unchanged",
 			err:     autorest.DetailedError{StatusCode: http.StatusConflict},
-			wantErr: true,
+			wantErr: autorest.DetailedError{StatusCode: http.StatusConflict},
 		},
 		{
 			name:    "nil error propagates unchanged",
 			err:     nil,
-			wantErr: false,
+			wantErr: nil,
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
-			origBackoff := transientRetryBackoff
-			transientRetryBackoff = wait.Backoff{Steps: 1, Duration: time.Millisecond}
-			defer func() { transientRetryBackoff = origBackoff }()
+			origBackoff := arm.TransientBackoff
+			arm.TransientBackoff = wait.Backoff{Steps: 1, Duration: time.Millisecond}
+			defer func() { arm.TransientBackoff = origBackoff }()
 
-			m := &manager{log: logrus.NewEntry(logrus.StandardLogger())}
-			err := m.retryableDelete("test-op", func() error {
+			log := logrus.NewEntry(logrus.StandardLogger())
+			err := arm.RetryableDelete(func() error {
 				return tt.err
-			})
-			if tt.wantErr {
-				assert.Error(t, err)
-			} else {
-				assert.NoError(t, err)
-			}
+			}, log, "test-op")
+			assert.Equal(t, tt.wantErr, err)
 		})
 	}
 }
 
 func TestRetryableDeleteRetryPath(t *testing.T) {
-	origBackoff := transientRetryBackoff
-	transientRetryBackoff = wait.Backoff{Steps: 2, Duration: time.Millisecond, Factor: 2.0}
-	defer func() { transientRetryBackoff = origBackoff }()
+	origBackoff := arm.TransientBackoff
+	arm.TransientBackoff = wait.Backoff{Steps: 2, Duration: time.Millisecond, Factor: 2.0}
+	defer func() { arm.TransientBackoff = origBackoff }()
 
 	calls := 0
-	m := &manager{log: logrus.NewEntry(logrus.StandardLogger())}
-	err := m.retryableDelete("test-retry-op", func() error {
+	log := logrus.NewEntry(logrus.StandardLogger())
+	err := arm.RetryableDelete(func() error {
 		calls++
 		if calls == 1 {
 			return autorest.DetailedError{StatusCode: http.StatusTooManyRequests}
 		}
 		return nil
-	})
+	}, log, "test-retry-op")
 	require.NoError(t, err)
 	assert.Equal(t, 2, calls, "expected inner function to be called twice: once for transient error, once for success")
 }
 
-func TestIsRetryable(t *testing.T) {
-	for _, tt := range []struct {
-		name       string
-		err        error
-		wantRetry  bool
-		wantLogMsg string
-	}{
-		{
-			name:       "retryable 429 returns true and logs",
-			err:        autorest.DetailedError{StatusCode: http.StatusTooManyRequests},
-			wantRetry:  true,
-			wantLogMsg: "error on test-op, retrying:",
-		},
-		{
-			name:      "non-retryable error returns false",
-			err:       errors.New("permanent failure"),
-			wantRetry: false,
-		},
-		{
-			name:       "retryable azcore 429 returns true and logs",
-			err:        &azcore.ResponseError{StatusCode: http.StatusTooManyRequests},
-			wantRetry:  true,
-			wantLogMsg: "error on test-op, retrying:",
-		},
-		{
-			name: "retryable autorest 409+Retry-After returns true and logs",
-			err: autorest.DetailedError{
-				StatusCode: http.StatusConflict,
-				Response: &http.Response{
-					StatusCode: http.StatusConflict,
-					Header:     http.Header{"Retry-After": []string{"1"}},
-				},
-			},
-			wantRetry:  true,
-			wantLogMsg: "error on test-op, retrying:",
-		},
-		{
-			name: "retryable azcore 409+Retry-After returns true and logs",
-			err: &azcore.ResponseError{
-				StatusCode: http.StatusConflict,
-				RawResponse: &http.Response{
-					StatusCode: http.StatusConflict,
-					Header:     http.Header{"Retry-After": []string{"1"}},
-				},
-			},
-			wantRetry:  true,
-			wantLogMsg: "error on test-op, retrying:",
-		},
-	} {
-		t.Run(tt.name, func(t *testing.T) {
-			logger, hook := logrustest.NewNullLogger()
-			logger.SetLevel(logrus.WarnLevel)
-
-			m := &manager{log: logrus.NewEntry(logger)}
-			predicate := m.isRetryable("test-op")
-
-			got := predicate(tt.err)
-			assert.Equal(t, tt.wantRetry, got)
-
-			if tt.wantLogMsg != "" {
-				require.Len(t, hook.Entries, 1)
-				assert.Contains(t, hook.LastEntry().Message, tt.wantLogMsg)
-				assert.Equal(t, logrus.WarnLevel, hook.LastEntry().Level)
-			} else {
-				assert.Empty(t, hook.Entries)
-			}
-		})
-	}
-}
-
 // TestDisconnectSecurityGroupRetryExhausted verifies that when the azcore subnet call fails, the error is propagated.
 func TestDisconnectSecurityGroupRetryExhausted(t *testing.T) {
-	origBackoff := transientRetryBackoff
-	transientRetryBackoff = wait.Backoff{Steps: 1, Duration: time.Millisecond, Factor: 2.0} // Steps: 1 = 1 attempt, 0 retries
-	defer func() { transientRetryBackoff = origBackoff }()
+	origBackoff := arm.TransientBackoff
+	arm.TransientBackoff = wait.Backoff{Steps: 1, Duration: time.Millisecond, Factor: 2.0} // Steps: 1 = 1 attempt, 0 retries
+	defer func() { arm.TransientBackoff = origBackoff }()
 
 	subscription := "00000000-0000-0000-0000-000000000000"
 	resourceGroup := "test-rg"
@@ -1224,9 +1150,9 @@ func TestDeleteResourcesRetry(t *testing.T) {
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
-			origBackoff := transientRetryBackoff
-			transientRetryBackoff = wait.Backoff{Steps: 2, Duration: time.Millisecond, Factor: 2.0}
-			defer func() { transientRetryBackoff = origBackoff }()
+			origBackoff := arm.TransientBackoff
+			arm.TransientBackoff = wait.Backoff{Steps: 2, Duration: time.Millisecond, Factor: 2.0}
+			defer func() { arm.TransientBackoff = origBackoff }()
 
 			controller := gomock.NewController(t)
 			defer controller.Finish()
@@ -1270,9 +1196,9 @@ func TestDeleteResourcesRetry(t *testing.T) {
 // TestDeleteResourcesRetryExhausted verifies that retry exhaustion propagates the error.
 // Uses a single representative error (autorest 429); the exhaustion path is the same for all retryable errors.
 func TestDeleteResourcesRetryExhausted(t *testing.T) {
-	origBackoff := transientRetryBackoff
-	transientRetryBackoff = wait.Backoff{Steps: 1, Duration: time.Millisecond, Factor: 2.0} // Steps: 1 = 1 attempt, 0 retries
-	defer func() { transientRetryBackoff = origBackoff }()
+	origBackoff := arm.TransientBackoff
+	arm.TransientBackoff = wait.Backoff{Steps: 1, Duration: time.Millisecond, Factor: 2.0} // Steps: 1 = 1 attempt, 0 retries
+	defer func() { arm.TransientBackoff = origBackoff }()
 
 	subscription := "00000000-0000-0000-0000-000000000000"
 	resourceGroup := "test-rg"

pkg/cluster/denyassignment.go

@@ -52,5 +52,7 @@ func (m *manager) createOrUpdateDenyAssignment(ctx context.Context) error {
 		},
 	}
 
-	return arm.DeployTemplate(ctx, m.log, m.deployments, resourceGroup, "storage", t, nil)
+	return arm.Retryable(func() error {
+		return arm.DeployTemplate(ctx, m.log, m.deployments, resourceGroup, "storage", t, nil)
+	}, m.log, "deploying deny assignment")
 }