Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't resolve references when being deleted #75

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

negz
Copy link
Member

@negz negz commented Nov 5, 2019

Description of your changes

Fixes #74 (at least, fixes it enough for now)

Per the comment, there's too high a chance we'll get stuck and not process the delete because we reference resources that are also being deleted. I considered:

  • Processing references at delete time but not blocking on accessor errors. I felt this introduced too much complexity for little gain compared to just not trying resolution at all.
  • Making AttributeReferencers no-ops when the field they would set already had a value. I think this is an avenue we should investigate (issue forthcoming) but it is awkward to implement with the current AttributeReferencer interface.
  • Enforcing ordered deletes, such that a referenced resource cannot be deleted (at least not by Crossplane) until its referencers have all been deleted. This is the most bulletproof, but also the most complicated solution and would require further design investigation to pursue.

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation, examples, or release notes.
  • Updated the RBAC permissions in clusterrole.yaml to include any new types.

Per the comment, there's too high a chance we'll get stuck and not process the
delete because we reference resources that are also being deleted. I considered:

* Processing references at delete time but not blocking on accessor errors. I
  felt this introduced too much complexity for little gain compared to just not
  trying resolution at all.
* Making AttributeReferencers no-ops when the field they would set already had a
  value. I think this is an avenue we should investigate (issue forthcoming) but
  it is awkward to implement with the current AttributeReferencer interface.
* Enforcing ordered deletes, such that a referenced resource cannot be deleted
  (at least not by Crossplane) until its referencers have all been deleted. This
  is the most bulletproof, but also the most complicated solution and would
  require further design investigation to pursue.

Signed-off-by: Nic Cope <negz@rk0n.org>
@upbound-bot
Copy link
Collaborator

92% (0.0%) vs master 92%

@negz
Copy link
Member Author

negz commented Nov 5, 2019

I tested this by using the stack-gcp release branch with a replace statement in my go.mod to this branch, and everything seems to work as expected. I created stuff out of order (i.e. subnet before network) and confirmed they blocked until references were resolvable, then deleted stuff (network, subnetwork, globaladdress, gkecluster, cloudsqlinstance, etc) in random order and confirmed they were all (eventually) deleted successfully.

Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

This seems like a good fix for getting v0.2.0 out the door! In regards to the third proposed option, it may also be worth investigating if, in some cases, deleting a resource should cause deletion of a set of its referenced resources. I am mostly thinking of a Network / Subnetwork scenario where the act of deleting the Network could be deemed to be implicitly saying that all Subnetworks within should be deleted as well (this is a common experience in cloud provider consoles). This behavior seems to potentially overlap with side-effect resource creation as well.

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

Successfully merging this pull request may close these issues.

References to deleting managed resources blocks deletion.
3 participants