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
Add DeletedWith
resource option
#11095
Conversation
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
Changelog[uncommitted] (2022-11-04)Features
|
DeletedWithParent
resource option
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
/run-acceptance-tests |
@same-id Thank you for making this PR, it's an impressive first time contribution! I've given you a shoutout on our internal slack and hope you can get in touch with Lee on the community slack so we can send some swag your way. At the same time, I'll confer with the team soon about how to proceed on this. We're usually pretty cautious about adding new resource options; if we have any changes in mind, we'll try to make pretty deliberate, specific suggestions. Looking forward to seeing this land - it might play a role in fixing one of the first bugs I filed against Pulumi in 2019 (pulumi/pulumi-gcp#268). |
We have an issue for this #10634. I'm not sure "parent" is the right relationship to be tracking this, it already is suffering a bit from confusing and overloaded semantics. My original thinking about this was that users wouldn't be able to control it at all, instead resource providers would just state the container relationships. On further thinking that probably won't suffice, if just because providers won't always be fully up to date and people will need local workarounds. But any option we do add should synchronize well with the idea that providers might also be setting things here, and as above I don't think it should have anything to do with the parent relationship. |
(Just noticed I switched from private user to work user, still me, same-id, with a different id ;-) )
Dor from our company (utila.io) opened it :-)
I was also not sure about using Parent for this feature since it binds the child resource to inherit properties (like transformations) that might not be relevant for "skip deletion" functionality. Another option was to use just a
I agree, probably we can model a Workaround might be to extend with Maybe even if resources are "skipping" deletion we would also want to communicate that to the user as a different operation: "delegated delete" instead of "delete"
I would love the overriding functionality hit master from one side since this solves an acute pain today. From the other hand I do understand that some changes deserve some time for discussion and to "sleep on it" - and then of course spend some time to pick the perfect option name :-) However I do think that I would hate to see this feature being dragged on out of existance. As I mentioned (in the TODO:) there are some edge cases like "Pending Deletions" + current deletions that we might also dwell on, or cases where the providers are changing: GCP provider -> Postgres Cloud SQL DB -> Postgres provider -> Role; deleting the Postgres Cloud SQL DB should save the deletion of the Role within it, even though they are handled by different providers. Maybe if we define this feature under non-stable API/ABI and release it under PULUMI_EXPERIMENTAL=1 (or a specific experimental flag, to avoid getting ALL experimental features) it will allow us to release this feature faster for people that like to live on the edge :-) This commit mainly shows that this functionality is easy to achieve with a relatively few changes, and might help set the discussion and the appetite for it. I would even argue that this type of feature would even be useful on a "best effort" basis -> worst case, you will do the "slow path", so the risk is minimal. Thanks! |
👍
Sure, I'm quite happy to just get a new resource option in now and we can internally iterate on later extending schemas and things to explain these relationships as well. The engine is not in place to be able to use that information yet, so it would just be wasted effort if done right now.
I think something like this is much more workable.
Is that likely? I've been trying to think of any examples where a resource has multiple containers, where if either any or all get deleted it also gets deleted but so far everything I've thought of only has one container. My worry with adding a
I think that will be needed. The engine still needs to do a state delete, and if we ever add lifecycle delete hooks they would still need to fire, so this isn't skipping the delete from the engines point of view, just from the providers point of view. |
We have precedent for this strategy. |
Tell me if you want me to resubmit the PR with that option (and maybe choose a different name) or whether you are going to implement this internally in Pulumi
I thought of OR relation only. If the role is deleted (need to make sure) then the IAM binding is deleted So: IAM-Binding { deletedWith: [NewRole, NewUser] } (no need to delete the IAM binding if role OR user is being deleted) But need to make sure.
|
If you can I'm happy to help with reviewing that and getting it in.
That sounds reasonable. So if either parent gets deleted the IAM binding would get deleted by aws as well so rather than us doing (Delete binding, Delete role) we can safely just do (Delete role) to get the same end result? Given that I think a |
As I mentioned in pulumi/pulumi-kubernetes#2211, Kubernetes is another example. There are, of course, some object that own cloud resources, like service with an external LB, storage volumes and various CRs (perhaps stacks managed by the Pulumi operator would be a good example along with CrossPlane and other similar solutions). The API will need to have an explicit opt-in with a default allow filter (so that user doesn't need to enumerate all the common resource types). |
Not sure about the name One more thing that concerns me is that I assume that you can't specify circular In the current implementation I used the Then none will get deleted. WDYT |
That's correct for Parents, and also for Dependencies.
It's not generally possible to define circular resources in pulumi programs anyway, but given that |
I understand, I was referring to my current implementation: Now, I skip deletion if Where If I change Then both deletions will be skipped, which is not intended. So there are 3 options:
|
Option three is clearly the ideal, but if it's overly tricky we could take option 2 for the first version of this feature. |
Maybe also introduce option number 4: (which is actually option number 0) Keep as it is. |
Yeh actually that's fine for the first pass at this. |
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
DeletedWithParent
resource optionDeletedWith
resource option
bors merge |
11095: Add `DeletedWith` resource option r=Frassle a=same-id # Description In many cases there is no need to delete resources if the container resource is going to be deleted as well. A few examples: * Database object (roles, tables) when database is being deleted * Cloud IAM bindings when user itself is being deleted This helps with: * Speeding the deletion process * Removing unnecessary calls to providers * Avoiding failed deletions when the pulumi user running the plan has access to the container resource but not the contained ones To avoid deleting contained resources, set the `DeletedWith` resource option to the container resource. TODO: Should we support DeletedWith with PendingDeletes? Special case might be when contained resource is marked as pending deletion but we now want to delete the container resource, so ultimately there is no need to delete the contained anymore <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> Fixes # (issue) ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. Not sure - maybe --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Sam Eiderman <sameid@gmail.com> Co-authored-by: Fraser Waters <fraser@pulumi.com>
Build failed: |
bors retry |
11095: Add `DeletedWith` resource option r=Frassle a=same-id # Description In many cases there is no need to delete resources if the container resource is going to be deleted as well. A few examples: * Database object (roles, tables) when database is being deleted * Cloud IAM bindings when user itself is being deleted This helps with: * Speeding the deletion process * Removing unnecessary calls to providers * Avoiding failed deletions when the pulumi user running the plan has access to the container resource but not the contained ones To avoid deleting contained resources, set the `DeletedWith` resource option to the container resource. TODO: Should we support DeletedWith with PendingDeletes? Special case might be when contained resource is marked as pending deletion but we now want to delete the container resource, so ultimately there is no need to delete the contained anymore <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> Fixes # (issue) ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. Not sure - maybe --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Sam Eiderman <sameid@gmail.com> Co-authored-by: Fraser Waters <fraser@pulumi.com>
Build failed: |
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
bors merge |
11095: Add `DeletedWith` resource option r=Frassle a=same-id # Description In many cases there is no need to delete resources if the container resource is going to be deleted as well. A few examples: * Database object (roles, tables) when database is being deleted * Cloud IAM bindings when user itself is being deleted This helps with: * Speeding the deletion process * Removing unnecessary calls to providers * Avoiding failed deletions when the pulumi user running the plan has access to the container resource but not the contained ones To avoid deleting contained resources, set the `DeletedWith` resource option to the container resource. TODO: Should we support DeletedWith with PendingDeletes? Special case might be when contained resource is marked as pending deletion but we now want to delete the container resource, so ultimately there is no need to delete the contained anymore <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> Fixes # (issue) ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. Not sure - maybe --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Sam Eiderman <sameid@gmail.com> Co-authored-by: Fraser Waters <fraser@pulumi.com>
Build failed: |
bors retry |
Build succeeded: |
Thanks for helping me take it to the finish line |
No worries :) Thanks for the substantial contribution! |
11306: [sdk/nodejs] Cleanup use of `asyncTest` and `assertAsyncThrows` r=justinvp a=justinvp The `asyncTest` and `assertAsyncThrows` helpers were necessary when originally written, but are no longer needed as Mocha has built-in support for testing async functions that return promises, and Node's `assert.rejects` can be used to assert whether a promise has been rejected. 11313: Regenerate .pyi files, sort diffs on proto cksum r=iwahbe a=iwahbe Regenerate the .pyi files as left from #11095. It's unclear to me why this didn't happen in the initial PR. The only thing I can think of is that .checksum.txt was manually edited. Co-authored-by: Justin Van Patten <jvp@justinvp.com> Co-authored-by: Ian Wahbe <ian@wahbe.com>
I think there's a small problem with the PR that we didn't flush out at testing. DeletedWith should be Can we "breaking change" this? |
We do plan on changing that, it will be a breaking change but it's likely no one is using this yet. |
11095: Add `DeletedWith` resource option r=Frassle a=same-id # Description In many cases there is no need to delete resources if the container resource is going to be deleted as well. A few examples: * Database object (roles, tables) when database is being deleted * Cloud IAM bindings when user itself is being deleted This helps with: * Speeding the deletion process * Removing unnecessary calls to providers * Avoiding failed deletions when the pulumi user running the plan has access to the container resource but not the contained ones To avoid deleting contained resources, set the `DeletedWith` resource option to the container resource. TODO: Should we support DeletedWith with PendingDeletes? Special case might be when contained resource is marked as pending deletion but we now want to delete the container resource, so ultimately there is no need to delete the contained anymore <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> Fixes # (issue) ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [x] I have added tests that prove my fix is effective or that my feature works <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. Not sure - maybe --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Sam Eiderman <sameid@gmail.com> Co-authored-by: Fraser Waters <fraser@pulumi.com>
11883: [sdk/{go,nodejs,python}] Fix `DeletedWith` resource option r=justinvp a=justinvp This change fixes the `DeletedWith` resource option in the Go, Node.js, and Python SDKs and adds tests. This feature was a community contribution and while there were engine tests included with the [original PR](#11095), there weren't any tests confirming the functionality worked correctly from each SDK. Here's a summary of the fixes: * Go: The `DeletedWith` resource option was never usable as it accepted a URN instead of a Resource. We discussed this internally a while back and decided to go ahead and fix this. (Note: While changing the signature is technically a breaking change, the feature is currently unusable, so the change would not break anyone, so there's no need to wait for a major version bump.) * Node.js: The `deletedWith` resource option did not work at all from the Node.js SDK because it was incorrectly passing the resource object itself in the RegisterResource request, rather than the resource's URN. * Python: The `deleted_with` resource option did not work at all from the Python SDK because it was incorrectly passing the resource object itself in the RegisterResource request, rather than the resource's URN. A `FailsOnDelete` resource has been added to the testprovider, which will fail when its `Delete` gRPC is called. The tests use this to ensure `Delete` is not called for resources of this type with the `DeletedWith` option specified. Fixes #11358 Fixes #11622 Co-authored-by: Justin Van Patten <jvp@justinvp.com>
Description
In many cases there is no need to delete resources if the container resource is going to be deleted as well.
A few examples:
This helps with:
To avoid deleting contained resources, set the
DeletedWith
resource option to the container resource.TODO:
Should we support DeletedWith with PendingDeletes?
Special case might be when contained resource is marked as pending deletion
but we now want to delete the container resource, so ultimately there is no
need to delete the contained anymore
Fixes # (issue)
Checklist
make changelog
and committed thechangelog/pending/<file>
documenting my change