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

Add --limit flag to pulumi stack history and consume from automation api #6257

Merged
merged 5 commits into from
Feb 8, 2021

Conversation

EvanBoyle
Copy link
Contributor

@EvanBoyle EvanBoyle commented Feb 5, 2021

We had some reports of perf issues related to pulumi stack history. Specifically we use this command to fetch update summary after an update with automation API. As you perform more updates, performance degrades as we fetch more data and decrypt more secrets. This change does two things:

  1. add a --limit=N flag to pulumi history and pulumi stack history to allow only fetching the most recent N records.
  2. consume this flag in the appropriate places in the Automation API SDKs for node, go, and python.

Note that this is technically a small breaking change for Go (other languages mark this as optional and previous SDK shape is still valid) but this is within the boundaries we've set for the alpha and is an important change to make for performance.

We don't have any infrastructure setup to test the filestate backend changes. There are tests that create stacks, select stacks, but none that go through the lifecycle of create, do an update, etc. Adding this was outside of what I have bandwidth for, but I tested this workflow locally with the filestate backend. See #3760.

Fixes #6253
Fixes #6252

Note this will need to be ported to C# after that lands.

Copy link
Member

@komalali komalali left a comment

Choose a reason for hiding this comment

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

LGTM except for a funky conditional.

Comment on lines +337 to +338
}
limit = len(allFiles) - limit
Copy link
Member

Choose a reason for hiding this comment

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

I think we're missing an else here.

Suggested change
}
limit = len(allFiles) - limit
} else {
limit = len(allFiles) - limit
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The condition is a little funky here, but it is written as intended. The first condition is the equivalent of min(limit, len(allFiles) incase the limit is larger than the total number of results:

		if limit > len(allFiles) {
			limit = len(allFiles)
		}

After that, we adjust the loop condition:

limit = len(allFiles) - limit

Copy link
Member

@komalali komalali Feb 8, 2021

Choose a reason for hiding this comment

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

limit = 10, len(allFiles) = 5

if 10 > 5 {
	limit = 5
}
limit = 5 - 5 = 0

So if the limit is larger than the total number of results we would end up with limit=0? Does limit=0 actually mean "no limit"?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I get it after looking at the code that follows.

@EvanBoyle EvanBoyle merged commit eefc104 into master Feb 8, 2021
@pulumi-bot pulumi-bot deleted the evan/limitHistory branch February 8, 2021 18:50
@cnunciato
Copy link
Member

Does this also (kinda) fix #6168?

@komalali
Copy link
Member

komalali commented Feb 9, 2021

@cnunciato it sure (kinda) does, thanks for bringing it up!

EvanBoyle added a commit that referenced this pull request Feb 10, 2021
orionstudt added a commit to orionstudt/pulumi that referenced this pull request Feb 12, 2021
mikhailshilkov added a commit that referenced this pull request Feb 18, 2021
* Init Workspace interface for C# Automation API

* fleshing out workspace interface and beginning of local workspace implementation

* initial run pulumi cmd implementation

* resolve issue with pulumi cmd cleanup wrapper task after testing

* flesh out local workspace implementation, flesh out stack implementation, cleanup run pulumi cmd implementation and make it an instance so it is mockable/testable, separate serialization in prep for custom converters

* project settings json serialization implemented

* Initial commit of language server

* Add deployment from language server

* Cleanup

* finish json serialization

* project runtime yaml serialization completed. just need stack config value yaml serialization

* Remove typed argument

* Limit concurrency

* Rename file for consistency

* final commit of a semi-working project settings & stack settings serialization so that it is in the commit history

* modify workspace API so that settings accessors aren't fully exposed since we are defering a complete serialization implementation until a later date

* yaml converters wrap any outgoing exceptions so resolve that

* getting the beginning of inline program GRPC communication set up

* stack lifecycle operations implemented, and switched to newtonsoft for JSON serialization

* change back to system.text.json with a custom object converter

* local workspace tests written, working on getting them passing

* fix the encoding on the GO files used for testing

* all tests passing except inline program, pulumi engine not available with inline

* inline program engine is now running as expecting, but inline program is not recognizing local stack config

* All tests passing, but no concurrency capability because of the singleton DeploymentInstance.

* cleanup unnecessary usings

* minor cleanup / changes after a quick review. Make sure ConfigureAwait is used where needed. Remove newtonsoft dependency from testing. Update workspace API to use existing PluginKind enum. Modify LanguageRuntimeService so that its semaphore operates process-wide.

* support for parallel execution of inline program, test included

* Update LocalWorkspaceTests.cs

remove some redundancy from the inline program parallel execution text

* flesh out some comments and make asynclocal instance readonly

* Strip out instance locking since it is no longer necessary with AsyncLocal wrapping the Deployment.Instance. Modify CreateRunner method such that we are ensuring there isn't a chance of delayed synchronous execution polluting the value of Deployment.Instance across calls to Deployment.RunAsync

* resolve conflicts with changes made to Deployment.TestAsync entrypoints

* update changelog

* write a test that fails if the CreateRunnerAndRunAsync method on Deployment is not marked async and fix test project data file ref

* make resource package state share the lifetime of the deployment so that their isn't cross deployment issues with resource packages, add support and tests for external resource packages (resource packages that aren't referenced by the executing assembly)

* enable parallel test collection execution in test suite, add some additional tests for deployment instance protection and ensuring that our first class stack exceptions are thrown when expected

* minor inline project name arg change, and re-add xunit json to build output (whoops)

* strip out concurrency changes since they are now in #6139, split automation into separate assembly, split automation tests into separate assembly

* add copyright to the top of each new file

* resolve some PR remarks

* inline program exception is now properly propagated to the caller on UpAsync and PreviewAsync

* modify PulumiFn to allow TStack and other delegate overloads without needing multiple first class delegates.

* whoops missing a copyright

* resolve getting TStack into IRunner so that outputs are registered correctly and so that there isn't 2 instances of Pulumi.Stack instantiated.

* resolve issue with propagation of TStack exceptions and add a test

* add support for a TStack PulumiFn resolved via IServiceProvider

* update automation API description

* fix comment and remove unnecessary TODOs

* disable packaging of automation api assembly

* re-name automation api documentation file appropriately

* add --limit support to dotnet automation api for stack history per #6257

* re-name XStack as WorkspaceStack

* replace --limit usage with --page-size and --page in dotnet automation api per #6292

Co-authored-by: evanboyle <evan@pulumi.com>
Co-authored-by: Josh Studt <josh.studt@figmarketing.com>
Co-authored-by: Dan Friedman <dan@thefriedmans.org>
Co-authored-by: David Ferretti <David.Ferretti@figmarketing.com>
Co-authored-by: Mikhail Shilkov <github@mikhail.io>
abhinav pushed a commit to pulumi/pulumi-dotnet that referenced this pull request Jan 11, 2023
* Init Workspace interface for C# Automation API

* fleshing out workspace interface and beginning of local workspace implementation

* initial run pulumi cmd implementation

* resolve issue with pulumi cmd cleanup wrapper task after testing

* flesh out local workspace implementation, flesh out stack implementation, cleanup run pulumi cmd implementation and make it an instance so it is mockable/testable, separate serialization in prep for custom converters

* project settings json serialization implemented

* Initial commit of language server

* Add deployment from language server

* Cleanup

* finish json serialization

* project runtime yaml serialization completed. just need stack config value yaml serialization

* Remove typed argument

* Limit concurrency

* Rename file for consistency

* final commit of a semi-working project settings & stack settings serialization so that it is in the commit history

* modify workspace API so that settings accessors aren't fully exposed since we are defering a complete serialization implementation until a later date

* yaml converters wrap any outgoing exceptions so resolve that

* getting the beginning of inline program GRPC communication set up

* stack lifecycle operations implemented, and switched to newtonsoft for JSON serialization

* change back to system.text.json with a custom object converter

* local workspace tests written, working on getting them passing

* fix the encoding on the GO files used for testing

* all tests passing except inline program, pulumi engine not available with inline

* inline program engine is now running as expecting, but inline program is not recognizing local stack config

* All tests passing, but no concurrency capability because of the singleton DeploymentInstance.

* cleanup unnecessary usings

* minor cleanup / changes after a quick review. Make sure ConfigureAwait is used where needed. Remove newtonsoft dependency from testing. Update workspace API to use existing PluginKind enum. Modify LanguageRuntimeService so that its semaphore operates process-wide.

* support for parallel execution of inline program, test included

* Update LocalWorkspaceTests.cs

remove some redundancy from the inline program parallel execution text

* flesh out some comments and make asynclocal instance readonly

* Strip out instance locking since it is no longer necessary with AsyncLocal wrapping the Deployment.Instance. Modify CreateRunner method such that we are ensuring there isn't a chance of delayed synchronous execution polluting the value of Deployment.Instance across calls to Deployment.RunAsync

* resolve conflicts with changes made to Deployment.TestAsync entrypoints

* update changelog

* write a test that fails if the CreateRunnerAndRunAsync method on Deployment is not marked async and fix test project data file ref

* make resource package state share the lifetime of the deployment so that their isn't cross deployment issues with resource packages, add support and tests for external resource packages (resource packages that aren't referenced by the executing assembly)

* enable parallel test collection execution in test suite, add some additional tests for deployment instance protection and ensuring that our first class stack exceptions are thrown when expected

* minor inline project name arg change, and re-add xunit json to build output (whoops)

* strip out concurrency changes since they are now in pulumi/pulumi#6139, split automation into separate assembly, split automation tests into separate assembly

* add copyright to the top of each new file

* resolve some PR remarks

* inline program exception is now properly propagated to the caller on UpAsync and PreviewAsync

* modify PulumiFn to allow TStack and other delegate overloads without needing multiple first class delegates.

* whoops missing a copyright

* resolve getting TStack into IRunner so that outputs are registered correctly and so that there isn't 2 instances of Pulumi.Stack instantiated.

* resolve issue with propagation of TStack exceptions and add a test

* add support for a TStack PulumiFn resolved via IServiceProvider

* update automation API description

* fix comment and remove unnecessary TODOs

* disable packaging of automation api assembly

* re-name automation api documentation file appropriately

* add --limit support to dotnet automation api for stack history per pulumi/pulumi#6257

* re-name XStack as WorkspaceStack

* replace --limit usage with --page-size and --page in dotnet automation api per pulumi/pulumi#6292

Co-authored-by: evanboyle <evan@pulumi.com>
Co-authored-by: Josh Studt <josh.studt@figmarketing.com>
Co-authored-by: Dan Friedman <dan@thefriedmans.org>
Co-authored-by: David Ferretti <David.Ferretti@figmarketing.com>
Co-authored-by: Mikhail Shilkov <github@mikhail.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants