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

Implement the --exclude-protected feature #8359

Merged
merged 18 commits into from
Nov 15, 2021

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Nov 5, 2021

This piggybacks on the same machinery used by the --target flag. By
examining the stack, we find a list of all resources managed by
Pulumi (in that stack). We then form them into a DAG, and mark all
resources as either protected or unprotected.

A resource is protected it has the Protect flag set, is has a child
that is protected or has a dependency that is protected. It is unprotected otherwise.

We then pass the urns of unprotected resources to the update options
passed to the destroy operation in the same way that --target does.

Description

Fixes #6539

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

This piggybacks on the same machinery used by the --target flag. By
examining the stack, we find a list of all resources managed by
Pulumi (in that stack). We then form them into a DAG, and mark all
resources as either protected or unprotected.

A resource is protected it has the `Protect` flag set or is has a child
with the `protect` flag set. It is unprotected otherwise.

We then pass the urns of unprotected resources to the update options
passed to the destroy operation in the same way that `--target` does.
@iwahbe iwahbe added the area/cli UX of using the CLI (args, output, logs) label Nov 5, 2021
@iwahbe iwahbe self-assigned this Nov 5, 2021
@iwahbe iwahbe marked this pull request as draft November 5, 2021 16:27
@iwahbe iwahbe marked this pull request as ready for review November 5, 2021 19:12
@iwahbe iwahbe requested a review from a team November 5, 2021 19:12
@pgavlin
Copy link
Member

pgavlin commented Nov 5, 2021

A resource is protected it has the Protect flag set or is has a child
with the protect flag set. It is unprotected otherwise.

Do we also treat resources that are dependencies of protected resources as protected? It feels like we'll need to, as we can't destroy those resources w/o first destroying the protected resources.

@iwahbe
Copy link
Member Author

iwahbe commented Nov 5, 2021

Do we also treat resources that are dependencies of protected resources as protected? It feels like we'll need to, as we can't destroy those resources w/o first destroying the protected resources.

We currently allow destroying dependencies of protected resources (we don't mark them as protected). My understanding was that dependencies encode only the data necessary to create a resource. I investigated after your comment, and found that this created some errors with invalid snapshots.

error: post-step event returned an error: failed to verify snapshot: resource urn:pulumi:dev::test-protect::aws:s3/bucket:Bucket::my-2bucket dependency urn:pulumi:dev::test-protect::aws:s3/bucket:Bucket::my-bucket refers to missing resource

I will rectify this to protect dependencies of protected products.

}

if targets != nil && len(*targets) > 0 && excludeProtected {
return result.FromError(errors.New("You cannot specify --target and --exclude-protected"))
Copy link
Member

Choose a reason for hiding this comment

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

Cannot specify it "yet"? Is there a fundamental problem here?

Copy link
Member Author

Choose a reason for hiding this comment

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

--exclude-protected means destroy everything that is not protected. --target means destroy this specific thing. Any flag specified by --target would either be invalid (because it is protected) or would already be targeted by the destroy.

Because these flag behaviors don't make sense together, I forbid specifying both of them.

@t0yv0
Copy link
Member

t0yv0 commented Nov 11, 2021

This is awesome! This is an awesome feature. I've just recently been proofreading some PRs around dependency handling and bugs in dependency handling and being quite confused. I'd like to work together to make this as explicit as possible.

One PR I'd like to get merged before is:
#8360

Then just like in 8360, I think we can possibly exploit DependencyGraph to encapsulate ordering assumptions as well as topsort and most importantly the definition of what do we mean by dependencies in there.

DependencyGraph currently is build on this base relation:

C child depends on P=C.Parent unless nil
R resource depends on R.Provider unless nil
R resource depends on explicit dependencies R.Dependencies

The transitive closure of the relation gives the "indirect" dependency relation. "dg.DependenciesOf" and "dg.DependsOn" query these relations.

Presumably here, we could:

  • load DependencyGraph on all resources
  • find the set of protected resources
  • use DependencyGraph to find protected_and_deps = union([dg.DependingOn(x) for x in protected])
  • compute set difference to_delete = all_resources - protected_and_deps
  • use DependencyGraph to order to_delete set for correct deletion ordering
  • delete it

@t0yv0
Copy link
Member

t0yv0 commented Nov 11, 2021


--exclude-protected means destroy everything that is not protected. --target means destroy this specific thing. Any flag specified by --target would either be invalid (because it is protected) or would already be targeted by the destroy.

Because these flag behaviors don't make sense together, I forbid specifying both of them.

Except --target has --target-dependents which presumably uses graph computation that could use restricting - that's what I was thinking, but when you put it this way it's probably not worth it, reasonable to prohibit.

@iwahbe
Copy link
Member Author

iwahbe commented Nov 12, 2021

@t0yv0 I would be happy to work with you to ensure everything is clear.

DependencyGraph currently is build on this base relation:

C child depends on P=C.Parent unless nil
R resource depends on R.Provider unless nil
R resource depends on explicit dependencies R.Dependencies

The transitive closure of the relation gives the "indirect" dependency relation. "dg.DependenciesOf" and "dg.DependsOn" > query these relations.

I would like this to be true, but I don't think it is. dg.DependenciesOf doesn't handle parents at all. dg.DependingOn gives only direct parents (grandparent does not show as depended on).

Presumably here, we could:

load DependencyGraph on all resources
find the set of protected resources
use DependencyGraph to find protected_and_deps = union([dg.DependingOn(x) for x in protected])
compute set difference to_delete = all_resources - protected_and_deps

I think we could do this once #8360 lands.
It seems like dg.DependenciesOf is not transitive and complete. I have refactored out a version that does what I need it to do. This was we can still have a centralized location for our DAG code.

use DependencyGraph to order to_delete set for correct deletion ordering
delete it

We shouldn't need to do this. The engine should be able to handle this.

@iwahbe iwahbe requested a review from t0yv0 November 12, 2021 23:27
Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

LGTM!

pkg/cmd/pulumi/destroy.go Outdated Show resolved Hide resolved
pkg/cmd/pulumi/destroy.go Outdated Show resolved Hide resolved
pkg/cmd/pulumi/destroy.go Outdated Show resolved Hide resolved
pkg/cmd/pulumi/destroy.go Show resolved Hide resolved
pkg/cmd/pulumi/destroy.go Outdated Show resolved Hide resolved
pkg/resource/graph/dependency_graph.go Show resolved Hide resolved
pkg/cmd/pulumi/destroy.go Show resolved Hide resolved
@iwahbe iwahbe merged commit 554660b into master Nov 15, 2021
@pulumi-bot pulumi-bot deleted the iwahbe/6539/add-exclude-protected-flag branch November 15, 2021 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli UX of using the CLI (args, output, logs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add --exclude-protected flag to pulumi destroy
3 participants