Allow tfgen to use pulumi convert for examples conversion #1401

t0yv0 · 2023-09-26T19:33:30Z

Requires a modified pulumi/pulumi-converter-terraform#29

A new option is added for tfgen, via the environment variable PULUMI_CONVERT. IF this option is truthy, make tfgen will now be using pulumi convert when converting examples, specifically for the TF HCL to Pulumi PCL phase of the conversion.

How this was tested. Tried on these providers:

pulumi-wavefront
pulumi-gcp

Ensured conversion rates improve. Ensured COVERAGE_OUTPUT_DIR, if set, continues to populate sensible conversion metrics. Ensured printed warnings and example conversion metrics out of make tfgen are still sensible.

When rolling this out to providers, recommend making sure that Pulumi CLI in use is pinned first, as the repositories are sensitive to small changes in the generated examples and a floating Pulumi CLI reference can introduce breaking CI checks.

github-actions · 2023-09-26T19:40:18Z

Diff for pulumi-azuread with merge commit 0679f5a

github-actions · 2023-09-26T19:40:24Z

Diff for pulumi-random with merge commit 0679f5a

t0yv0 · 2023-09-27T02:08:26Z

@Frassle @iwahbe @mjeffryes @mikhailshilkov

This is rough around the edges but it's promising. I've got it to render pulumi-gcp example set through the new converter engaged out of process via pulumi convert. It seems viable.

pulumi-converter-terraform spins CPU for about 3min, but is fairly low on memory utilization (<50mb).

We can parallelize it to stand up N*CPU copies.

Before parallelization as things stand the total timings for schema generation with this on GCP are:

      187.20 real       254.00 user        15.11 sys

That's not bad we can take that. The appeal of the new converter is high.

t0yv0 · 2023-09-27T02:15:28Z

The annoying bits:

I hit "limit reached on GitHub API" because something was hitting up GitHub.com/pulumi/pulumi-gcp to find latest release too often; I've modified the loader to try to recognize that it already has the partial schema for GCP in-place; I'm surprised and a little intrigued about how this works in prod without the changes here and without hitting the limit
The GCP provider names itself "google-beta" but there's also "google" and "gcp" and it's really finicky which one is the provider identifier; when talking to pulumi-convert apparently mapping files sent via --mappings have to be called by convention "gcp.json" but this is not obvious should it be "gcp.json" or "google.json" or..
It's extremely annoying that the code retains the package loader to perform pcl binding; this loader has to match what the underlying loader in pulumi convert is going to do; definitely for the current provider such as gcp, but really also for any dynamically loaded providers that show up in examples like "std"; there's no consistent pinning
by virtue of above, we also still build-depend on every language's codegen here

t0yv0 · 2023-09-27T02:17:31Z

On the positive side, the code shouldn't panic so we don't need to inject recover and lose track panics. I will hook up example metrics to get them right (that's on the TODO). Perhaps could also push through a bulk examples.json interface for all the other converters (ts,py,yaml,java,cs)? Or is that too ambitious?

pkg/tfgen/convert_cli.go

github-actions · 2023-09-28T19:30:21Z

Diff for pulumi-random with merge commit 4ea49c1

github-actions · 2023-09-28T19:30:52Z

Diff for pulumi-azuread with merge commit 4ea49c1

github-actions · 2023-09-28T23:40:04Z

Diff for pulumi-random with merge commit 6ab56be

github-actions · 2023-09-28T23:40:42Z

Diff for pulumi-azuread with merge commit 6ab56be

github-actions · 2023-09-29T18:34:08Z

Diff for pulumi-random with merge commit de3eab7

github-actions · 2023-09-29T18:35:28Z

Diff for pulumi-azuread with merge commit de3eab7

github-actions · 2023-09-29T19:19:41Z

Diff for pulumi-random with merge commit 8eed677

github-actions · 2023-09-29T19:20:48Z

Diff for pulumi-azuread with merge commit 8eed677

github-actions · 2023-09-29T19:21:06Z

Diff for pulumi-random with merge commit 74e9ffa

github-actions · 2023-09-29T19:22:13Z

Diff for pulumi-azuread with merge commit 74e9ffa

t0yv0 · 2023-09-29T19:27:26Z

limit reached on GitHub API is resolved; was a bug in this code. A few more low level issues are resolved. This is working better.

github-actions · 2023-09-29T19:29:22Z

Diff for pulumi-random with merge commit 33f79a8

github-actions · 2023-09-29T19:31:06Z

Diff for pulumi-azuread with merge commit 33f79a8

codecov-commenter · 2023-09-29T19:31:35Z

Codecov Report

Merging #1401 (18166cf) into master (772baf6) will increase coverage by 0.36%.
The diff coverage is 73.19%.

@@            Coverage Diff             @@
##           master    #1401      +/-   ##
==========================================
+ Coverage   57.01%   57.37%   +0.36%     
==========================================
  Files         147      147              
  Lines       24400    24714     +314     
==========================================
+ Hits        13911    14180     +269     
- Misses       9505     9527      +22     
- Partials      984     1007      +23

Files	Coverage Δ
pkg/tfgen/docs.go	`72.94% <100.00%> (+2.04%)`	⬆️
pkg/tfgen/generate.go	`57.68% <ø> (+0.39%)`	⬆️
pkg/tfgen/generate_schema.go	`81.25% <100.00%> (+0.06%)`	⬆️
pkg/tfgen/convert_cli.go	`69.62% <69.62%> (ø)`

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pkg/tfgen/convert_cli.go

github-actions · 2023-09-29T20:49:53Z

Diff for pulumi-random with merge commit 0e7435a

github-actions · 2023-09-29T20:51:36Z

Diff for pulumi-azuread with merge commit 0e7435a

github-actions · 2023-10-02T21:02:21Z

Diff for pulumi-azuread with merge commit 9d47b37

github-actions · 2023-10-02T21:03:02Z

Diff for pulumi-azuread with merge commit 837c1cd

github-actions · 2023-10-02T21:04:05Z

Diff for pulumi-random with merge commit 9d47b37

github-actions · 2023-10-02T21:05:15Z

Diff for pulumi-random with merge commit 837c1cd

github-actions · 2023-10-02T21:37:31Z

Diff for pulumi-azuread with merge commit 42a5c98

github-actions · 2023-10-02T21:38:22Z

Diff for pulumi-random with merge commit 42a5c98

github-actions · 2023-10-02T22:19:38Z

Diff for pulumi-azuread with merge commit 7b2c036

github-actions · 2023-10-02T22:21:52Z

Diff for pulumi-random with merge commit 7b2c036

github-actions · 2023-10-02T22:32:56Z

Diff for pulumi-azuread with merge commit 79ed526

github-actions · 2023-10-02T22:33:29Z

Diff for pulumi-random with merge commit 79ed526

iwahbe · 2023-10-03T17:44:59Z

README.md

@@ -106,3 +106,4 @@ tfgen, the command that generates Pulumi schema/code for a bridged provider supp
 * `PULUMI_SKIP_MISSING_MAPPING_ERROR`: If truthy, tfgen will not fail if a data source or resource in the TF provider is not mapped to the Pulumi provider. Instead, a warning is printed. Default is `false`.
 * `PULUMI_SKIP_EXTRA_MAPPING_ERROR`: If truthy, tfgen will not fail if a mapped data source or resource does not exist in the TF provider. Instead, warning is printed. Default is `false`.
 * `PULUMI_MISSING_DOCS_ERROR`: If truthy, tfgen will fail if docs cannot be found for a data source or resource. Default is `false`.
+* `PULUMI_CONVERT`: If truthy, tfgen will shell out to `pulumi convert` for converting example code from TF HCL to Pulumi PCL


Do we expect to use this ourselves?

Yes. The idea is to roll this out to Tier 1 providers as soon as we can. In terms of retiring this flag, we'd need to make sure our entire fleet has migrated (including Tier 2-3) and then stakeholders and community have migrated, then we can retire it and make it the default.

pkg/tfgen/convert_cli.go

iwahbe · 2023-10-03T18:10:03Z

pkg/tfgen/convert_cli.go

+// This may need to be coarse-grain parallelized to speed up larger providers at the cost of more
+// memory, for example run 4 instances of `pulumi convert` on 25% of examples each.


I would prefer if this kind of speedup could happen in the engine, where we just pass the number of concurrent processes to run at once. It is safe to parallelize within process because there is no shared mutable state.¹

Footnotes

If there is shared mutable state, then parallelizing by process splitting will be non-deterministic. ↩

It is safe to parallelize within process because there is no shared mutable state.

Famous last words. Prove there's no shared mutable state in large go systems (think static var).

Hence the footnote: It will be either safe to parallelize within a single process or it isn't, in which case I'll be scared to parallelize across processes because each update shares mutable state.

At a first approximation, yes, but it's not apples to apples.

If you're parallelizing in-process, you can get issues related to say panics on concurrent map element access for shared state.

If you're parallelizing outside of process, you can get issues related to using shared OS resources such as file handles or folders.

It's just a guess at this point, I don't know if any of those are actual issues. We could try and see at some point.

pkg/tfgen/convert_cli.go

iwahbe · 2023-10-03T18:16:49Z

pkg/tfgen/convert_cli_test.go

+	if runtime.GOOS == "windows" {
+		// Currently there is a test issue in CI/test setup:
+		//
+		// convertViaPulumiCLI: failed to clean up temp bridge-examples.json file: The


Have we manually tested running the new convert workflow on windows? We do support authoring bridged providers on windows AFAIK.

Nope, we haven't. It's not on the critical path. I can add it to the bottom of the epic.

It's necessary for removing the old convert logic, but shouldn't be blocking for us to use the new logic. We might want to add a warning for users who turn on the new converter on windows computers.

iwahbe · 2023-10-03T18:18:57Z

I would really like a plan for how this change will roll out.

t0yv0 · 2023-10-03T19:19:49Z

The plan is included in the epic #1280

github-actions · 2023-10-03T19:41:09Z

Diff for pulumi-azuread with merge commit 60260d4

github-actions · 2023-10-03T19:42:13Z

Diff for pulumi-random with merge commit 60260d4

iwahbe

LGTM

t0yv0 commented Sep 27, 2023

View reviewed changes

pkg/tfgen/convert_cli.go Outdated Show resolved Hide resolved

t0yv0 commented Sep 27, 2023

View reviewed changes

pkg/tfgen/convert_cli.go Outdated Show resolved Hide resolved

t0yv0 force-pushed the t0yv0/convert-examples-via-cli branch from 440d362 to 99b08cd Compare September 28, 2023 19:24

t0yv0 force-pushed the t0yv0/convert-examples-via-cli branch from def6b6c to 9ab59aa Compare September 29, 2023 18:31

t0yv0 marked this pull request as ready for review September 29, 2023 19:20

t0yv0 changed the title ~~Refactor: convert examples via cli~~ Allow tfgen to use pulumi convert for examples conversion Sep 29, 2023

t0yv0 requested review from iwahbe and Frassle September 29, 2023 19:28

Frassle reviewed Sep 29, 2023

View reviewed changes

pkg/tfgen/convert_cli.go Outdated Show resolved Hide resolved

pkg/tfgen/convert_cli.go Outdated Show resolved Hide resolved

pkg/tfgen/convert_cli.go Outdated Show resolved Hide resolved

pkg/tfgen/convert_cli.go Show resolved Hide resolved

t0yv0 added 5 commits October 2, 2023 17:00

Fix test

61d0754

DO not use needless env vars

a055d30

Enable the test; merge PR when it can pass

982aa0a

Enhance the error message

a4a020c

Add GenerateSchema test

e046f4e

t0yv0 force-pushed the t0yv0/convert-examples-via-cli branch from bf6dcc4 to e046f4e Compare October 2, 2023 21:00

Skip on Windows

fbe0a3f

Ensure the new code is actually tested

b271387

Fix lint

18166cf

t0yv0 mentioned this pull request Oct 3, 2023

Rollout experimental converter to Tier 1 providers #1413

Closed

4 tasks

iwahbe reviewed Oct 3, 2023

View reviewed changes

PR feedback

a7ddbbe

t0yv0 requested a review from iwahbe October 3, 2023 19:38

iwahbe approved these changes Oct 3, 2023

View reviewed changes

t0yv0 merged commit e3aabd3 into master Oct 3, 2023

t0yv0 deleted the t0yv0/convert-examples-via-cli branch October 3, 2023 22:03

		// This may need to be coarse-grain parallelized to speed up larger providers at the cost of more
		// memory, for example run 4 instances of `pulumi convert` on 25% of examples each.

Allow tfgen to use pulumi convert for examples conversion #1401

Allow tfgen to use pulumi convert for examples conversion #1401

Conversation

t0yv0 commented Sep 26, 2023 • edited Loading

github-actions bot commented Sep 26, 2023

github-actions bot commented Sep 26, 2023

t0yv0 commented Sep 27, 2023

t0yv0 commented Sep 27, 2023 • edited Loading

t0yv0 commented Sep 27, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

t0yv0 commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

codecov-commenter commented Sep 29, 2023 • edited Loading

Codecov Report

github-actions bot commented Sep 29, 2023

github-actions bot commented Sep 29, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

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

iwahbe commented Oct 3, 2023

t0yv0 commented Oct 3, 2023

github-actions bot commented Oct 3, 2023

github-actions bot commented Oct 3, 2023

iwahbe left a comment

Choose a reason for hiding this comment

t0yv0 commented Sep 26, 2023 •

edited

Loading

t0yv0 commented Sep 27, 2023 •

edited

Loading

codecov-commenter commented Sep 29, 2023 •

edited

Loading