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

Fix potential deadlocks among provider level #15396

Merged
merged 1 commit into from
Feb 12, 2022

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Feb 12, 2022

Fixes #15355

Implemented a small tool https://github.com/magodo/terraform-provider-azurerm-deadlock to ensure this change here won't affect the other places. The only output of above tool is as below:

Potential deadlock:
  azurerm_subnet->azurerm_virtual_network:
        - internal/services/network/network_interface_locking.go:15:1
  azurerm_virtual_network->azurerm_subnet:
        - internal/services/firewall/firewall_resource.go:222:1
        - internal/services/firewall/firewall_resource.go:439:1
        - internal/services/network/network_profile_resource.go:198:1
        - internal/services/network/network_profile_resource.go:99:1
        - internal/services/network/subnet_nat_gateway_association_resource.go:55:1
        - internal/services/network/subnet_network_security_group_association_resource.go:184:1
        - internal/services/network/subnet_network_security_group_association_resource.go:55:1
        - internal/services/network/subnet_resource.go:285:1
        - internal/services/network/subnet_resource.go:460:1
        - internal/services/redis/redis_cache_resource.go:339:1
        - internal/services/redis/redis_cache_resource.go:685:1
        - internal/services/web/app_service_slot_virtual_network_swift_connection_resource.go:210:1
        - internal/services/web/app_service_slot_virtual_network_swift_connection_resource.go:60:1
        - internal/services/web/app_service_virtual_network_swift_connection_resource.go:186:1
        - internal/services/web/app_service_virtual_network_swift_connection_resource.go:53:1

This reveals that among the provider, it always lock the azurerm_virtual_network resource type prior to the azurerm_subnet, except internal/services/network/network_interface_locking.go.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @magodo ! that is an awesome tool, i wonder if it is something that would make sense to incorporate as a linter? and use across different azure providers? i'm thinking stack may benefit from this as well? LGTM 🙌

@katbyte katbyte merged commit 9743c0f into hashicorp:main Feb 12, 2022
@github-actions github-actions bot added this to the v2.97.0 milestone Feb 12, 2022
@magodo
Copy link
Collaborator Author

magodo commented Feb 13, 2022

@katbyte Thank you for merging this! I didn't think of checking the dead lock for stack/azuread provider. This tool is much dependent on the way how the azurerm provider code constructed. Luckily, at this point of time the tool can detect all the places that a locks.(Multiple)ByName is called (the amount of places matches grep result). However, it might quickly miss some of them or even failed to get the resource type the it is trying to lock (maybe) due to it is not a trivial const string any more. So if we use it as a linter, it might fail itself if the code refactors in some way, and needs effort to maintain. Instead, to make it as a adhoc tool to do one time check, and then keep that lock order in mind when later doing review or development should be more feasible?

@github-actions
Copy link

This functionality has been released in v2.97.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NIC and Subnet-NSG-Association creation might dead lock
2 participants