helper/signalwrapper and azurerm_storage_account listens for signals #8215
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Confirmed a dangling resource. 😵 I'm going to look into this tomorrow but definitely do not merge. Please feel free to review my direction though. |
| }) | ||
|
|
||
| // Check the result of the wrapped function. I put this into a select | ||
| // since we will likely also want to introduce a time-based timeout. |
There was a problem hiding this comment.
Second sentence no longer accurate, timeout in place.
|
This LGTM! The use of cancellation channel will be able to be rolled out to all of the AzureRM resources - that was introduced in their SDK 2.1-beta |
|
Addressed all feedback. |
|
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
azurerm_storage_accountcreation can take a long time (24+ hours). If you Ctrl-C, Terraform currently waits for this to complete before cancelling but for this particular resource it is too long.This PR introduces a new package
helper/signalwrapperfor easily wrapping functions that can cancel based on signals and early exiting. This package is used with theazurerm_storage_accountresource to implement cancellability.I tested this and it definitely responds to Ctrl-C now. I'm however extremely skeptical in the Azure SDK that it is doing anything other than simply exiting. We don't receive an ID in the cancelled case and it cancels immediately. I'm really scared that we're actually just dangling a resource this way but I haven't gotten the storage account to show up yet. I just can't believe this is actually working, so I'm going to test it more. I'd love your thoughts @stack72 on the Azure stuff. Besides this, 95+% of the LoC in this PR are fine for review and well tested.
So please review but allow me to merge once I verify more behavior.
WARNING: This is not a long term solution. There are very subtle races here: if you Ctrl-C before it reaches the code to create the storage account you could end up waiting anyways. And its also just unclean that provider code is listening for OS signals. In the future, I'd like to introduce a Stop or Cancel API to the
ResourceProviderinterface that can bubble a stop request through, therefore avoiding the race conditions and also abstracting providers away from "signals" to an API.