Skip to content

Conversation

kumargauravsharma
Copy link
Contributor

Issue #, if available:

Replication group delete takes some time to delete while kubectl delete -f replication_group.yaml removes the custom resource record from cluster.

Description of changes:

This PR adds logic inside sdk::sdkDelete() to requeue reconcile while replication group is deleting.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-bot ack-bot requested review from jaypipes and nmvk June 15, 2021 07:39
@kumargauravsharma kumargauravsharma requested review from echen-98, jaypipes and nmvk and removed request for jaypipes and nmvk June 15, 2021 07:39
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great @kumargauravsharma but I think you may have forgotten to git add a file...? :)

}
status := *r.ko.Status.Status
return status == "deleting"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -0,0 +1,4 @@
if isDeleting(r) {
return requeueWaitWhileDeleting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you need to git add a conditions.go file? :) I don't see where requeueWaitWhileDeleting is defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requeueWaitWhileDeleting is defined in hooks.go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doh! sorry, not sure how I missed that :)

path: Events
AuthToken:
is_secret: true
hooks:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure but maybe you need a sdk_find_post_set_output hook as well? (similar to dk_delete_pre_build_request.go.tpl)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not needed.
I have a upcoming separate PR to patch resource which is being deleted, so that kubectl describe provides latest state of the resource as it gets deleted.
But that is independent of this PR.

generator.yaml Outdated
sdk_delete_pre_build_request:
template_path: hooks/sdk_delete_pre_build_request.go.tpl
sdk_delete_post_request:
template_path: hooks/sdk_delete_post_request.go.tpl
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template_path: hooks/sdk_delete_post_request.go.tpl
template_path: hooks/sdk_delete_post_request.go.tpl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the extra space.

@nmvk
Copy link
Contributor

nmvk commented Jun 15, 2021

/approve

@ack-bot
Copy link
Collaborator

ack-bot commented Jun 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: echen-98, jaypipes, kumargauravsharma, nmvk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [echen-98,jaypipes,kumargauravsharma,nmvk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -0,0 +1,8 @@
if respErr == nil {
if foundResource, err := rm.sdkFind(ctx, r); err != ackerr.NotFound {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a DELETE operation on ReplicationGroup has returned no error, doesn't this mean that the resource will always be in a deleting status? Is it necessary to do another sdkFind() operation here?

Further, if a DELETE operation on ReplicationGroup returns no error, is there any chance that the ReplicationGroup can end up not being deleted in the backend Elasticache service? If there is no chance of this, is it really necessary to do anything here other than return nil and let the elasticache-controller reconcile the resource and remove its finalizer in cleanup() and get rid of the CR on the k8s side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants