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

Start policy packs in parallel #14495

Merged
merged 6 commits into from Nov 20, 2023
Merged

Start policy packs in parallel #14495

merged 6 commits into from Nov 20, 2023

Conversation

tgummerer
Copy link
Collaborator

@tgummerer tgummerer commented Nov 3, 2023

Currently we start up and thus compile policy packs one by one. When multiple policy packs need to be loaded, this increases the start up time substantially. In previous tests plugins took ~1-3s to start up, so having multiple of these the time adds up quickly.

Loading them in parallel will help reduce the startup time here.

Fixes #14454

Checklist

  • I have run make tidy to update any new dependencies
  • I have run make lint to verify my code passes the lint check
    • I have formatted my code using gofumpt
  • 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 Cloud API version

Currently we start up and thus compile policy packs one by one.  When
multiple policy packs need to be loaded, this increases the start up
time substantially.  In previous tests plugins took ~1-3s to start up,
so having multiple of these the time adds up quickly.

Loading them in parallel will help reduce the startup time here.
@tgummerer tgummerer requested a review from a team November 3, 2023 16:16
@pulumi-bot
Copy link
Contributor

pulumi-bot commented Nov 3, 2023

Changelog

[uncommitted] (2023-11-20)

Features

  • [cli/plugin] Load policy packs in parallel on startup to reduce startup time
    #14495

Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

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

Does this not play havoc with the UX given you'll have multiple instance of npm/pip installing things in parallel writing to the same terminal?

@tgummerer
Copy link
Collaborator Author

Does this not play havoc with the UX given you'll have multiple instance of npm/pip installing things in parallel writing to the same terminal?

Yeah good point. I pushed up a commit that avoids doing the installation step in parallel, where npm/pip installing is being run, but that still loads the policies in a goroutine. This won't result in much of a performance improvement when policies need to be installed, though that should be the much less common case, as I'd expect users to usually have the policies cached locally already, and they only need to be run.

@Frassle
Copy link
Member

Frassle commented Nov 13, 2023

Yeh that seems like a good balance to start with.

Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

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

This looks reasonable, but do we have any tests that actually load multiple policy packs to ensure this parallel load doesn't race anything?

@tgummerer
Copy link
Collaborator Author

This looks reasonable, but do we have any tests that actually load multiple policy packs to ensure this parallel load doesn't race anything?

I'm actually struggling to find any tests that load even a single policy pack, so I'm gonna look into adding tests.

@tgummerer
Copy link
Collaborator Author

I added a few tests here, though they are all integration tests. I felt like that made most sense, but I'd be curious to hear if anyone has other ideas on how to test this.

@Frassle Frassle changed the title start policy packs in parallel Start policy packs in parallel Nov 16, 2023
@tgummerer tgummerer added this pull request to the merge queue Nov 16, 2023
@Frassle Frassle removed this pull request from the merge queue due to a manual request Nov 16, 2023
@tgummerer tgummerer added this pull request to the merge queue Nov 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 16, 2023
@tgummerer tgummerer added this pull request to the merge queue Nov 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2023
@tgummerer tgummerer added this pull request to the merge queue Nov 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2023
We have utf-8 characters in the command line, and should be supporting
testing those.  Explicity specify utf-8 encoding when reading the
integration test files, so we don't fail on some platform where the
default encoding might not allow those characters.
@tgummerer tgummerer added this pull request to the merge queue Nov 20, 2023
Merged via the queue into master with commit 13371f9 Nov 20, 2023
44 checks passed
@tgummerer tgummerer deleted the tg/start-plugins-parallel branch November 20, 2023 14:38
github-merge-queue bot pushed a commit that referenced this pull request Nov 30, 2023
### Features

- [cli/config] Include config values from ESC in `pulumi config`
  [#14560](#14560)

- [cli/config] Add commands for managing stack environments
  [#14628](#14628)

- [cli/config] Add a command to create an ESC environment from stack
config
  [#14634](#14634)

- [sdk/go] add optional display name and tag fields to project templates
  [#14587](#14587)

- [cli/plugin] Load policy packs in parallel on startup to reduce
startup time
  [#14495](#14495)

- [sdkgen/{go,nodejs,python}] Resource methods with plain: true outputs
can now return plain values without an Output wrapper. In particular,
this feature enables resource methods to serve as explicit provider
factories by returning preconfigured explicit providers.
  [#13592](#13592)


### Bug Fixes

- [auto/go] Fix a datarace in cloning git repos.
  [#14643](#14643)

- [auto/go] Fixes event stream lag on windows runtime
  [#14659](#14659)

- [engine] Engine now correctly handles any resource name.
  [#14107](#14107)

- [engine] Fix a panic in cancellation.
  [#14612](#14612)

- [engine] Fix root directory passed to langauge plugins when starting
pulumi in a subfolder.
  [#14684](#14684)

- [sdkgen] Schemas now validate that 'urn' and 'id' are not used as
resource output properties.
  [#14637](#14637)

- [sdkgen] Fixes marshalling the "plain" flag from object or resource
properties
  [#14648](#14648)

- [programgen/nodejs] Fix generated readFile function so that it
includes the encoding and returns a string
  [#14633](#14633)

- [sdkgen/{dotnet,go,nodejs,python}] No longer writing out name and
project from alias definitions into SDKs, only type
  [#14625](#14625)

- [sdk/go] Fix optional handling on nested props
  [#14629](#14629)

- [sdkgen/go] Fixes plain and optional properties for generated types
for Go SDKs using generics
  [#14616](#14616)

- [sdkgen/go] Generate non-plain type variants for types used as inputs
inside unions
  [#14679](#14679)

- [sdk/python] Introduces RuntimeError when we detect a cycle upon
adding dependencies to the graph. Additionally adds
"PULUMI_ERROR_ON_DEPENDENCY_CYCLES" as a new environment variable to
control this behavior. Set to `False` to return to the previous
behavior, which could potentially re-introduce infinite hangs for some
programs.
  [#14597](#14597)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate parallelizing exuction of policy packs
3 participants