Skip to content

Commit 308bc45

Browse files
Returning the latest resource from the Delete() call (#21)
Related issue: aws-controllers-k8s/community#836 This PR contains code changes to patch custom resource with latest details during long running delete. It does so if resource manager's Delete function return requeue related errors. The expectation is that resource manager's ReadOne method would set appropriate conditions (example: `ACK.ResourceSynced` to false with message) and provide latest details about the resource which is being deleted. Conditions can also be set at some common place where common conditions logic should reside. Testing: `make test` passed for runtime. **Alternate approach** that was considered, but was not taken up as it involved non-intrusive changes to resource manager's APIs. Following are the details: Currently, [resource manager's `Delete` API](https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/pkg/resource/manager.go.tpl#L136) returns only one data field of type `error`, it does not return `resource`. Also, the [`sdk.go::sdkDelete()` logic](https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/pkg/resource/sdk.go.tpl#L141) does not parse the return result output of service delete API. It can be updated such that `sdk.go::sdkDelete()` logic parses the output response, and allows a `sdk_delete_post_set_output` hook for service controllers to have custom logic (as needed). The resultant resource reference would then be returned from this method. In case, requeue is required then the return values from this method will have both resource, error (requeue type) as not nil. And the [reconciler:cleanup() logic here](https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/runtime/reconciler.go#L327), would patch the current resource with the returned resource from `rm.Delete()` call and return the error (if any). Though this change is similar to the approach taken for [resource manager ReadOne here](https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/pkg/resource/manager.go.tpl#L76-L78), this is more intrusive as it changes the Delete API's return type. However, it does have advantage that there is no need for another ReadOne call from reconciler, and also provides flexibility to service controller's to implement custom hook specific to delete scenario. [EDIT]: the alternate approach that is mentioned above, is the final approach taken per the discussion/comments in this PR. Related PR in code-generator: aws-controllers-k8s/code-generator#114
1 parent 089321f commit 308bc45

File tree

3 files changed

+29
-11
lines changed

3 files changed

+29
-11
lines changed

mocks/pkg/types/aws_resource_manager.go

Lines changed: 14 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/runtime/reconciler.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -349,26 +349,34 @@ func (r *resourceReconciler) cleanup(
349349
defer exit(err)
350350

351351
rlog.Enter("rm.ReadOne")
352-
latest, err := rm.ReadOne(ctx, current)
352+
observed, err := rm.ReadOne(ctx, current)
353353
rlog.Exit("rm.ReadOne", err)
354354
if err != nil {
355355
if err == ackerr.NotFound {
356356
// If the aws resource is not found, remove finalizer
357-
return r.setResourceUnmanaged(ctx, latest)
357+
return r.setResourceUnmanaged(ctx, current)
358358
}
359359
return err
360360
}
361361
rlog.Enter("rm.Delete")
362-
err = rm.Delete(ctx, latest)
362+
latest, err := rm.Delete(ctx, observed)
363363
rlog.Exit("rm.Delete", err)
364+
if latest != nil {
365+
// The Delete operation is likely asynchronous and has likely set a Status
366+
// field on the returned CR to something like `deleting`. Here, we patchResource()
367+
// in order to save these Status field modifications.
368+
_ = r.patchResource(ctx, current, latest)
369+
}
364370
if err != nil {
371+
// NOTE: Delete() implementations that have asynchronously-completing
372+
// deletions should return a RequeueNeededAfter.
365373
return err
366374
}
367375

368376
// Now that external AWS service resources have been appropriately cleaned
369377
// up, we remove the finalizer representing the CR is managed by ACK,
370378
// allowing the CR to be deleted by the Kubernetes API server
371-
if err = r.setResourceUnmanaged(ctx, latest); err != nil {
379+
if err = r.setResourceUnmanaged(ctx, current); err != nil {
372380
return err
373381
}
374382
rlog.Info("deleted resource")

pkg/types/aws_resource_manager.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,9 @@ type AWSResourceManager interface {
5959
) (AWSResource, error)
6060

6161
// Delete attempts to destroy the supplied AWSResource in the backend AWS
62-
// service API.
63-
Delete(context.Context, AWSResource) error
62+
// service API, returning an AWSResource representing the
63+
// resource being deleted (if delete is asynchronous and takes time)
64+
Delete(context.Context, AWSResource) (AWSResource, error)
6465
// ARNFromName returns an AWS Resource Name from a given string name. This
6566
// is useful for constructing ARNs for APIs that require ARNs in their
6667
// GetAttributes operations but all we have (for new CRs at least) is a

0 commit comments

Comments
 (0)