feat(sdk/nodejs): delegates alias computation to the engine #11206

kpitzen · 2022-10-31T18:42:54Z

Description

This moves the alias computation from the node SDK to the engine (which now does these computations for us as of #10819 ). This dramatically improves performance when resources with many aliases are parented to one another.

Fixes #11062

Checklist

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

I have run make changelog and committed the changelog/pending/<file> documenting my change

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

pulumi-bot · 2022-10-31T18:43:32Z

Changelog

[uncommitted] (2022-12-15)

Features

[sdk/nodejs] Delegates alias computation to engine for Node SDK
#11206

Frassle · 2022-10-31T19:19:31Z

Annoyingly all this old code needs to stay because we need to support users on a new SDK but old cli. You'll need to make a supports_feature grpc call to see if it's a new enough engine to skip doing aliases in the cli.

kpitzen · 2022-11-03T13:03:52Z

sdk/nodejs/tests/runtime/langhost/run.spec.ts

@@ -1418,7 +1417,7 @@ describe("rpc", () => {
                    // SupportsFeature callback
                    (call: any, callback: any) => {
                        const resp = new resproto.SupportsFeatureResponse();
-                        resp.setHassupport(false);
+                        resp.setHassupport(true);


I'm not 100% sure why this was set to false before - do we need to support that? If so, I can just add an opt to switch this on-and-off per test.

I think it's OK to set it to true. I don't think we have any tests that assume it's false.

sdk/nodejs/resource.ts

sdk/nodejs/runtime/resource.ts

XBeg9 · 2022-11-29T16:28:03Z

this issue is giving us a hard time using pulumi with typescript, any ideas when this could be fixed?

kpitzen · 2022-11-29T20:34:59Z

this issue is giving us a hard time using pulumi with typescript, any ideas when this could be fixed?

Hi @XBeg9 ! This is still very much on my plate - there were a couple other pressing issues taking my attention, but the good news is we're on the last remaining failing test case. Once that's resolved (I hope to do so by EoW) we can get this in, and it will go out with the following release.

Thanks!

kpitzen · 2022-12-06T16:38:50Z

bors try

bors · 2022-12-06T17:44:41Z

try

Build failed:

bors-ok

kpitzen · 2022-12-12T15:47:16Z

bors try

kpitzen · 2022-12-15T20:26:31Z

bors try

kpitzen · 2022-12-15T20:39:04Z

bors try

bors · 2022-12-15T20:39:05Z

try

Already running a review

kpitzen · 2022-12-15T20:39:18Z

bors cancel

bors · 2022-12-15T21:07:00Z

try

Build failed:

bors-ok

justinvp

LGTM otherwise

sdk/nodejs/runtime/resource.ts

justinvp · 2022-12-15T21:35:36Z

sdk/nodejs/runtime/resource.ts

        // Wait for all aliases. Note that we use `res.__aliases` instead of `opts.aliases` as the former has been processed
        // in the Resource constructor prior to calling `registerResource` - both adding new inherited aliases and
        // simplifying aliases down to URNs.
        const aliases = [];
-        const uniqueAliases = new Set<string>();
-        for (const alias of (res.__aliases || [])) {
+        const uniqueAliases = new Set<Alias | URN>();


It's a little strange having Set<Alias> given that Alias is an object type which I believe means object reference equality will be used to compare what's already in the Set<Alias>.

I wonder if rather than keeping this code here, it should move into the !monitorSupportsStructuredAliases && parent case above, e.g. something like (I'm writing in GH without compiler help):

let aliases; if (!monitorSupportsStructuredAliases && parent) { const computedAliases = allAliases(opts.aliases || [], name!, type!, parent, parent.__name!); aliases = []; const uniqueAliases = new Set<URN>(); for (const alias of computedAliases) { const aliasVal = await output(alias).promise(); if (!uniqueAliases.has(aliasVal)) { uniqueAliases.add(aliasVal); aliases.push(aliasVal); } } } else { aliases = opts.aliases || []; }

Ah, missed that you'd still need to loop through and do const aliasVal = await output(alias).promise(); for the non-computed case. I think it's simpler to leave it the way you had it.

justinvp · 2022-12-15T21:38:21Z

sdk/nodejs/runtime/settings.ts

+ * connected to is able to support alias specs across its RPC interface. When it does, we marshal aliases
+ * in a special way.
+ */
+export async function monitorSupportsAliasSpecs(): Promise<boolean> {


Should we mark this as @internal? I don't think we'd expect anyone outside this library to use this.

We should probably go back and make the other exported ones @internal at some point as well (breaking anyone who happened to use it; so maybe not until a major version bump; though, unlikely anyone else is using them directly).

justinvp · 2022-12-15T21:45:13Z

sdk/nodejs/tests/runtime/langhost/run.spec.ts

@@ -1418,7 +1417,7 @@ describe("rpc", () => {
                    // SupportsFeature callback
                    (call: any, callback: any) => {
                        const resp = new resproto.SupportsFeatureResponse();
-                        resp.setHassupport(false);
+                        resp.setHassupport(true);


I think it's OK to set it to true. I don't think we have any tests that assume it's false.

kpitzen · 2022-12-15T22:26:33Z

bors merge

11206: feat(sdk/nodejs): delegates alias computation to the engine r=kpitzen a=kpitzen  # Description This moves the alias computation from the node SDK to the engine (which now does these computations for us as of #10819 ). This dramatically improves performance when resources with many aliases are parented to one another.  Fixes #11062 ## Checklist  - [x] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  Co-authored-by: Kyle Pitzen <kyle.pitzen@gmail.com>

bors · 2022-12-15T22:56:15Z

Build failed:

bors-ok

kpitzen · 2022-12-15T22:56:24Z

bors retry

bors · 2022-12-16T00:12:18Z

Build succeeded:

bors-ok

kpitzen · 2022-12-16T21:57:18Z

Hey @XBeg9 ! Just wanted to let you know that this has been merged and is queued up for Pulumi 3.49.1, so as long as you're using that version or greater of both the CLI and Node SDK, when it's released, you should have this feature now! Thank you again for your patience :)

XBeg9 · 2022-12-17T06:07:20Z

Fantastic news, @kpitzen ! Thanks for taking care of this; if you need beta testing, I am here to help. Otherwise waiting for the release.

12848: Fix aliasing children r=justinvp a=justinvp **Note:** This is a large PR that I've separated into many smaller commits to make it easier to review commit by commit. ## Background Originally, each SDK would compute alias URNs from alias "specs" and the alias URNs would be sent to the engine. #10819 added support for having the engine compute the alias URNs by enabling SDKs to send just the alias specs to the engine. #11206 updated the Node.js SDK to leverage this new capability, and subsequent PRs enabled it for the other SDKs. Unfortunately, there are a couple of issues: 1. Engine: A bug in alias computation 2. Node.js SDK: A bug in the alias specs sent to the engine ## Engine Bug There is an issue with how the engine computes the aliases when the resource is a child and doesn't have `Parent` set on the alias spec (and the parent doesn't have any aliases). ```python class FooResource(pulumi.ComponentResource): def __init__(self, name, opts=None): super().__init__("my:module:FooResource", name, None, opts) class ComponentResource(pulumi.ComponentResource): def __init__(self, name, opts=None): super().__init__("my:module:ComponentResource", name, None, opts) FooResource("childrenamed", pulumi.ResourceOptions( parent=self, aliases=[pulumi.Alias(name="child")] )) ``` In the example above, `ComponentResource` has a child `FooResource` which was renamed from `child` to `childrenamed`. The engine does not compute the correct alias: ``` expected: urn:pulumi:stack::project::my:module:ComponentResource$my:module:FooResource::child actual: urn:pulumi:stack::project::my:module:FooResource::child ``` The problem is due to: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/pkg/resource/deploy/step_generator.go#L370-L382 ... and: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/go/common/resource/alias.go#L24-L26 Because the alias spec doesn't have `Parent` specified, the parent type is not being included the computed alias URN. Existing tests such as https://github.com/pulumi/pulumi/tree/master/tests/integration/aliases/python/rename_component_and_child didn't catch the problem because the alias specifies both the `name` and `parent`: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/tests/integration/aliases/python/rename_component_and_child/step2/__main__.py#L15 In this case, specifying `parent` on the alias shouldn't be necessary. However, even after removing `parent` from the alias spec, the test still succeeds because the parent itself has an alias: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/tests/integration/aliases/python/rename_component_and_child/step2/__main__.py#L18 ... and parent aliases are inherited as part of a child's aliases, so we still get an alias that works from the inheritance. If we change the test to make no changes to the parent such that it doesn't have any aliases, then we get the failure as we'd expect. A similar problem will happen when retyping a child. ### Fix The fix involves using the child's parent in the calculated alias URN when `Parent` isn't specified for the alias. As part of this, we need to properly handled `NoParent` because right now the engine is not correctly using it. The struct representing an alias in the engine does not have a `NoParent` field: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/go/common/resource/alias.go#L8-L15 And therefore does not copy it over in the gRPC request: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/pkg/resource/deploy/source_eval.go#L1082-L1088 Instead, the `Alias` struct has an incorrect `NoParent` method which returns `true` if the `Parent` field has a value of `""`: https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/go/common/resource/alias.go#L24-L26 ## Node.js SDK Bug The Node.js SDK is not sending the right `Parent` and `NoParent` alias spec fields. https://github.com/pulumi/pulumi/blob/117955ce14b6cf2dd093f8356ec7e26427aca4bf/sdk/nodejs/runtime/resource.ts#L332-L361 - It's only setting `NoParent` when the alias doesn't have a `Parent` set and the resource doesn't have a parent. - If the resource has a parent and `Parent` isn't set on the resource, it sends the parent's URN for `Parent`. Instead: - It should set `NoParent` when `Parent` is set to `undefined`. - It should only send `Parent` if the user's alias has specified a `Parent`. This makes the Node.js SDK behave like the other SDKs. Fixes #12662 Co-authored-by: Justin Van Patten <jvp@justinvp.com>

kpitzen force-pushed the KP/11062 branch from 6448864 to eb756a3 Compare November 3, 2022 13:02

kpitzen commented Nov 3, 2022

View reviewed changes

kpitzen marked this pull request as ready for review November 3, 2022 13:04

kpitzen requested a review from Frassle November 3, 2022 13:04

Frassle reviewed Nov 3, 2022

View reviewed changes

sdk/nodejs/resource.ts Outdated Show resolved Hide resolved

sdk/nodejs/runtime/resource.ts Outdated Show resolved Hide resolved

kpitzen force-pushed the KP/11062 branch 4 times, most recently from 3596d1b to 86e0bdd Compare November 4, 2022 18:09

Frassle reviewed Nov 7, 2022

View reviewed changes

sdk/nodejs/runtime/resource.ts Outdated Show resolved Hide resolved

sdk/nodejs/runtime/resource.ts Outdated Show resolved Hide resolved

kpitzen force-pushed the KP/11062 branch from 86e0bdd to 9313dff Compare November 11, 2022 14:43

kpitzen marked this pull request as draft November 16, 2022 17:20

kpitzen force-pushed the KP/11062 branch 4 times, most recently from 2244c58 to 8da6e68 Compare November 16, 2022 22:50

kpitzen commented Nov 16, 2022

View reviewed changes

sdk/nodejs/runtime/resource.ts Outdated Show resolved Hide resolved

Frassle mentioned this pull request Nov 29, 2022

OOM - JavaScript heap out of memory when trying to create resources #11476

Closed

kpitzen force-pushed the KP/11062 branch 2 times, most recently from 7fe3fb2 to 04d8201 Compare December 6, 2022 16:32

bors bot added a commit that referenced this pull request Dec 6, 2022

Try #11206:

c69f972

kpitzen force-pushed the KP/11062 branch 2 times, most recently from 2aa9bd1 to e300a59 Compare December 8, 2022 14:25

kpitzen force-pushed the KP/11062 branch from 3a5ffbb to fd645de Compare December 15, 2022 20:25

kpitzen marked this pull request as ready for review December 15, 2022 20:26

bors bot added a commit that referenced this pull request Dec 15, 2022

Try #11206:

83fd59a

kpitzen force-pushed the KP/11062 branch from fd645de to 15f3e99 Compare December 15, 2022 20:34

kpitzen force-pushed the KP/11062 branch from 15f3e99 to ae3e40b Compare December 15, 2022 20:41

kpitzen requested review from justinvp, Frassle, RobbieMcKinstry, iwahbe and AaronFriel December 15, 2022 21:36

justinvp approved these changes Dec 15, 2022

View reviewed changes

feat(sdk/nodejs): delegates alias computation to the engine

92cb709

kpitzen force-pushed the KP/11062 branch from ae3e40b to 92cb709 Compare December 15, 2022 22:23

bors bot merged commit 24f0ae3 into master Dec 16, 2022

bors bot deleted the KP/11062 branch December 16, 2022 00:12

justinvp mentioned this pull request May 8, 2023

Fix aliasing children #12848

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk/nodejs): delegates alias computation to the engine #11206

feat(sdk/nodejs): delegates alias computation to the engine #11206

kpitzen commented Oct 31, 2022

pulumi-bot commented Oct 31, 2022 •

edited

Frassle commented Oct 31, 2022

kpitzen Nov 3, 2022

justinvp Dec 15, 2022

XBeg9 commented Nov 29, 2022

kpitzen commented Nov 29, 2022

kpitzen commented Dec 6, 2022

bors bot commented Dec 6, 2022

kpitzen commented Dec 12, 2022

kpitzen commented Dec 15, 2022

kpitzen commented Dec 15, 2022

bors bot commented Dec 15, 2022

kpitzen commented Dec 15, 2022

bors bot commented Dec 15, 2022

justinvp left a comment

justinvp Dec 15, 2022

justinvp Dec 15, 2022

justinvp Dec 15, 2022

justinvp Dec 15, 2022

kpitzen commented Dec 15, 2022

bors bot commented Dec 15, 2022

kpitzen commented Dec 15, 2022

bors bot commented Dec 16, 2022

kpitzen commented Dec 16, 2022 •

edited

XBeg9 commented Dec 17, 2022

feat(sdk/nodejs): delegates alias computation to the engine #11206

feat(sdk/nodejs): delegates alias computation to the engine #11206

Conversation

kpitzen commented Oct 31, 2022

Description

Checklist

pulumi-bot commented Oct 31, 2022 • edited

Changelog

[uncommitted] (2022-12-15)

Features

Frassle commented Oct 31, 2022

kpitzen Nov 3, 2022

Choose a reason for hiding this comment

justinvp Dec 15, 2022

Choose a reason for hiding this comment

XBeg9 commented Nov 29, 2022

kpitzen commented Nov 29, 2022

kpitzen commented Dec 6, 2022

bors bot commented Dec 6, 2022

try

kpitzen commented Dec 12, 2022

kpitzen commented Dec 15, 2022

kpitzen commented Dec 15, 2022

bors bot commented Dec 15, 2022

try

kpitzen commented Dec 15, 2022

bors bot commented Dec 15, 2022

try

justinvp left a comment

Choose a reason for hiding this comment

justinvp Dec 15, 2022

Choose a reason for hiding this comment

justinvp Dec 15, 2022

Choose a reason for hiding this comment

justinvp Dec 15, 2022

Choose a reason for hiding this comment

justinvp Dec 15, 2022

Choose a reason for hiding this comment

kpitzen commented Dec 15, 2022

bors bot commented Dec 15, 2022

kpitzen commented Dec 15, 2022

bors bot commented Dec 16, 2022

kpitzen commented Dec 16, 2022 • edited

XBeg9 commented Dec 17, 2022

pulumi-bot commented Oct 31, 2022 •

edited

kpitzen commented Dec 16, 2022 •

edited