Allow multiple processes to run terraform providers mirror concurrently with the same target directory

[bgfraser]

Terraform Version

v1.2.8

Use Cases

Using ls -d modules/*/ | xargs -P ... to run parallel terraform providers mirror processes against a large local module directory has some failures due to the concurrent processing of the archive files in the stagingPath location.

Attempted Solutions

No solution available within existing features. Any parallel implementation of terraform providers mirror on the same filesystem risks failures if two threads are processing the same provider concurrently. Examples of this error in cli output:

Error: Invalid provider package

Failed to authenticate registry.terraform.io/hashicorp/google v4.28.0 for linux_amd64: archive has incorrect checksum zh:848fc2d01238e0460a3a6162bab9f3b86674a5516424c12827a1954b61b381f7 (expected
zh:2993a3ba417c576ca9c6adccb6a6e914b4dedd3f91a735fd14ab8910936d8c11).

Error: Invalid provider package

Failed to authenticate registry.terraform.io/hashicorp/google-beta v4.28.0 for linux_amd64: failed to compute checksum for
../../providers-mirror-xargs/registry.terraform.io/hashicorp/google-beta/.terraform-provider-google-beta_4.28.0_linux_amd64.zip: lstat
../../providers-mirror-xargs/registry.terraform.io/hashicorp/google-beta/.terraform-provider-google-beta_4.28.0_linux_amd64.zip: no such file or directory.

Proposal

If stagingPath,

terraform/internal/command/providers_mirror.go

Line 189 in 4fb8a5a

stagingPath := filepath.Join(filepath.Dir(targetPath), "."+filepath.Base(targetPath))

could be supplied as an optional flag then it could be passed in by xargs with a unique prefix or suchlike, allowing for a unique staging location per process.

References

No response

[apparentlymart]

Hi @bgfraser! Thanks for this feature request.

After reviewing this code, I believe that it's a requirement that stagingPath be in the same directory as the target path because that is what allows this code to perform an atomic rename of the directory to place it at its final location without creating a time window where other concurrent process might observe a partially-populated directory.

However, I think it could be reasonable to change the process for generating that path so that it includes a component that is likely to be unique for each concurrent process. Currently it just prepends a dot, and so all processes generate the same temporary path. It could instead prepend a string which includes a pseudorandom number, or the current process ID, or something along those lines so that the staging directory still belongs to the correct parent directory but has a different name in each process.

I think supporting concurrent work here has an additional requirement too: in the final step where the command calls os.Rename to move the directory into its final location, it's likely that some of the concurrent processes will see a "file already exists" error due to the many processes all racing to be the one to create that directory entry. I think we'd want to update this logic so that it no longer calls os.Remove first -- allowing any existing directory to remain -- and then also has a special case for when os.IsExist(err) returns true. If that function does return true then we should check to make sure the existing target directory matches one of the expected checksums, and treat it as a success if it does,

---

There is a similar discussion over in #31964 about introducing some similar logic for the global cache directory, so that multiple concurrent writers can cooperate to populate the cache. Over there I shared some concerns about making this implementation safe across all of the platforms Terraform supports, and I think that same concern also applies here but it's helpful to see that terraform providers mirror is already doing part of that proposal (atomically renaming a directory into place) and so hopefully we can implement the rest of this for terraform providers mirror first and get confident that's working well, and then if so we can follow exactly the same steps for the global cache directory too.

(My sense is that it's lower risk to try this in terraform providers mirror because it's outside of the critical path for Terraform's primary workflow, whereas the plugin cache directory is handled as part of terraform init and so any new regressions there would potentially block everyday Terraform work.)

[bgfraser]

Thank you @apparentlymart for the considered and positive response, and apologies for my slow response! I very rarely use GH public so I only just saw it. I'll have a poke around over the EOY break and see if I can come up with a workable modification.