Process deletions conservatively in parallel #1963

swgillespie · 2018-09-20T21:38:04Z

This commit allows the engine to conservatively delete resources in
parallel when it is sure that it is legal to do so. In the absence of a
true data-flow oriented step scheduler, this approach provides a
significant improvement over the existing serial deletion mechanism.

Instead of processing deletes serially, this commit will partition the
set of condemned resources into sets of resources that are known to be
legally deletable in parallel. The step executor will then execute those
independent lists of steps one-by-one until all steps are complete.

This commit allows the engine to conservatively delete resources in parallel when it is sure that it is legal to do so. In the absence of a true data-flow oriented step scheduler, this approach provides a significant improvement over the existing serial deletion mechanism. Instead of processing deletes serially, this commit will partition the set of condemned resources into sets of resources that are known to be legally deletable in parallel. The step executor will then execute those independent lists of steps one-by-one until all steps are complete.

swgillespie · 2018-09-20T22:19:09Z

Fixes #1625 unless we think that this conservative approach isn't good enough for us

pgavlin · 2018-09-20T22:38:18Z

pkg/resource/graph/resource_set.go

+import "github.com/pulumi/pulumi/pkg/resource"
+
+// ResourceSet represents a set of Resources.
+type ResourceSet struct {


This seems like overkill--why not just use a map directly?

I wanted ResourceSet.Intersect and it's annoying to do that with just maps. I don't mind having this either way, I was just not a fan of doing set intersections of with map[*resource.State]bool.

Note that you can just do

type ResourceSet map[*resource.State]bool func (rs ResourceSet) Intersect(other ResourceSet) ResourceSet { i := make(ResourceSet) for r := range rs { if other[r] { i[r] = true } } return i }

And then just interact with the set normally otherwise.

Oh, cool, I wasn't aware that you could do this while also preserving map-like behavior.

pgavlin · 2018-09-21T17:36:35Z

pkg/resource/deploy/step_executor.go

+
+// A CompletionToken is a token returned by the step executor that is completed when the chain has completed execution.
+// Callers can use it to optionally wait synchronously on the completion of a chain.
+type CompletionToken struct {


Out of curiosity, do any of the executor/token/etc. types need to be exported? IIRC they're now all internal to this package, right?

I think you're right, these don't need to be exported.

swgillespie · 2018-09-21T17:48:30Z

pkg/resource/deploy/step_generator.go

+			if len(condemnedDependencies) == 0 && !condemned[dg.ParentOf(res)] {
+				// If not, it's safe to delete res at this stage.
+				logging.V(7).Infof("Planner scheduling deletion of '%v'", res.URN)
+				if res.Delete && !pendingDeletesAreReplaces {


I accidentally misnamed pendingDeletesAreReplaces here (I renamed it just before opening the PR). The polarity of this bool should be reversed.

Will fix in the next commit.

swgillespie · 2018-09-21T18:55:23Z

One issue that I just encountered that will need to be fixed before this merges is that this optimization is not legal when the host language doesn't provide the engine with full dependency information (namely Python, most likely Go as well). For those languages we will have to resort to serial deletes as we do today.

joeduffy · 2018-09-25T16:04:15Z

I so can't wait for this to land!

namely Python, most likely Go as well

Go should faithfully record dependencies. You're right that Python absolutely does not, however.

swgillespie · 2018-09-25T17:47:28Z

This PR is ready to merge, pending reviews.

swgillespie · 2018-09-26T18:09:24Z

ping @pgavlin @lukehoban - I'd like to get this in so we can begin getting some mileage on it, if possible.

pgavlin · 2018-09-27T17:33:23Z

pkg/resource/deploy/step_generator.go

-						// If we already deleted this resource due to some other DBR, don't do it again.
-						if sg.deletes[urn] {
-							continue
+					if sg.opts.TrustDependencies {


Just to be clear, this returns us to the original behavior of delete-before-create when we're running a Python program, right? Are Python dependencies incomplete, or just wrong?

Just to be clear, this returns us to the original behavior of delete-before-create when we're running a Python program, right?

Yes, that's correct.

Are Python dependencies incomplete, or just wrong?

They're incomplete. Python does not honor dependsOn nor does it make any attempt to track implicit dependencies.

To be clear, there's most likely going to be no behavior change here when using Python. The Python dependency graph is always empty because no dependencies are reported. When adding TrustDependencies, I figured that it should guard against any use of dependency graphs, since it's (in my mind) wrong to use one if we know that the information contained in it is incomplete and untrustworthy.

FWIW, we could "improve" this situation by handling this in the language host and having it report all preceding resources as dependencies. I don't think that we should do that--there are few enough places where we use dependency information that I don't believe that it's necessary--but I think that it's worth mentioning for completeness.

pgavlin · 2018-09-27T17:40:02Z

pkg/resource/deploy/step_generator.go

@@ -399,75 +402,182 @@ func (sg *stepGenerator) GenerateSteps(event RegisterResourceEvent) ([]Step, *re
 	return []Step{NewCreateStep(sg.plan, event, new)}, nil
 }

-func (sg *stepGenerator) GenerateDeletes() []Step {
+func (sg *stepGenerator) GenerateDeletes() []antichain {


It feels like it may be a bit cleaner to let this method generate the list of steps and then let the caller schedule that list (rather than splitting the responsibility for step generation across multiple methods). That would eliminate a fair bit of the conditional code here, and would allow scheduleDeletes to omit the pendingDeletesAreReplaces parameter.

To make this more concrete, I would change the signature of scheduleDeletes s.t. it takes a []Step. It can then decide whether to just return the slice as-is (in the case that dependency information is not conservatively correct) or to schedule using antichain decomposition.

Makes sense - will do.

pgavlin · 2018-09-27T17:44:54Z

pkg/resource/graph/dependency_graph.go

@@ -69,6 +69,53 @@ func (dg *DependencyGraph) DependingOn(res *resource.State) []*resource.State {
 	return dependents
 }

+// DependenciesOf returns a ResourceSet of resources upon which the given resource depends.
+func (dg *DependencyGraph) DependenciesOf(res *resource.State) ResourceSet {


Is there any reason not to include the parent here? In a very real sense it is a dependency--a resource R with a parent P cannot be registered prior to P.

Nope, and it probably should - I have to explicitly add the parent to the set of dependencies in the step generator anyway for this reason.

pgavlin · 2018-09-27T17:45:39Z

pkg/resource/graph/dependency_graph.go

+
+	cursorIndex, ok := dg.index[res]
+	contract.Assert(ok)
+	for i := cursorIndex; i >= 0; i-- {


Can this be cursorIndex-1?

Yep. will fix.

swgillespie · 2018-09-27T21:31:19Z

@pgavlin The last two commits address your PR feedback.

pgavlin · 2018-09-27T21:41:46Z

pkg/resource/graph/dependency_graph.go

+		}
+	}
+
+	set[dg.ParentOf(res)] = true


Can we just look for this resource in the loop above? That would also let us eliminate the ParentOf function below.

pgavlin

LGTM!

Thanks for bearing with me.

swgillespie · 2018-09-27T22:49:14Z

thanks for the review!

swgillespie requested review from pgavlin and lukehoban September 20, 2018 21:38

pgavlin reviewed Sep 20, 2018

View reviewed changes

CR: Make ResourceSet a normal map

3b45068

pgavlin reviewed Sep 21, 2018

View reviewed changes

swgillespie commented Sep 21, 2018

View reviewed changes

swgillespie added 3 commits September 21, 2018 14:52

Only use the dependency graph if we can trust it

ebdaec1

Reverse polarity of pendingDeletesAreReplaces

d7f91a4

CR: un-export a few types

c19a19c

pgavlin reviewed Sep 27, 2018

View reviewed changes

swgillespie added 2 commits September 27, 2018 14:20

CR: simplify control flow in step generator when scheduling

0db96b1

CR: parents are dependencies, fix loop index

b38bfc0

pgavlin reviewed Sep 27, 2018

View reviewed changes

CR: Remove ParentOf, add new test for parent dependencies

9f4227d

pgavlin approved these changes Sep 27, 2018

View reviewed changes

swgillespie merged commit ed0353e into master Sep 27, 2018

pulumi-bot deleted the swgillespie/parallel-delete branch September 27, 2018 22:49

swgillespie mentioned this pull request Sep 27, 2018

Parallelize resource deletion #1625

Closed

swgillespie mentioned this pull request Jan 23, 2019

Python: Turn on delete parallelism #2382

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process deletions conservatively in parallel #1963

Process deletions conservatively in parallel #1963

swgillespie commented Sep 20, 2018

swgillespie commented Sep 20, 2018

pgavlin Sep 20, 2018

swgillespie Sep 20, 2018

pgavlin Sep 20, 2018

swgillespie Sep 20, 2018

pgavlin Sep 21, 2018

swgillespie Sep 21, 2018

swgillespie Sep 21, 2018

swgillespie Sep 21, 2018

swgillespie commented Sep 21, 2018

joeduffy commented Sep 25, 2018

swgillespie commented Sep 25, 2018

swgillespie commented Sep 26, 2018

pgavlin Sep 27, 2018

swgillespie Sep 27, 2018

swgillespie Sep 27, 2018

pgavlin Sep 27, 2018

pgavlin Sep 27, 2018

pgavlin Sep 27, 2018

swgillespie Sep 27, 2018

pgavlin Sep 27, 2018

swgillespie Sep 27, 2018

pgavlin Sep 27, 2018

swgillespie Sep 27, 2018

swgillespie commented Sep 27, 2018

pgavlin Sep 27, 2018

swgillespie Sep 27, 2018

pgavlin left a comment

swgillespie commented Sep 27, 2018

Process deletions conservatively in parallel #1963

Process deletions conservatively in parallel #1963

Conversation

swgillespie commented Sep 20, 2018

swgillespie commented Sep 20, 2018

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

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

swgillespie commented Sep 21, 2018

joeduffy commented Sep 25, 2018

swgillespie commented Sep 25, 2018

swgillespie commented Sep 26, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swgillespie commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgavlin left a comment

Choose a reason for hiding this comment

swgillespie commented Sep 27, 2018