fix: Ensure terraform tests have a cache path and logger #3161
Conversation
|
Looks like CI tests pass, but locally I keep running into: |
|
Here's another error I see locally, it's even more weird since the installation of |
|
Hmm, problem goes away when disabling parallel tests. Would indicate there's a concurrency problem with running multiple Terraform instances in parallel. Not sure why it only seems to manifests when setting the |
|
@kylecarbs @spikecurtis any ideas as to why this is happening? Do we just disable parallel tests for these cases? |
|
It appears that when the cachedir is not set, we don't set Terraform's By setting that directory, and setting at the top level, all the test cases are using the same cache dir, and perhaps terraform is not carefully built to handle non-exclusive access to that cache by multiple terraform processes. |
|
Ah, thanks for that insight @spikecurtis. While testing I did try to explicitly set However, that did not help. So based on what you wrote it would seem that indeed terraform is not being careful (enough). How about if we ensure each provisioner sets the terraform cache dir to a subdir, e.g. |
|
It does seem a shame that we can’t have multiple jobs share the plug-in cache, as this will limit performance/scale. Perhaps we should report the issue to Hashicorp and see what they say. Setting the cache to a subdir based on provisioner won’t unblock this PR because there is just one provisioner created at the top level of the test. And if you refactor to create new provisioners in the subtests, the issue will probably go away for now since we only run a single job at a time. |
|
Oh doh, that makes sense, thanks for saving me from that goose chase. I'll look into reporting this upstream. |
|
This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity. |
22886d3 to
ca6b211
Compare
|
@spikecurtis I found this in the documentation:
So it looks like a conscious choice not to support it. I've implemented a fix in One thing that worries me is that we could, potentially, still have multiple calls to |
If no cache path is set, then no cache is used. Each job gets its own directory, so it should not be possible to have multiple calls to One thing I am worried about though, is that we are in the habit of starting multiple provisionerds on the same system, so we'll need to take some care to ensure that they have independent cache directories (if set) since the locking is only in the context of a single provisioner. |
Yeah, that's a fair conclusion.
Good callout. This is something I had in mind too. In this case I was thinking we could assign each provisioner a subfolder within the cache, combined with the mutex that should solve the immediate issue. We'd need to assign each provisioner an ID though. Perhaps a numerical ID from |
This PR sets cache path for terraform in tests.
However, for some reason this does not work.
Related: #3159