Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize edge deployment backend updates via mutex #213

Merged

Conversation

jonnangle
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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 daniel-corbett self-requested a review March 27, 2024 16:19
@daniel-corbett daniel-corbett merged commit cd3b5be into signalsciences:main Mar 27, 2024
2 checks passed
@daniel-corbett
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with concurrent sigsci_edge_deployment_service_backend updates
2 participants