signalsciences / terraform-provider-sigsci

Signal sciences terraform provider
MIT License
26 stars 30 forks source link

Serialize edge deployment backend updates via mutex #213

Closed jonnangle closed 7 months ago

jonnangle commented 7 months ago

Fixes #201

When multiple calls to edgeDeployment/<sid>/backends occur simultaneously for different service IDs during a single Terraform apply, it's common for one to fail with the error message: Error: failed to activate service. This issue seems to stem from an API constraint.

This PR introduces a provider mutex that can be used to serialize access to this API call (and could also be used for others, if required). This avoids the need to set -parallelism=1 globally, which we'd prefer not to do when running alongside the fastly provider, because it would unnecessarily limit the throughput of that provider as well.

jonnangle commented 7 months ago

Current behaviour:

module.test-2.fastly_service_vcl.this: Modifying... [id=id001]
module.test.fastly_service_vcl.this: Modifying... [id=id002]
module.test-2.fastly_service_vcl.this: Still modifying... [id=id001, 10s elapsed]
module.test.fastly_service_vcl.this: Still modifying... [id=id002, 10s elapsed]
module.test-2.fastly_service_vcl.this: Modifications complete after 13s [id=id001]
module.test-2.sigsci_edge_deployment_service_backend.this[0]: Modifying... [id=id001]
module.test.fastly_service_vcl.this: Modifications complete after 14s [id=id002]
module.test.sigsci_edge_deployment_service_backend.this[0]: Modifying... [id=id002]
module.test-2.sigsci_edge_deployment_service_backend.this[0]: Still modifying... [id=id001, 10s elapsed]
module.test-2.sigsci_edge_deployment_service_backend.this[0]: Modifications complete after 10s [id=id001]
module.test.sigsci_edge_deployment_service_backend.this[0]: Still modifying... [id=id002, 10s elapsed]
╷
│ Error: failed to activate service
│
│   with module.test.sigsci_edge_deployment_service_backend.this[0],
│   on .terraform/modules/test/terraform/modules/services/fastly-service/main.tf line 552, in resource "sigsci_edge_deployment_service_backend" "this":
│  552: resource "sigsci_edge_deployment_service_backend" "this" {

With this PR:

module.test-2.fastly_service_vcl.this: Modifying... [id=id001]
module.test.fastly_service_vcl.this: Modifying... [id=id002]
module.test-2.fastly_service_vcl.this: Still modifying... [id=id001, 10s elapsed]
module.test.fastly_service_vcl.this: Still modifying... [id=id002, 10s elapsed]
module.test.fastly_service_vcl.this: Modifications complete after 14s [id=id002]
module.test.sigsci_edge_deployment_service_backend.this[0]: Modifying... [id=id002]
module.test-2.fastly_service_vcl.this: Modifications complete after 15s [id=id001]
module.test-2.sigsci_edge_deployment_service_backend.this[0]: Modifying... [id=id001]
module.test.sigsci_edge_deployment_service_backend.this[0]: Modifications complete after 2s [id=id002]
module.test-2.sigsci_edge_deployment_service_backend.this[0]: Still modifying... [id=id001, 10s elapsed]
module.test-2.sigsci_edge_deployment_service_backend.this[0]: Modifications complete after 15s [id=id001]

Apply complete! Resources: 0 added, 4 changed, 0 destroyed.
daniel-corbett commented 7 months ago

Hi @jonnangle - thanks for the submission!

This avoids the need to set -parallelism=1 globally

We are happy to accept this PR but I think there may be a bit of a misunderstanding here so just wanted to ensure we are on the same page.

Even after this PR is merged the overall provider still requires parallelism=1. This guideline was specified in the FAQ well before the Edge deployment code was added. In fact, it goes back to this providers inception. There are larger changes and fixes (likely on the backend API) that need to happen to remove that requirement.

jonnangle commented 7 months ago

Hi @daniel-corbett, thanks for this!

the overall provider still requires parallelism=1.

That's understood 👍 We typically set parallelism=1 for almost all of our Signal Sciences configuration work - in most cases, this provider is the only one in the mix. We'll definitely continue to do that moving forward.

daniel-corbett commented 7 months ago

New release 2.1.8 includes this PR. Thanks again for your contribution!