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

Lookup remote outputs concurrently #1114

Merged
merged 2 commits into from Apr 4, 2020
Merged

Conversation

dmattia
Copy link
Contributor

@dmattia dmattia commented Apr 4, 2020

At transcend-io, we have >300 terragrunt modules with fairly deep dependency chains. In fact, our primary backend ECS module requires looking up the terraform output -json of 52 modules (after caching).

Here are some benchmarks of running the code on my local laptop. All benchmarks were taken on the module with 52 output calls, all after the terragrunt-cache was completely made to minimize the effects of downloading providers, external modules, etc.

Using terragrunt version v0.23.2:
terragrunt plan 53.33s user 12.51s system 28% cpu 3:54.80 total
terragrunt plan 52.82s user 12.22s system 26% cpu 4:04.88 total
terragrunt plan 54.26s user 12.33s system 30% cpu 3:38.67 total

Using a binary made from go build main.go:
~/transcend/terragrunt/main plan 59.45s user 12.95s system 91% cpu 1:18.80 total
~/transcend/terragrunt/main plan 58.19s user 13.15s system 90% cpu 1:18.77 total
~/transcend/terragrunt/main plan 57.66s user 12.78s system 83% cpu 1:23.94 total

This change adds a dependency on golang.org/x/sync, which is very useful for running concurrent operations with easy error handling

At transcend-io, we have >300 terragrunt modules with fairly deep dependency chains. In fact, our primary backend ECS module requires looking up the `terraform output -json` of 52 modules (after caching).

Here are some benchmarks of running the code on my local laptop. All benchmarks were taken on the module with 52 `output` calls, all after the terragrunt-cache was completely made to minimize the effects of downloading providers, external modules, etc.

Using `terragrunt version v0.23.2`:
terragrunt plan  53.33s user 12.51s system 28% cpu 3:54.80 total
terragrunt plan  52.82s user 12.22s system 26% cpu 4:04.88 total

Using a binary made from `go build main.go`:
~/transcend/terragrunt/main plan  59.45s user 12.95s system 91% cpu 1:18.80 total
~/transcend/terragrunt/main plan  58.19s user 13.15s system 90% cpu 1:18.77 total
~/transcend/terragrunt/main plan  57.66s user 12.78s system 83% cpu 1:23.94 total

This change adds a dependency on golang.org/x/sync, which is very useful for running concurrent operations with easy error handling
@dmattia
Copy link
Contributor Author

dmattia commented Apr 4, 2020

If you have any suggestions for adding tests I'd be happy to add them! From my quick analysis, it doesn't look like there is much existing testing infrastructure around dependency parsing, but I very well could have missed it as I'm very new to golang.

I'm not very familiar with testing go channels, as I just learned about channels about an hour ago, but I'd be glad to learn and add tests if you think it's a good idea

Copy link
Member

@yorinasub17 yorinasub17 left a comment

Thanks for your contribution! Have one minor style nit but otherwise, this looks good to me! Our existing test suite for dependencies will actually be sufficient as we have at least one fixture that gets multiple dependencies (https://github.com/gruntwork-io/terragrunt/blob/master/test/fixture-get-output/integration/app3/terragrunt.hcl) in a config.

Will kick off the build shortly so you have one test cycle.

// dependencyMap is the top level map that maps dependency block names to the encoded version, which includes
// various attributes for accessing information about the target config (including the module outputs).
dependencyMap := map[string]cty.Value{}
lock := sync.Mutex{}
g, _ := errgroup.WithContext(context.Background())
Copy link
Member

@yorinasub17 yorinasub17 Apr 4, 2020

Choose a reason for hiding this comment

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

NIT: We like to use descriptive variable names. Can you rename this to dependencyGroup or dependencyErrGroup?

Copy link
Contributor Author

@dmattia dmattia Apr 4, 2020

Choose a reason for hiding this comment

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

done!

@yorinasub17
Copy link
Member

yorinasub17 commented Apr 4, 2020

Build passed and update looks good, so going to merge. Thanks for the contribution!

@yorinasub17 yorinasub17 merged commit 7cf1056 into gruntwork-io:master Apr 4, 2020
1 check passed
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.

None yet

2 participants