Fixing deadlock in subnet resources

[Moeser]

This is meant to fix #2489

[katbyte]

this is #3405 reopened

[Moeser]

this is #3405 reopened

Yah, 3405 somehow got stuck in a blackhole. This is my attempt at pulling it out of the event horizon and into view :)

[katbyte]

Hey @Moeser,

Sorry for the delay in reviewing both of these PR's - the main cause of the delay here is the complexity in testing it. These locks were introduced to work around a number of bugs in the Azure API's when using multiple resources in the same Networking stack at the same time (see one of the previous bug reports for more information).

As a part of this work we ended up doing a ton of testing in this area to confirm that the resources were locking only on the elements they needed (at least, at that time). Unfortunately we also learnt that the test framework we use doesn't properly account for these scenarios - and as such a bunch of this testing needs to be done manually.

Unfortunately our testing framework has issues catching this - and as such (when we did this last time) we noted that this involved a bunch of manual testing to confirm this worked (and didn't introduce any deadlock), which took a substantial amount of time.

We should likely add configurations to the example folder that trigger the various scenarios so we can more quickly check for them, but unfortunately we haven't had the time to do so yet. From our side the team are currently on other tasks in preparation for the 2.0 milestone - which means that unfortunately we've had to prioritise that work over testing this. Whilst I can appreciate it's frustrating that this has been delayed/without feedback - we're working to get to this as soon as we can - in the interim thank you for your patience (and humour)

[Moeser]

Thank you for the detailed and insightful response @katbyte ! It's good to know the context around why this is a complicated change to review. I'll continue to wait patiently as you suggest :)

[tombuildsstuff]

@Moeser just to give an update here: after talking this through internally we're going to do some testing and then push this functionality out as the only feature in a bug release (v1.33.1) shortly following the next release (v1.33.0).

Doing it this way allows users to roll back to a previous version with the same functionality should this cause any dependency issues - which also means this isn't blocked until 2.0.

Apologies for the delay with these PR's - as @katbyte mentioned unfortunately there's a ton of manual testing here, but taking this approach should allow us the best of both world (confirming this works manually, and allowing users to roll back if there's any issues)

Thanks!

[Moeser]

@tombuildsstuff Sounds good to me, thanks for the update!

[katbyte]

Thanks for the @Moeser, LGTM 👍

[ghost]

This has been released in version 1.33.1 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.33.1"
}
# ... other configuration ...

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!