Add RetainOnDelete resource option #8746

Frassle · 2022-01-13T11:46:23Z

Description

Adds a new resource option "RetainOnDelete" which if true skips calling into the provider to actually delete the resource. Instead the engine simply drops it from our state file (or replaces it in the state file in the case of replaces). Users must be aware that this "leaks" cloud resources as Pulumi is no longer tracking them, but that is the whole point of this option.

Fixes #7747 & #8450

Checklist

I have added tests that prove my fix is effective or that my feature works

I have updated the CHANGELOG-PENDING file with my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

Frassle · 2022-01-13T11:50:18Z

CHANGELOG_PENDING.md

@@ -1,5 +1,7 @@
 ### Improvements

+- [sdk] - Add `RetainOnDelete` as a resource option.


Open to name suggestions if anyone thinks RetainOnDelete isn't clear enough. Another idea was "OrphanOnDelete".

I prefer RetainOnDelete over OrphanOnDelete.

Note this also aligns with CloudFormation’s similar feature: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-attribute-deletionpolicy.html. ARM and Terraform don’t have this feature.

Were there other candidate options we should consider?

Frassle · 2022-01-13T11:51:12Z

pkg/resource/deploy/import.go

@@ -166,7 +166,7 @@ func (i *importer) getOrCreateStackResource(ctx context.Context) (resource.URN,
 	typ, name := resource.RootStackType, fmt.Sprintf("%s-%s", projectName, stackName)
 	urn := resource.NewURN(stackName, projectName, "", typ, tokens.QName(name))
 	state := resource.NewState(typ, urn, false, false, "", resource.PropertyMap{}, nil, "", false, false, nil, nil, "",
-		nil, false, nil, nil, nil, "")
+		nil, false, nil, nil, nil, "", false)


Should probably add a way to set retainOnDelete for imports, does that make sense?

Frassle · 2022-01-13T15:17:11Z

Further thinking about this I'm wondering if this should be more closely related to our current External concept? Maybe that should be a tristate of "managed", "external", "managed but used elsewhere so don't delete"?

codecov · 2022-01-13T17:34:06Z

Codecov Report

Merging #8746 (205759c) into master (6a8f7be) will decrease coverage by 0.01%.
The diff coverage is 69.23%.

❗ Current head 205759c differs from pull request most recent head 2feb6b6. Consider uploading reports for the commit 2feb6b6 to get more accurate results

@@            Coverage Diff             @@
##           master    #8746      +/-   ##
==========================================
- Coverage   59.36%   59.34%   -0.02%     
==========================================
  Files         643      637       -6     
  Lines       99898    97776    -2122     
  Branches     1420     1387      -33     
==========================================
- Hits        59309    58030    -1279     
+ Misses      37206    36468     -738     
+ Partials     3383     3278     -105

Impacted Files	Coverage Δ
pkg/backend/snapshot.go	`83.03% <0.00%> (-0.85%)`	⬇️
...k/dotnet/Pulumi/Resources/ResourceOptions_Merge.cs	`0.00% <0.00%> (-100.00%)`	⬇️
sdk/go/common/apitype/core.go	`100.00% <ø> (ø)`
sdk/go/pulumi/resource.go	`80.92% <0.00%> (-1.71%)`	⬇️
sdk/nodejs/resource.ts	`30.31% <ø> (-1.71%)`	⬇️
...dk/python/lib/pulumi/runtime/proto/resource_pb2.py	`100.00% <ø> (ø)`
sdk/python/lib/pulumi/runtime/resource.py	`44.95% <ø> (+0.86%)`	⬆️
sdk/nodejs/proto/resource_pb.js	`40.30% <50.00%> (+0.10%)`	⬆️
sdk/python/lib/pulumi/resource.py	`49.73% <66.66%> (-0.53%)`	⬇️
sdk/proto/go/resource.pb.go	`49.20% <80.00%> (+0.24%)`	⬆️
... and 190 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a8f7be...2feb6b6. Read the comment docs.

t0yv0 · 2022-01-24T21:43:10Z

Will be reviewing this tomorrow. There's a bit of conflict that crept in also FYI.

Frassle · 2022-01-24T22:28:11Z

Will be reviewing this tomorrow. There's a bit of conflict that crept in also FYI.

I'm going to see about working in @AaronFriel idea of different delete behaviors so probably makes sense to hold off for now. I'll give you a nudge when I think it's ready again.

Frassle · 2022-02-11T09:37:11Z

pkg/resource/deploy/step.go

+	} else if s.old.DeleteBehaviour == resource.DeleteBehaviourDrop {
+		// Deleting a "drop on delete" is a no-op as the user has explicitly asked us to not delete the resource.
+		// But we want to diag warn that Pulumi has stopped tracking this resource
+		msg := fmt.Sprintf("Reference to %s dropped, Pulumi has not deleted this resource", s.old.ID)


Open to better wording on this message. Should it be even more clear about the cost implications?

Yup this may be a bit tricky. "Pulumi dropped the reference to %s is no longer tracking it, but it remains provisioned in the cloud"? Except it's not necessarily cloud. "remains provisioned?"

Frassle · 2022-02-11T09:42:57Z

sdk/go/common/resource/delete_behaviour.go

+const (
+	DeleteBehaviourDelete  DeleteBehaviour = DeleteBehaviour(pulumirpc.DeleteBehaviour_DELETE)
+	DeleteBehaviourDrop                    = DeleteBehaviour(pulumirpc.DeleteBehaviour_DROP)
+	DeleteBehaviourProtect                 = DeleteBehaviour(pulumirpc.DeleteBehaviour_PROTECT)


Protect here is the same as the protect flag. We could with 4.0 remove the protect bool flag and just use this. Also scope here to add a new Protect mode that doesn't even allow replacement (the current protect will allow resources to be replaced, just not fully deleted). Then again maybe we should split this behavior flag into DeleteBehaviour (i.e. what to do for a Delete) and ReplaceBehaviour (what to do for a Replace)? Like would it be valid to say delete replacements but drop the final delete? Protect against replacements but allow the final delete?

Ah so it's exact same. Er, should we just use protect flag? I'm a bit too conservative perhaps - use what's out there already without changing.

mikhailshilkov · 2022-02-11T14:54:05Z

Let's not merge this until we finalize the discussion in #7747. Let me know if I need to help drive to conclusion.

t0yv0 · 2022-02-15T18:29:38Z

pkg/resource/deploy/step.go

@@ -337,26 +337,34 @@ func (s *DeleteStep) Logical() bool           { return !s.replacing }
 func (s *DeleteStep) Apply(preview bool) (resource.Status, StepCompleteFunc, error) {
 	// Refuse to delete protected resources (unless we're replacing them in
 	// which case we will of checked protect elsewhere)
-	if !s.replacing && s.old.Protect {
+	if !s.replacing && (s.old.Protect || s.old.DeleteBehaviour == resource.DeleteBehaviourProtect) {


Repeating a qn here - Protect and DeleteBehaviorProtect seem awfully similar, are they different somehow that we want both?

t0yv0 · 2022-02-15T18:30:23Z

pkg/resource/deploy/step.go

@@ -337,26 +337,34 @@ func (s *DeleteStep) Logical() bool           { return !s.replacing }
 func (s *DeleteStep) Apply(preview bool) (resource.Status, StepCompleteFunc, error) {
 	// Refuse to delete protected resources (unless we're replacing them in
 	// which case we will of checked protect elsewhere)
-	if !s.replacing && s.old.Protect {
+	if !s.replacing && (s.old.Protect || s.old.DeleteBehaviour == resource.DeleteBehaviourProtect) {
 		return resource.StatusOK, nil, fmt.Errorf("unable to delete resource %q\n"+
 			"as it is currently marked for protection. To unprotect the resource, "+


Does the message to "remove protect flag" still apply if the cause is DeleteBehaviorProtect?

t0yv0 · 2022-02-15T18:36:45Z

our current External concept? Maybe that should be a tristate of "managed", "external", "managed but used elsewhere so don't delete"?

Any docs refs on this concept?

t0yv0 · 2022-02-15T18:37:47Z

Looked through the code, implementation is fine, just wondering about the UX on these option design, left some questions.

t0yv0 · 2022-02-16T16:56:08Z

sdk/python/lib/pulumi/resource.py

@@ -482,6 +482,11 @@ class ResourceOptions:
    require replacement instead of update only if `"*"` is passed.
    """

+    retain_on_delete: Optional[bool]
+    """
+    Option to not actually delete resources from providers when the engine calls Delete.


Nit: try to explaining this in terms of what the user gets not how it's implemented. Taking a shot at it:

"If set to True, deletions retain the resource it the cloud instead of deleting it. Pulumi simply stops tracking the deleted resource."

t0yv0 · 2022-02-16T17:02:44Z

pkg/resource/stack/deployment.go

@@ -322,6 +322,7 @@ func SerializeResource(res *resource.State, enc config.Encrypter, showSecrets bo
 		Aliases:                 res.Aliases,
 		ImportID:                res.ImportID,
 		SequenceNumber:          res.SequenceNumber,
+		RetainOnDelete:          res.RetainOnDelete,


So.. this means the flag is to be stored in the state file correct?

Has to be. Does mean if you deploy with an old CLI it will reset back to false, but we can't get this information from anywhere else for Deletes.

Hmm okay I'd think for downgrading CLI we could potentially even accept "undefined behavior" here, but your'e saying even then we still need it stored here.

Also curious for my understanding - when we extend ResourceV3, the state backends like "service" just store the extra data, the schema is fully controlled by the code here? We don't need more changes?

t0yv0

LGTM with nits and final question on whether statefile persist of the flag should be avoided or not (see the ticket).

* Plumb in basics of retainOnDelete * Add test * Make test pass * Add to changelog * Add to API list * lint * Add semicolon * Fix Infof call * Fix method call * new delete mode work * cleanup * protectTest * Fix up test * Fix replace * Fix up test * Warn on drop * lint * Change to just a bool flag * Regenerate proto * Rework to just a bool flag with no error * Remove old comment * Fix C# typo * rm extra space * Add missing semicolon * Reformat python * False typo * Fix typo in js function name * Reword docs * lint * Read doesn't need retainOnDelete

Frassle added 5 commits January 13, 2022 11:17

Plumb in basics of retainOnDelete

e1c504e

Add test

741626a

Make test pass

fc35db8

Merge remote-tracking branch 'origin/master' into retainOnDelete

b9e58bc

Add to changelog

586ac85

Frassle changed the title ~~Fraser/retain on delete~~ Add RetainOnDelete resource option Jan 13, 2022

Frassle commented Jan 13, 2022

View reviewed changes

Frassle added 5 commits January 13, 2022 15:26

Add to API list

df94180

lint

0de44a4

Add semicolon

d4e1787

Fix Infof call

92215eb

Fix method call

205759c

t0yv0 self-requested a review January 19, 2022 20:53

Frassle added 4 commits January 20, 2022 22:35

Merge remote-tracking branch 'origin/master' into retainOnDelete

df2b15d

new delete mode work

aff89af

cleanup

f62ef65

protectTest

9b0ae39

Frassle added 5 commits February 10, 2022 10:47

Merge remote-tracking branch 'origin/master' into retainOnDelete

6f1ac52

Fix up test

95a5481

Fix replace

f254bc2

Fix up test

aad928a

Warn on drop

f79e5bd

Frassle commented Feb 11, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into retainOnDelete

da4fbf8

Frassle marked this pull request as ready for review February 11, 2022 09:55

lint

0aea245

t0yv0 reviewed Feb 15, 2022

View reviewed changes

lukehoban mentioned this pull request Feb 15, 2022

Add RetainOnDelete resource option #7747

Closed

Frassle added 11 commits February 15, 2022 22:59

Change to just a bool flag

2f9a39c

Regenerate proto

a7eb0ab

Rework to just a bool flag with no error

8fbb146

Merge remote-tracking branch 'origin/master' into retainOnDelete

2e0c8a2

Remove old comment

144a665

Fix C# typo

a576eef

rm extra space

a7629dd

Add missing semicolon

4995a1d

Reformat python

72c87e7

False typo

ac6b8c0

Fix typo in js function name

2feb6b6

t0yv0 reviewed Feb 16, 2022

View reviewed changes

t0yv0 approved these changes Feb 16, 2022

View reviewed changes

Frassle added 4 commits February 16, 2022 17:27

Reword docs

0849626

lint

c9a0d94

Merge branch 'master' into fraser/retainOnDelete

01b2f1d

Read doesn't need retainOnDelete

2f1870f

Frassle merged commit 5d91f8f into master Feb 16, 2022

pulumi-bot deleted the fraser/retainOnDelete branch February 16, 2022 22:11

AaronFriel mentioned this pull request Mar 22, 2022

Implement RetainOnDelete resource option pulumi/pulumi-yaml#124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RetainOnDelete resource option #8746

Add RetainOnDelete resource option #8746

Frassle commented Jan 13, 2022 •

edited

Loading

Frassle Jan 13, 2022

lukehoban Feb 16, 2022

Frassle Jan 13, 2022

Frassle commented Jan 13, 2022

codecov bot commented Jan 13, 2022 •

edited

Loading

t0yv0 commented Jan 24, 2022

Frassle commented Jan 24, 2022

Frassle Feb 11, 2022

t0yv0 Feb 15, 2022

Frassle Feb 11, 2022

t0yv0 Feb 15, 2022

mikhailshilkov commented Feb 11, 2022

t0yv0 Feb 15, 2022

t0yv0 Feb 15, 2022

t0yv0 commented Feb 15, 2022

t0yv0 commented Feb 15, 2022

t0yv0 Feb 16, 2022

t0yv0 Feb 16, 2022

Frassle Feb 16, 2022

t0yv0 Feb 16, 2022

t0yv0 left a comment

		@@ -1,5 +1,7 @@
		### Improvements

		- [sdk] - Add `RetainOnDelete` as a resource option.

Add RetainOnDelete resource option #8746

Add RetainOnDelete resource option #8746

Conversation

Frassle commented Jan 13, 2022 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frassle commented Jan 13, 2022

codecov bot commented Jan 13, 2022 • edited Loading

Codecov Report

t0yv0 commented Jan 24, 2022

Frassle commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhailshilkov commented Feb 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Feb 15, 2022

t0yv0 commented Feb 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 left a comment

Choose a reason for hiding this comment

Frassle commented Jan 13, 2022 •

edited

Loading

codecov bot commented Jan 13, 2022 •

edited

Loading