From 619dfd05731c90f7eaf07d17aba28d8b919fa897 Mon Sep 17 00:00:00 2001 From: Jon Huhn Date: Tue, 19 Sep 2023 12:12:30 -0500 Subject: [PATCH] delete LRO state when operations fail --- azure/services/async/async.go | 22 +++++++----- azure/services/async/async_test.go | 58 +++++++++++++++++++++++++----- 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/azure/services/async/async.go b/azure/services/async/async.go index ba3d6f4286a..3a166ff3bf0 100644 --- a/azure/services/async/async.go +++ b/azure/services/async/async.go @@ -106,20 +106,23 @@ func (s *Service[C, D]) CreateOrUpdateResource(ctx context.Context, spec azure.R result, poller, err := s.Creator.CreateOrUpdateAsync(ctx, spec, resumeToken, parameters) errWrapped := errors.Wrapf(err, "failed to create or update resource %s/%s (service: %s)", rgName, resourceName, serviceName) - if poller != nil { + if poller != nil && azure.IsContextDeadlineExceededOrCanceledError(err) { future, err := converters.PollerToFuture(poller, infrav1.PutFuture, serviceName, resourceName, rgName) if err != nil { return nil, errWrapped } s.Scope.SetLongRunningOperationState(future) return nil, azure.WithTransientError(azure.NewOperationNotDoneError(future), requeueTime()) - } else if err != nil { - return nil, errWrapped } - // Once the operation is done, delete the long-running operation state. + // Once the operation is done, delete the long-running operation state. Even if the operation ended with + // an error, clear out any lingering state to try the operation again. s.Scope.DeleteLongRunningOperationState(resourceName, serviceName, futureType) + if err != nil { + return nil, errWrapped + } + log.V(2).Info("successfully created or updated resource", "service", serviceName, "resource", resourceName, "resourceGroup", rgName) return result, nil } @@ -147,20 +150,23 @@ func (s *Service[C, D]) DeleteResource(ctx context.Context, spec azure.ResourceS // Delete the resource. log.V(2).Info("deleting resource", "service", serviceName, "resource", resourceName, "resourceGroup", rgName) poller, err := s.Deleter.DeleteAsync(ctx, spec, resumeToken) - if poller != nil { + if poller != nil && azure.IsContextDeadlineExceededOrCanceledError(err) { future, err := converters.PollerToFuture(poller, infrav1.DeleteFuture, serviceName, resourceName, rgName) if err != nil { return errors.Wrap(err, "failed to convert poller to future") } s.Scope.SetLongRunningOperationState(future) return azure.WithTransientError(azure.NewOperationNotDoneError(future), requeueTime()) - } else if err != nil && !azure.ResourceNotFound(err) { - return errors.Wrapf(err, "failed to delete resource %s/%s (service: %s)", rgName, resourceName, serviceName) } - // Once the operation is done, delete the long-running operation state. + // Once the operation is done, delete the long-running operation state. Even if the operation ended with + // an error, clear out any lingering state to try the operation again. s.Scope.DeleteLongRunningOperationState(resourceName, serviceName, futureType) + if err != nil && !azure.ResourceNotFound(err) { + return errors.Wrapf(err, "failed to delete resource %s/%s (service: %s)", rgName, resourceName, serviceName) + } + log.V(2).Info("successfully deleted resource", "service", serviceName, "resource", resourceName, "resourceGroup", rgName) return nil } diff --git a/azure/services/async/async_test.go b/azure/services/async/async_test.go index cf49bb0f1e5..fdc5fbef90e 100644 --- a/azure/services/async/async_test.go +++ b/azure/services/async/async_test.go @@ -84,11 +84,25 @@ func TestServiceCreateOrUpdateResource(t *testing.T) { r.ResourceName().Return(resourceName), r.ResourceGroupName().Return(resourceGroupName), s.GetLongRunningOperationState(resourceName, serviceName, infrav1.PutFuture).Return(validPutFuture), - c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), resumeToken, gomock.Any()).Return(nil, fakePoller[MockCreator](g, http.StatusAccepted), nil), + c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), resumeToken, gomock.Any()).Return(nil, fakePoller[MockCreator](g, http.StatusAccepted), context.DeadlineExceeded), s.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})), ) }, }, + { + name: "operation failed", + serviceName: serviceName, + expectedError: "failed to create or update resource mock-resourcegroup/mock-resource (service: mock-service): foo", + expect: func(g *WithT, s *mock_async.MockFutureScopeMockRecorder, c *mock_async.MockCreatorMockRecorder[MockCreator], r *mock_azure.MockResourceSpecGetterMockRecorder) { + gomock.InOrder( + r.ResourceName().Return(resourceName), + r.ResourceGroupName().Return(resourceGroupName), + s.GetLongRunningOperationState(resourceName, serviceName, infrav1.PutFuture).Return(validPutFuture), + c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), resumeToken, gomock.Any()).Return(nil, fakePoller[MockCreator](g, http.StatusAccepted), errors.New("foo")), + s.DeleteLongRunningOperationState(resourceName, serviceName, infrav1.PutFuture), + ) + }, + }, { name: "get returns resource not found error", serviceName: serviceName, @@ -100,7 +114,7 @@ func TestServiceCreateOrUpdateResource(t *testing.T) { s.GetLongRunningOperationState(resourceName, serviceName, infrav1.PutFuture).Return(nil), c.Get(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType)).Return(nil, &azcore.ResponseError{StatusCode: http.StatusNotFound}), r.Parameters(gomockinternal.AContext(), nil).Return(fakeParameters, nil), - c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), "", gomock.Any()).Return(nil, fakePoller[MockCreator](g, http.StatusAccepted), nil), + c.CreateOrUpdateAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), "", gomock.Any()).Return(nil, fakePoller[MockCreator](g, http.StatusAccepted), context.DeadlineExceeded), s.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})), ) }, @@ -187,13 +201,13 @@ func TestServiceDeleteResource(t *testing.T) { serviceName string expectedError string expectedResult interface{} - expect func(s *mock_async.MockFutureScopeMockRecorder, d *mock_async.MockDeleterMockRecorder[MockDeleter], r *mock_azure.MockResourceSpecGetterMockRecorder) + expect func(g *GomegaWithT, s *mock_async.MockFutureScopeMockRecorder, d *mock_async.MockDeleterMockRecorder[MockDeleter], r *mock_azure.MockResourceSpecGetterMockRecorder) }{ { name: "invalid future", serviceName: serviceName, expectedError: "could not decode future data", - expect: func(s *mock_async.MockFutureScopeMockRecorder, d *mock_async.MockDeleterMockRecorder[MockDeleter], r *mock_azure.MockResourceSpecGetterMockRecorder) { + expect: func(_ *GomegaWithT, s *mock_async.MockFutureScopeMockRecorder, _ *mock_async.MockDeleterMockRecorder[MockDeleter], r *mock_azure.MockResourceSpecGetterMockRecorder) { gomock.InOrder( r.ResourceName().Return(resourceName), r.ResourceGroupName().Return(resourceGroupName), @@ -203,15 +217,43 @@ func TestServiceDeleteResource(t *testing.T) { }, }, { - name: "valid future", + name: "operation in progress", + serviceName: serviceName, + expectedError: "operation type DELETE on Azure resource mock-resourcegroup/mock-resource is not done. Object will be requeued after 15s", + expect: func(g *GomegaWithT, s *mock_async.MockFutureScopeMockRecorder, d *mock_async.MockDeleterMockRecorder[MockDeleter], r *mock_azure.MockResourceSpecGetterMockRecorder) { + gomock.InOrder( + r.ResourceName().Return(resourceName), + r.ResourceGroupName().Return(resourceGroupName), + s.GetLongRunningOperationState(resourceName, serviceName, infrav1.DeleteFuture).Return(validDeleteFuture), + d.DeleteAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), gomock.Any()).Return(fakePoller[MockDeleter](g, http.StatusAccepted), context.DeadlineExceeded), + s.SetLongRunningOperationState(gomock.AssignableToTypeOf(&infrav1.Future{})), + ) + }, + }, + { + name: "operation succeeds", serviceName: serviceName, expectedError: "", - expect: func(s *mock_async.MockFutureScopeMockRecorder, d *mock_async.MockDeleterMockRecorder[MockDeleter], r *mock_azure.MockResourceSpecGetterMockRecorder) { + expect: func(_ *GomegaWithT, s *mock_async.MockFutureScopeMockRecorder, d *mock_async.MockDeleterMockRecorder[MockDeleter], r *mock_azure.MockResourceSpecGetterMockRecorder) { + gomock.InOrder( + r.ResourceName().Return(resourceName), + r.ResourceGroupName().Return(resourceGroupName), + s.GetLongRunningOperationState(resourceName, serviceName, infrav1.DeleteFuture).Return(validDeleteFuture), + d.DeleteAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), gomock.Any()).Return(nil, nil), + s.DeleteLongRunningOperationState(resourceName, serviceName, infrav1.DeleteFuture), + ) + }, + }, + { + name: "operation fails", + serviceName: serviceName, + expectedError: "failed to delete resource mock-resourcegroup/mock-resource (service: mock-service): foo", + expect: func(g *GomegaWithT, s *mock_async.MockFutureScopeMockRecorder, d *mock_async.MockDeleterMockRecorder[MockDeleter], r *mock_azure.MockResourceSpecGetterMockRecorder) { gomock.InOrder( r.ResourceName().Return(resourceName), r.ResourceGroupName().Return(resourceGroupName), s.GetLongRunningOperationState(resourceName, serviceName, infrav1.DeleteFuture).Return(validDeleteFuture), - d.DeleteAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), gomock.Any()), + d.DeleteAsync(gomockinternal.AContext(), gomock.AssignableToTypeOf(azureResourceGetterType), gomock.Any()).Return(fakePoller[MockDeleter](g, http.StatusAccepted), errors.New("foo")), s.DeleteLongRunningOperationState(resourceName, serviceName, infrav1.DeleteFuture), ) }, @@ -231,7 +273,7 @@ func TestServiceDeleteResource(t *testing.T) { svc := New[MockCreator, MockDeleter](scopeMock, nil, deleterMock) specMock := mock_azure.NewMockResourceSpecGetter(mockCtrl) - tc.expect(scopeMock.EXPECT(), deleterMock.EXPECT(), specMock.EXPECT()) + tc.expect(g, scopeMock.EXPECT(), deleterMock.EXPECT(), specMock.EXPECT()) err := svc.DeleteResource(context.TODO(), specMock, tc.serviceName) if tc.expectedError != "" {