Cache temporary folder #114

brikis98 · 2017-01-29T18:45:35Z

In #111, I added the ability for Terragrunt to download Terraform files from a given source URL. The implementation in that PR downloaded the files to a different temp folder every time. As a result, it also had to run terraform get and terraform remote config from scratch every time. All together, this added a significant amount of overhead to every single Terraform command.

In this PR, I reuse the same temp folder for a given source URL. The exact caching logic is a bit complicated, as there are different cases to handle for local source URLs vs remote source URLs vs remote source URLs with version numbers (e.g. ref=v0.0.3). The result is you only have to download the source code once on your computer. After that, everything is exactly as fast as the normal way of using Terraform.

… path

brikis98 · 2017-01-30T01:30:33Z

cli/download_source_test.go

+	testDownloadTerraformSourceIfNecessary(t, canonicalUrl, downloadDir, "# Hello, World")
+}
+
+// TODO re-enable these two tests once this is merged into master, since they are trying to download code from master


Just a heads up that there are a few test cases here trying to download from the Terragrunt repo itself. Once this is merged to master, a few of these will need to tweaked to work correctly. It's a bit awkward to maintain, but very handy as an integration test to check that the caching works correctly.

Roger. Nice way of handling tests with git.

brikis98 · 2017-01-30T01:35:20Z

@josh-padnick Please review when you have a chance. This caching technique makes a pretty massive difference in usability. For example, for our Jenkins code, which relies on ~8 remote Terraform modules, without the caching, every single Terragrunt command has a ~35 second overhead, most of it spent running terraform get and terraform remote config. With the caching, the overhead is 0.

josh-padnick · 2017-01-31T18:13:12Z

Just reviewing this now, before I even get into the code, can we make the case that this feature of Terragrunt adds value above and beyond the new "supercharged" terraform init command (CTRL-F for "Features (External)")? Conceptually they seems very similar, though it's not clear to me how much caching they do.

brikis98 · 2017-01-31T18:21:56Z

Just reviewing this now, before I even get into the code, can we make the case that this feature of Terragrunt adds value above and beyond the new "supercharged" terraform init command (CTRL-F for "Features (External)")? Conceptually they seems very similar, though it's not clear to me how much caching they do.

Good question. I'm not sure until I actually use it, but I'm guessing they download code into the local folder as a way to bootstrap a new project. Our use case is to download code into a tmp folder as a way to deploy an immutable version of your code.

josh-padnick

Looks great overall. I think you made good decisions around how to cache the downloads (though see my comment about the case where the content behind the same URL has been altered).

Biggest open item for me is how this relates to terraform init in Terraform 0.9. But within the context of this PR, LGTM!

josh-padnick · 2017-01-31T18:23:36Z

cli/download_source.go

+		return false, nil
+	}
+
+	currentVersion := encodeSourceVersion(terraformSource.CanonicalSourceURL)


Looking at the definition of encodeSourceVersion(), you appear to be hashing the URL. But what if the URL is the same but its contents are different? This is the case, for example, when we re-release the same git tag. I believe this is the thought behind the -update in terraform get -update.

Yes, I considered this. In the future, if this is a common case, we could add a terragrunt-update or similar flag. For now, the trivial workaround is to delete the relevant tmp folder manually. My hope, however, is that this is a relatively rare case.

josh-padnick · 2017-01-31T18:26:17Z

cli/download_source.go

+//    (e.g. git::github.com/foo/bar//some-module) identifies the module name, and we always want to download the same
+//    module name into the same folder (see the encodeSourceName method). We also assume the version of the module is
+//    stored in the query string (e.g. ref=v0.0.3), so we store the base 64 encoded sha1 of the query string in a
+//    file called .terragrunt-source-version within S.


s/S is the base 64 encoded sha1 has of s/S is the base 64 encoded sha1 of s/

Fixed: f937e39

josh-padnick · 2017-01-31T18:45:49Z

cli/download_source.go

+		DownloadDir: downloadDir,
+		VersionFile: versionFile,
+	}, nil
+}


Wow, lots of nuance here. Nicely handled.

josh-padnick · 2017-01-31T18:48:36Z

cli/download_source.go

+	terragruntOptions.Logger.Printf("Downloading Terraform configurations from %s into %s", terraformSource.CanonicalSourceURL, terraformSource.DownloadDir)
+
+	terragruntInitOptions := terragruntOptions.Clone(terragruntOptions.TerragruntConfigPath)
+	terragruntInitOptions.TerraformCliArgs = []string{"init", terraformSource.CanonicalSourceURL.String(), terraformSource.DownloadDir}


Consider reviewing hashicorp/terraform#11286 for forward-compatibility.

Looks like init may be changed in a backwards incompatible way to be an interactive command. Not sure I can do much about that for now. One alternative is to try to use go-getter directly to reimplement the download functionality, but I'd rather wait and see if with the next version of Terraform, we can leverage init with special flags to do what we need.

josh-padnick · 2017-01-31T18:51:33Z

cli/download_source_test.go

+	testDownloadTerraformSourceIfNecessary(t, canonicalUrl, downloadDir, "# Hello, World")
+}
+
+// TODO re-enable these two tests once this is merged into master, since they are trying to download code from master


Roger. Nice way of handling tests with git.

josh-padnick · 2017-01-31T18:52:19Z

glide.lock

+  - service/mobileanalytics
+  - service/cloudfront/sign
+  - service/dynamodb/dynamodbiface
+  - service/sqs/sqsiface


Do we really need this many additional AWS services? Will this bloat the binary?

Gah. Stupid glide. All I did was add one damn dependency. Hopefully this new version works, and has far fewer extras: 15ed21e

josh-padnick · 2017-01-31T18:53:27Z

util/file.go

+		}
+	}
+	return nil
+}


Does this need to be able to handle recursive deletes?

It's designed solely to delete files. I've updated the comment to make that clearer: 931215d

josh-padnick · 2017-01-31T18:55:24Z

cli/download_source.go

+
+	// The path to a file in DownloadDir that stores the version number of the code
+	VersionFile         string
+}


Would be helpful to add a comment on what this struct as a whole represents.

Added: 6de18ef

josh-padnick · 2017-01-31T18:55:32Z

cli/download_source.go

+
+	// The path to a file in DownloadDir that stores the version number of the code
+	VersionFile         string
+}


Would be helpful to add a comment on what this struct as a whole represents. It took me a minute to figure out this represents a set of "source code" for Terraform.

Added: 6de18ef

josh-padnick · 2017-01-31T19:01:25Z

cli/download_source.go

+	rawSourceUrl, err := getter.Detect(source, canonicalWorkingDir, getter.Detectors)
+	if err != nil {
+		return nil, errors.WithStackTrace(err)
+	}


Good call using go-getter to normalize URLs. Looks like a great library.

It's actually the library Terraform/Packer/etc use to download files, including the terraform init command.

brikis98 · 2017-02-01T00:09:31Z

Thanks for the feedback, merging now.

brikis98 added 5 commits January 27, 2017 17:47

Re-use temporary folder for source downloads

0e5b907

Only download files if the source path has changed or is a local file…

354c2db

… path

Use sha1 hash of file path

ccf316e

Cache bust only if version changes. Add unit tests.

d60f47c

Fix download-source test cases

ab12052

brikis98 commented Jan 30, 2017

View reviewed changes

josh-padnick approved these changes Jan 31, 2017

View reviewed changes

brikis98 added 4 commits January 31, 2017 23:52

Add comments on TerraformSource struct

6de18ef

Fix typo in download_source comments

f937e39

Fix glide.lock

15ed21e

Update comment on DeleteFiles method

931215d

brikis98 merged commit f8ad6c9 into master Feb 1, 2017

brikis98 deleted the cache-tmp-folder branch February 1, 2017 00:09

brikis98 mentioned this pull request Feb 1, 2017

Add a --terragrunt-source-update flag #117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache temporary folder #114

Cache temporary folder #114

brikis98 commented Jan 29, 2017 •

edited

Loading

brikis98 Jan 30, 2017

josh-padnick Jan 31, 2017

brikis98 commented Jan 30, 2017 •

edited

Loading

josh-padnick commented Jan 31, 2017

brikis98 commented Jan 31, 2017

josh-padnick left a comment

josh-padnick Jan 31, 2017

brikis98 Jan 31, 2017

josh-padnick Jan 31, 2017

brikis98 Jan 31, 2017

josh-padnick Jan 31, 2017

josh-padnick Jan 31, 2017

brikis98 Jan 31, 2017

josh-padnick Jan 31, 2017

josh-padnick Jan 31, 2017

brikis98 Feb 1, 2017

josh-padnick Jan 31, 2017

brikis98 Feb 1, 2017

josh-padnick Jan 31, 2017

brikis98 Jan 31, 2017

josh-padnick Jan 31, 2017

brikis98 Jan 31, 2017

josh-padnick Jan 31, 2017

brikis98 Jan 31, 2017

brikis98 commented Feb 1, 2017

Cache temporary folder #114

Cache temporary folder #114

Conversation

brikis98 commented Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brikis98 commented Jan 30, 2017 • edited Loading

josh-padnick commented Jan 31, 2017

brikis98 commented Jan 31, 2017

josh-padnick left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brikis98 commented Feb 1, 2017

brikis98 commented Jan 29, 2017 •

edited

Loading

brikis98 commented Jan 30, 2017 •

edited

Loading