Include transitive children in dependency list for deletes #7788
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A candidate approach for addressing the deletion order issues identified in #7780.
justinvp
reviewed
Aug 18, 2021
pgavlin
reviewed
Aug 18, 2021
pgavlin
approved these changes
Aug 19, 2021
pgavlin
added a commit
that referenced
this pull request
Aug 19, 2021
These changes implement a higher-level fix for #7780 by expanding the dependencies of a resource as part of step generation. Dependency expansion is responsible for expanding dependencies on component resources into dependencies on the component resource _and_ the component resource's descendents _at the time the dependent is registered_. For example, given a dependency on Comp1 in the following parent/child graph: Comp1 _______|__________ | | | Cust1 Comp2 Comp3 ___|___ | | | | Cust2 Cust3 Comp4 | | Cust4 Cust5 The expanded dependencies set will be [Comp1, Cust1, Comp2, Comp3, Cust2, Cust3, Comp4, Cust5]. Notably, Cust4 is *not* a member of the set because it is a child of a custom resource. These changes are complementary to the changes in #7788. The primary benefit of the early expansion approach is that the expanded dependencies will be present in events and the statefile, so downstream consumers do not need to replicate the expansion logic in order to understand the totality of a resource's dependencies.
pgavlin
added a commit
that referenced
this pull request
Aug 19, 2021
These changes implement a higher-level fix for #7780 by expanding the dependencies of a resource as part of step generation. Dependency expansion is responsible for expanding dependencies on component resources into dependencies on the component resource _and_ the component resource's descendents _at the time the dependent is registered_. For example, given a dependency on Comp1 in the following parent/child graph: Comp1 _______|__________ | | | Cust1 Comp2 Comp3 ___|___ | | | | Cust2 Cust3 Comp4 | | Cust4 Cust5 The expanded dependencies set will be [Comp1, Cust1, Comp2, Comp3, Cust2, Cust3, Comp4, Cust5]. Notably, Cust4 is *not* a member of the set because it is a child of a custom resource. These changes are complementary to the changes in #7788. The primary benefit of the early expansion approach is that the expanded dependencies will be present in events and the statefile, so downstream consumers do not need to replicate the expansion logic in order to understand the totality of a resource's dependencies.
pgavlin
added a commit
that referenced
this pull request
Aug 19, 2021
These changes implement a higher-level fix for #7780 by expanding the dependencies of a resource as part of step generation. Dependency expansion is responsible for expanding dependencies on component resources into dependencies on the component resource _and_ the component resource's descendents _at the time the dependent is registered_. For example, given a dependency on Comp1 in the following parent/child graph: Comp1 _______|__________ | | | Cust1 Comp2 Comp3 ___|___ | | | | Cust2 Cust3 Comp4 | | Cust4 Cust5 The expanded dependencies set will be [Comp1, Cust1, Comp2, Comp3, Cust2, Cust3, Comp4, Cust5]. Notably, Cust4 is *not* a member of the set because it is a child of a custom resource. These changes are complementary to the changes in #7788. The primary benefit of the early expansion approach is that the expanded dependencies will be present in events and the statefile, so downstream consumers do not need to replicate the expansion logic in order to understand the totality of a resource's dependencies.
pgavlin
added a commit
that referenced
this pull request
Aug 20, 2021
These changes implement a higher-level fix for #7780 by expanding the dependencies of a resource as part of step generation. Dependency expansion is responsible for expanding dependencies on component resources into dependencies on the component resource _and_ the component resource's descendents _at the time the dependent is registered_. For example, given a dependency on Comp1 in the following parent/child graph: Comp1 _______|__________ | | | Cust1 Comp2 Comp3 ___|___ | | | | Cust2 Cust3 Comp4 | | Cust4 Cust5 The expanded dependencies set will be [Comp1, Cust1, Comp2, Comp3, Cust2, Cust3, Comp4, Cust5]. Notably, Cust4 is *not* a member of the set because it is a child of a custom resource. These changes are complementary to the changes in #7788. The primary benefit of the early expansion approach is that the expanded dependencies will be present in events and the statefile, so downstream consumers do not need to replicate the expansion logic in order to understand the totality of a resource's dependencies.
pgavlin
added a commit
that referenced
this pull request
Aug 26, 2021
Consider the following Pulumi program:
```typescript
class MyResource extends pulumi.CustomResource {
// ...
}
class MyComponent extends pulumi.ComponentResource {
constructor(name: string) {
super("myComponent", name);
this.child = new MyResource("child", {}, {parent: this});
}
}
// Create a component resource. The constructor returns promptly, but
// the registration of the component itself happens asynchronously.
// Similarly, its child resource's constructor returns promptly, but
// its registration happens asynchronously. The SDK guarantees that the
// latter happens after the former by waiting on the parent's URN prior
// to registration (note that this is necessary in order for the runtime
// to fill out the `parent` field of the registration request).
const component = new MyComponent("component");
// Create a resource that depends on the component resource. The
// registration of this resource happens asynchronously, and races with
// the asynchronous registration of the component's child.
const res = new MyResource("resource", {}, {dependsOn: component});
```
Due to the races described in the comments, even when all of a
component's children are constructed prior to exiting the component's
constructor (e.g. not in applies, thens, etc.) we cannot guarantee that
the children have been registered prior to any resources that depend on
the component. Because of this, the approach that #7788 takes for
expanding dependencies on components to dependencies on their children
is not necessarily reliable, as it only adds children that appear prior
to dependents in the snapshot.
These changes adjust the dependency graph to add all children of a
component that would not create cycles in the dependency graph.
Fixes #7833.
pgavlin
added a commit
that referenced
this pull request
Aug 26, 2021
Consider the following Pulumi program:
```typescript
class MyResource extends pulumi.CustomResource {
// ...
}
class MyComponent extends pulumi.ComponentResource {
constructor(name: string) {
super("myComponent", name);
this.child = new MyResource("child", {}, {parent: this});
}
}
// Create a component resource. The constructor returns promptly, but
// the registration of the component itself happens asynchronously.
// Similarly, its child resource's constructor returns promptly, but
// its registration happens asynchronously. The SDK guarantees that the
// latter happens after the former by waiting on the parent's URN prior
// to registration (note that this is necessary in order for the runtime
// to fill out the `parent` field of the registration request).
const component = new MyComponent("component");
// Create a resource that depends on the component resource. The
// registration of this resource happens asynchronously, and races with
// the asynchronous registration of the component's child.
const res = new MyResource("resource", {}, {dependsOn: component});
```
Due to the races described in the comments, even when all of a
component's children are constructed prior to exiting the component's
constructor (e.g. not in applies, thens, etc.) we cannot guarantee that
the children have been registered prior to any resources that depend on
the component. Because of this, the approach that #7788 takes for
expanding dependencies on components to dependencies on their children
is not necessarily reliable, as it only adds children that appear prior
to dependents in the snapshot.
These changes adjust the dependency graph to add all children of a
component that would not create cycles in the dependency graph.
Fixes #7833.
pgavlin
added a commit
that referenced
this pull request
Aug 26, 2021
Consider the following Pulumi program:
```typescript
class MyResource extends pulumi.CustomResource {
// ...
}
class MyComponent extends pulumi.ComponentResource {
constructor(name: string) {
super("myComponent", name);
this.child = new MyResource("child", {}, {parent: this});
}
}
// Create a component resource. The constructor returns promptly, but
// the registration of the component itself happens asynchronously.
// Similarly, its child resource's constructor returns promptly, but
// its registration happens asynchronously. The SDK guarantees that the
// latter happens after the former by waiting on the parent's URN prior
// to registration (note that this is necessary in order for the runtime
// to fill out the `parent` field of the registration request).
const component = new MyComponent("component");
// Create a resource that depends on the component resource. The
// registration of this resource happens asynchronously, and races with
// the asynchronous registration of the component's child.
const res = new MyResource("resource", {}, {dependsOn: component});
```
Due to the races described in the comments, even when all of a
component's children are constructed prior to exiting the component's
constructor (e.g. not in applies, thens, etc.) we cannot guarantee that
the children have been registered prior to any resources that depend on
the component. Because of this, the approach that #7788 takes for
expanding dependencies on components to dependencies on their children
is not necessarily reliable, as it only adds children that appear prior
to dependents in the snapshot.
These changes adjust the dependency graph to add all children of a
component that would not create cycles in the dependency graph.
Fixes #7833.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes deletion order issues outlined in #7780 related to dependencies on multi language components not ensuring that all (transitive) children of the dependency are treated as dependencies.
Since the state file is not guaranteed to include all transitive dependencies explicitly specified (because dependencies on multi-langauge components will only include the dependency on the component itself, not all of it's transitive children), this PR enlightens the computation of dependencies for deletion ordering to expand the transitive dependency to include transitive children of direct dependencies.
Planned follow-ups: