Unpack TF plugins in a more atomic way.

[mplzik]

The original implementation of plugin cache handling unpacks plugin
archives in a way that can result in race conditions when multiple
terraform processes are accessing the same plugin cache. Since the
archives are being decompressed in chunks and output files are written
directly to the cache, we observed following manifestations of the issue:

• text file busy errors if other terraform processes are already
  running the plugin and another process tries to replace it.
• various plugin checksum errors triggered likely by simultaneous checksumming
  and rewriting the file.

This PR changes the zip archives with plugins are handled:

1. Instead of writing directly to the target directory,
   installFromLocalArchive is now writing to a temporary staging
   directory prefixed with.temp that resides in the plugin cache dir
   to ensure this is on the same filesystem.
2. After unpacking, the directory structure of the staging directory is
   replicated in the targetDir. The directories are created as needed
   and any files are moved using os.Rename. After this, the staging
   directory is removed.
3. Since the temporary staging directories can be picked up by
   SearchLocalDirectory and make it fail in the situation when they're
   removed during directory traversal, the function has been modified to
   skip any entry that starts with dot.

Signed-off-by: Milan Plzik [email protected]

Fixes #31964

Target Release

1.5.x

Draft CHANGELOG entry

BUG FIXES

• Concurrent write access to plugin cache directory has been fixed.

[hashicorp-cla]

All committers have signed the CLA.

[crw]

Thanks for this submission, I will raise it in triage.

[brandon-fryslie]

Oh please this would make me so happy, thanks

[crw]

Thanks for the reminder, and thanks @mplzik again for the submission.

The review from triage is that this change would require more due diligence. One issue with this implementation is that it relies on os.rename being atomic, which it is not. This may make the concurrency issue worse on different platforms.

We also noticed that the documentation should call out more clearly that the plugin cache is not concurrency-safe.

I'll leave this PR open for discussion for the moment, but will likely close it after a time as the feeling was it would be better to redesign a concurrency-safe plugin cache, which would require more discussion and prioritization.

Thanks again for this submission!

[mplzik]

@crw I definitely agree that this is not solution to all the problems -- hence my comment // On supported platforms, this should perform atomic replacement of the file., with supported meaning platforms where this is supported for os.Rename. Speaking of this, would hiding this change behind a (default off) flag would make this PR more acceptable? I understand that implementing a really atomic cache would be preferred, but likely also entails much bigger engineering time investment (and it hasn't been implemented for a longer time, suggesting it might not be trivial).

Let me also do a bit of an argument in favor of merging this PR from the risk assessment side.

The golang documentation is not going too far on describing os.Rename non-atomicity, but https://stackoverflow.com/questions/30385225/is-there-an-os-independent-way-to-atomically-overwrite-a-file goes a bit farther in describing the platform-specific behavior. Specifically (assuming files on the same volume):

• On Linux, this operations should be atomic (as much as the underlying filesystems can guarantee), translating into Renameat syscall
• On Windows, the golang implementation uses MoveFileEx, which according to information available is trying to do atomic replace if applicable, resorting to delete/copy operation if this is not supported. (see https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/449bb49d-8acc-48dc-a46f-0760ceddbfc3/movefileexmovefilereplaceexisting-ntfs-same-volume-atomic, Doug's answer)
• On MacOS, this translates to Rename syscall which is supposed to be atomic.

I see two main differences:

• The renaming approach uses more disk space temporarily, so some real-world setups that are on almost full disks might be tipped to exceeding available disk space. This might, however, happen also with increasing size of the providers.
• In the windows case, the file might be deleted (and thus stop existing) for a short period of time if atomic rename is not supported. I believe this will trigger mostly the same issues that are manifesting now (although I can't provide more data to prove that -- our infrastructure is not using any Windows machines to run Terraform).

[joaocc]

Hi. Just some 0.02€ from userland. We (and many others using terraform directly or via wrappers - in my case terragrunt) are having a terrible and very painfull time due to the concurrency issues, stopping us from moving ahead, and causing uncertainty in processes which should be quite trivial.
I would tend to agree with @mplzik in that, even if this is not 100% bulletprof, the fact that it is supposed to work much better on linux and (from what I understood) on Windows, means that a huge number of users should have their experience improved, and get back to using terraform in a productive and streamlined manner.
If in the future there is an "even better way" (TM) to improve the cache in more complex scenarios (like network file systems), then it would be also very welcomed, of course. But in the meantime, the benefits would already be rolled out.
Thanks

[brandon-fryslie]

I'm failing to understand the rationale behind blocking a small, simple change that would fix the problem for the vast majority of users in favor of holding out for an unknown, unplanned bulletproof solution that (likely) no one is working on. I'm finding it difficult to believe this could have been fixed for months already and yet it just sits here getting stale.

You can always revert the PR when you implement the really badass perfect plugin cache that you're now committed to implementing. And if you're not committed to it, why block this? Can we at least get a timeline of when you expect to release the real fix?

[crw]

First, apologies to @mplzik for the late response on this, the team appreciates your thoughtful response on this PR. The team is still concerned about edge cases with this solution. Speaking based on my own observations of how the maintainer team considers PRs, I think this is a difficult section of code to change from outside due to plugin cache management being in the critical path for every user.

@brandon-fryslie, to answer your question, the rationale is that the primary path to run Terraform is in sequence, not in parallel. The plugin cache is not concurrency safe. You are correct to assume no one is working on a more robust solution, as making the plugin cache safe for concurrent execution of Terraform is not currently a priority for the team. I'll re-raise this PR with our product manager to see if concurrency-safety for the plugin cache is something we can prioritize. Given that Terraform, as a whole, is not meant to be run in parallel, this likely has considerations beyond the plugin cache. However, it is always possible we will need to prioritize this as we add new features going forward.

Thanks for your feedback and continued interest in this feature.

[expnch]

the primary path to run Terraform is in sequence, not in parallel.

Just to chime in - running Terraform in parallel (for different projects that use the same modules) is a huge part of our CI/CD workflow.

[liamcervante]

Presumably we could get rid of the need to refactor the SearchLocalDirectory function if we wrote the files into a directory created by os.TempDir() instead of inside our cache? Any reason not to do that?