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

r/(linux|windows)_virtual_machine(_scale_set): support for Force Delete #11216

Merged
merged 17 commits into from
May 26, 2021

Conversation

tombuildsstuff
Copy link
Contributor

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

Copy link
Member

@mbfrahry mbfrahry left a 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)

@grayzu
Copy link
Collaborator

grayzu commented Apr 6, 2021

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.

@tombuildsstuff
Copy link
Contributor Author

@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.

@madewithsmiles
Copy link

@tombuildsstuff thanks for the quick turnaround. Please hold off merging this PR if possible.
I am part of the service team and I am raising a few issues from this PR internally. I/@grayzu will follow up here once things are clear. Thanks.

@grayzu
Copy link
Collaborator

grayzu commented Apr 8, 2021

@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:

  1. Whether or not to wait for the VM(SS) to shut down as part of the power off.
  2. Whether or not to wait for the VM(SS) to be powered off.
  3. Whether or not to use forceDelete when deleting the VM(SS).

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).

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.

LGTM 👍

@katbyte katbyte modified the milestones: v2.55.0, v2.56.0 Apr 9, 2021
@hashicorp hashicorp deleted a comment Apr 9, 2021
@tombuildsstuff
Copy link
Contributor Author

@grayzu

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).

That's what the latest commit(s) do, there's now 2 flags available:

provider "azurerm" {
  features {
    virtual_machine {
      force_delete = true
      shutdown_before_deletion = false
    }
  }
}

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)

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

@grayzu grayzu left a 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 {
Copy link
Collaborator

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

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.

@@ -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`.
Copy link
Collaborator

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."

---

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

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

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.

azurerm/internal/features/defaults.go Outdated Show resolved Hide resolved
@katbyte katbyte modified the milestones: v2.56.0, v2.57.0 Apr 16, 2021
@katbyte katbyte modified the milestones: v2.57.0, v2.58.0 Apr 30, 2021
@katbyte katbyte modified the milestones: v2.58.0, v2.59.0 May 7, 2021
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM!

@katbyte katbyte modified the milestones: v2.59.0, v2.60.0 May 14, 2021
@grayzu
Copy link
Collaborator

grayzu commented May 20, 2021

Thanks @katbyte! Your changes look good to me.

@katbyte katbyte modified the milestones: v2.60.0, v2.61.0 May 20, 2021
@katbyte katbyte merged commit 904a8d1 into master May 26, 2021
@katbyte katbyte deleted the f/vm-force-delete branch May 26, 2021 22:26
katbyte added a commit that referenced this pull request May 26, 2021
@ghost
Copy link

ghost commented May 27, 2021

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 ...

@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 Jun 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

r/(linux|windows)_virtual_machine: support for Force Delete
5 participants