-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
r/(linux|windows)_virtual_machine(_scale_set): support for Force Delete #11216
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tombuildsstuff! I tried running the tests but I'm not sure if these new attributes have been added properly.
=== CONT TestAccWindowsVirtualMachine_otherForceDelete
testing.go:620: Step 1/2 error: Error running pre-apply refresh:
Error: Unsupported argument
on config892966522/terraform_plugin_test.tf line 5, in provider "azurerm":
5: force_delete = true
An argument named "force_delete" is not expected here.
--- FAIL: TestAccWindowsVirtualMachine_otherForceDelete (6.86s)
=== CONT TestAccLinuxVirtualMachine_otherForceDelete
testing.go:620: Step 1/2 error: Error running pre-apply refresh:
Error: Unsupported argument
on config259639429/terraform_plugin_test.tf line 5, in provider "azurerm":
5: force_delete = true
An argument named "force_delete" is not expected here.
--- FAIL: TestAccLinuxVirtualMachine_otherForceDelete (6.67s)
Also @tombuildsstuff, can you adjust this so that it will skip the Power off all together when Force delete is enabled? If force delete is enabled, the intent is to simply have the VM be deleted without waiting for it to shutdown or power off. |
@grayzu chatting about this internally based on the Service Team's comments the other day there's benefits to having 2 flags here (one for force delete, one for skipping the shutdown) - since Force Delete will likely become the default option in 3.x, as (based on the Service Team's comments) there's reliability benefits for using Force Delete over regular Delete. For the moment that allows two feature flags to be present (one for force delete, one for skipping the shutdown) - which allows the Force Delete flag to ultimately be flipped on by default in 3.0 to leverage the reliability benefits they mentioned - whilst allowing both behaviours to exist. |
@tombuildsstuff thanks for the quick turnaround. Please hold off merging this PR if possible. |
@tombuildsstuff, I agree that having separate flags for shutdown and forceDelete makes sense. The ask here is to optimize the ForceDelete so that it aligns with the design intentions. To explain a bit further, there are three phases that can be configured as part of a VM(SS) delete:
The flag that we have now allows users to skip the shutdown as part of the power off process (no 1 above). With this flag set, the VM will still be powered off before it is deleted. The new flag enables forceDelete to be turned on as part of the VM(SS) delete process (no 3 above). The desire is to have steps 1 & 2 above skipped when forceDelete is turned on so the VM(SS) is not shutdown or powered off, it is just deleted. So in the end the ask is to have a flag for Shutdown that would allow customers to control wether or not to skip shutdown as part of the power off process and a flag for forceDelete that would short cut the process and go straight to delete the VM(SS). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
That's what the latest commit(s) do, there's now 2 flags available:
which will give you that behaviour (skipping shutdown & "force delete") - and allow the inverse (for a shutdown with force delete, to allow the reliability benefits of Force Delete, when that OS disk ultimately needs to be reused, for example via capturing an image of the virtual machine) |
…rtual_machine` block
…ult in 3.0 We can't do this now since the API errors if this preview functionality isn't enabled
…irtual Machine prior to deletion This is necessary since there's cost benefits to doing so with larger machines - whilst we could opt to do this automatically in the case of Force Delete based on the Service Teams description of this functionality, Force Delete should likely become the default in 3.x since it's more reliable for other items in the stack than a regular delete, however this can't be done yet until the feature leaves the preview period (as at the moment it's a public preview which errors if it's disabled) Ultimately this allows users who want to use Force Delete without shutting the VM can do so using both feature toggles together - whilst allowing the more resilient deletion (which will become the default at some point in the future) to be used with a gradual shut-down in the case of specific VM's where this can be useful.
e449d7f
to
424568c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I have added a couple of comments regarding force delete to ensure that we align with the intended behavior of the new feature.
} | ||
if err := powerOffFuture.WaitForCompletionRef(ctx, client.Client); err != nil { | ||
return fmt.Errorf("waiting for power off of Linux Virtual Machine %q (Resource Group %q): %+v", id.Name, id.ResourceGroup, err) | ||
if meta.(*clients.Client).Features.VirtualMachine.ShutdownBeforeDeletion { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no scenarios where it makes sense to power off a vm when Force Delete is enabled. This should be skipped if ForceDelete feature is true as well.
} | ||
if err := powerOffFuture.WaitForCompletionRef(ctx, client.Client); err != nil { | ||
return fmt.Errorf("waiting for power off of Windows Virtual Machine %q (Resource Group %q): %+v", id.Name, id.ResourceGroup, err) | ||
if meta.(*clients.Client).Features.VirtualMachine.ShutdownBeforeDeletion { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as linux_virtual_machine_resource regarding skipping this for forceDelete.
website/docs/index.html.markdown
Outdated
@@ -208,8 +208,16 @@ The `virtual_machine` block supports the following: | |||
|
|||
~> **Note:** When using a graceful shutdown, Azure gives the Virtual Machine a 5 minutes window in which to complete the shutdown process, at which point the machine will be force powered off - [more information can be found in this blog post](https://azure.microsoft.com/en-us/blog/linux-and-graceful-shutdowns-2/). | |||
|
|||
* `force_delete` - Should the `azurerm_linux_virtual_machine` and `azurerm_windows_virtual_machine` resources send a `Force Delete` flag when deleting the Virtual Machine? Force Delete force-detaches all devices from the Virtual Machine during deletion, rather than waiting for them to gracefully detach - which can be helpful during the deletion of other resources. Defaults to `false`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update to the following to accurate represent the force_delete option:
"When deleting an azurerm_linux_virtual_machine
or an azurerm_windows_virtual_machine
, Force Delete provides the ability to forcefully and immediately delete the VM, which will quickly free up sub-resources, such as NICs, ip addresses, disks, etc., associated with the virtual machine. This allows those freed resources to be reattached to another VM instance or deleted."
website/docs/index.html.markdown
Outdated
--- | ||
|
||
The `virtual_machine_scale_set` block supports the following: | ||
|
||
* `force_delete` - Should the `azurerm_linux_virtual_machine_scale_set` and `azurerm_windows_virtual_machine_scale_set` resources send a `Force Delete` flag when deleting the Virtual Machine Scale Set? Force Delete force-detaches all devices from the Virtual Machine Scale Set during deletion, rather than waiting for them to gracefully detach - which can be helpful during the deletion of other resources. Defaults to `false`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update to the following to accurate represent the force_delete option
"When deleting an azurerm_linux_virtual_machine_scale_set
or an azurerm_windows_virtual_machine_scale_set
, Force Delete provides the ability to forcefully and immediately delete the VM, which will quickly free up sub-resources, such as NICs, ip addresses, disks, etc., associated with the virtual machine. This allows those freed resources to be reattached to another VM instance or deleted."
var forceDeletion *bool = nil | ||
if meta.(*clients.Client).Features.VirtualMachineScaleSet.ForceDelete { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as linux_virtual_machine_resource regarding skipping this for forceDelete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks @katbyte! Your changes look good to me. |
This has been released in version 2.61.0 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 = "~> 2.61.0"
}
# ... other configuration ... |
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. |
This PR introduces support for the opt-in Force Delete behaviour for the following resources:
azurerm_linux_virtual_machine
azurerm_linux_virtual_machine_scale_set
azurerm_windows_virtual_machine
azurerm_windows_virtual_machine_scale_set
This functionality is an optional behaviour during the deletion of a Virtual Machine where the machine will be hard powered-off and all devices forcibly removed from the Virtual Machine - which can both improve the shut-down time and work around issues within the Azure API where resources can remain attached to a Virtual Machine during deletion.
Whilst generally speaking that'd make sense to be the default behaviour - unfortunately the API makes this an opt-in behaviour at this point in time and errors if this preview functionality isn't enabled, so this will have to be defaulted off for the moment.
Fixes #11089