Skip to content

Commit

Permalink
Merge pull request #4011 from nojnhuh/async-done-poller
Browse files Browse the repository at this point in the history 
delete LRO state when operations fail
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Sep 27, 2023
2 parents 82a01b3 + 619dfd0 commit 3a40dd4
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 16 deletions.
22 changes: 14 additions & 8 deletions azure/services/async/async.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
58 changes: 50 additions & 8 deletions azure/services/async/async_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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{})),
)
},
Expand Down Expand Up @@ -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),
Expand All @@ -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),
)
},
Expand All @@ -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 != "" {
Expand Down

0 comments on commit 3a40dd4

Please sign in to comment.