New Resource: `azurerm_virtual_machine_implicit_data_disk_from_source` #25537

ms-zhenhua · 2024-04-09T01:50:39Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

Note: ideally, this feature should be supported by adding data_disks in azurerm_[linux|windows]_virtual_machine. However, due to technical limitations, a new resource is defined to support this feature.

This new resource is used to support a new virtual machine feature named implicit disk creation. Being different from the existing resource azurerm_virtual_machine_data_disk_attachment, which attaches an existing managed disk to the VM, this new resource creates a new data disk when updating a VM and allows the system to place the this disk closer to the parent VM to provide better latency and align the fault domain of the disk with the parent VM for providing better reliability.

This resource is similar with azurerm_virtual_machine_data_disk_attachment so that they share the same ID format. However, since this new resource uses some unique new properties, it is defined as a separate resource. These two resources are distinguished by different values of the create_option, which are added into the import code.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_virtual_machine_implicit_data_disk_from_source - a new resource to support implicit disk creation feature for virtual machines

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #0000

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

tombuildsstuff

hey @ms-zhenhua

I've taken a look through and left some comments inline - but as it stands I suspect the name of this Resource is going to be a source of confusion in its current form.

Since we're creating a Data Disk from a Snapshot and then attaching that - perhaps a better name for this would be azurerm_virtual_machine_implicit_data_disk_from_source?

Thanks!

tombuildsstuff · 2024-04-09T07:22:02Z

internal/services/compute/virtual_machine_implicit_data_disk_resource.go

+						// delete data disk if delete_option is set to delete
+						if toBeDeletedDisk != nil && pointer.From(toBeDeletedDisk.DeleteOption) == virtualmachines.DiskDeleteOptionTypesDelete &&
+							toBeDeletedDisk.ManagedDisk != nil && toBeDeletedDisk.ManagedDisk.Id != nil {
+							diskClient := metadata.Client.Compute.DisksClient
+							diskId, err := commonids.ParseManagedDiskID(*toBeDeletedDisk.ManagedDisk.Id)
+							if err != nil {
+								return err
+							}
+
+							err = diskClient.DeleteThenPoll(ctx, *diskId)
+							if err != nil {
+								return fmt.Errorf("deleting Managed Disk %s: %+v", *diskId, err)
+							}
+						}


This is an API setting - why are we deleting the disk here?

Because this disk is created by the backend so that it's not managed by any TF resource such as azurerm_managed_disk. If we do not delete it explicitly here, it will only be removed from the VM configuration, but the disk itself still exists. When VM is deleted, this disk will not be deleted together because it's already been removed from the VM configuration. Since the disk still exists in the resource group, the resource group cannot be deleted unless prevent_deletion_if_contains_resources=false. Due to this reason, I choose to explicitly delete the disk when DeleteOption=true.
Could you kindly help confirm whether it's feasible? If not, I can remove the delete_option and use prevent_deletion_if_contains_resources=false to delete this resource.

Hmmm @ms-zhenhua, what're your thoughts on deleting the disk when it's unhooked from the vm and not worrying about delete_option? This resource means that Terraform is creating the disk and I would expect Terraform to also delete the disk

Are there use cases when we want to keep the disk around?

Hi @mbfrahry, when we create this resource, what we really do is to add a data disk configuration to the existing VM, which will trigger the Azure backend to create an implicit data disk for the VM. However, when we delete this resource, what we really do is just remove the data disk configuration from the VM, but the physical data disk is only detached from the VM and still exists in the resource group. That's why I use the delete option to manually delete this data disk, otherwise, the resource group cannot be deleted normally because the data disk still exists in the resource group.

tombuildsstuff · 2024-04-09T07:22:48Z

internal/services/compute/virtual_machine_implicit_data_disk_resource.go

+			if model := resp.Model; model != nil {
+				if props := model.Properties; props != nil {
+					if profile := props.StorageProfile; profile != nil {
+						if dataDisks := profile.DataDisks; dataDisks != nil {


there's ways to provision a VM without a StorageProfile and/or DataDisks (e.g. when creating from certain types of image) - as such we'd end up doing nothing in those cases here - we'd need to update this to account for that

code updated. Thanks.

tombuildsstuff · 2024-04-09T07:23:07Z

internal/services/compute/virtual_machine_implicit_data_disk_resource.go

+			Type:     pluginsdk.TypeString,
+			Required: true,
+			ValidateFunc: validation.StringInSlice([]string{
+				string(virtualmachines.CachingTypesNone),


See the None topic in the Contributor Guide

code updated. Thanks.

tombuildsstuff · 2024-04-09T07:34:05Z

internal/services/compute/virtual_machine_implicit_data_disk_resource.go

+	_ sdk.ResourceWithCustomImporter = VirtualMachineImplicitDataDiskResource{}
+)
+
+type VirtualMachineImplicitDataDiskResource struct{}


Per the PR description:

this new resource creates a new data disk when updating a VM and allows the system to place the this disk closer to the parent VM to provide better latency and align the fault domain of the disk with the parent VM for providing better reliability.

Can you provide a source showing the performance benefits of this? Last I read using a Zone/Proximity Placement Group should give the same benefit here - and from what I understood was replacing the Fault Domains concept (which is associated with Availability Sets, which I believe aren't recommended anymore?)?

Based on this issue (Azure/azure-cli#28686) - since this exists only for copies of a Disk from a Snapshot, rather than a net-new Disk - I'm concerned that the resource name is going to be misleading too?

Confirmed with the service team, Proximity Placement Group can reduce distances between VMs, however, it does not include the data disk resources.
Availability Sets are being replaced by VMSS Flex, which will support storage aligned FDs in a single zone.
This feature is mainly used for optimizing the operations that need to happen when attaching a disk to a VM in a multi-FD deployment. By using implicit disk creation, system will create a new disk and place the disk in the right Fault Domain, which can provide a better latency.

ms-zhenhua · 2024-04-17T03:35:27Z

Hi @tombuildsstuff, thank you for the review. I have updated the code and comments. Could you kindly have another review?

…r-azurerm into upgrade-lighthouse-version

mbfrahry

Hey @ms-zhenhua, I've left some more comments, I'm still hung up on the Delete method and would like some further discussion on that piece

mbfrahry · 2024-05-20T23:32:35Z

internal/services/compute/virtual_machine_implicit_data_disk_from_source.go

@@ -0,0 +1,476 @@
+package compute


This filename wants to be virtual_machine_implicit_data_disk_from_source_resource.go

updated. Thanks.

mbfrahry · 2024-05-20T23:32:56Z

internal/services/compute/virtual_machine_implicit_data_disk_from_source_test.go

@@ -0,0 +1,992 @@
+package compute_test


And this filename wants to be virtual_machine_implicit_data_disk_from_source_resource_test.go

updated. Thanks.

mbfrahry · 2024-05-20T23:44:31Z

internal/services/compute/virtual_machine_data_disk_attachment_resource.go

+				return nil, fmt.Errorf("Data Disk %s was not found", *id)
+			}
+
+			if disk.CreateOption != virtualmachines.DiskCreateOptionTypesAttach && disk.CreateOption != virtualmachines.DiskCreateOptionTypesEmpty {


Is it possible to make a test for this?

Yes, added a new testcase:

=== RUN TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk
=== PAUSE TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk
=== CONT TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk
--- PASS: TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk (594.13s)
PASS

mbfrahry · 2024-05-20T23:46:38Z

internal/services/compute/virtual_machine_data_disk_attachment_resource.go

@@ -154,6 +197,15 @@ func resourceVirtualMachineDataDiskAttachmentCreateUpdate(d *pluginsdk.ResourceD
 		WriteAcceleratorEnabled: pointer.To(writeAcceleratorEnabled),
 	}

+	// there are ways to provision a VM without a StorageProfile and/or DataDisks


This should also include a test to confirm that the existing VM won't have any diffs when a disk is attached this way

I have re-run all existing testcases which are passed and attached in the description.

Though this comment said there are ways to provision a VM without a StorageProfile and/or DataDisks with certain types of images, I do not really know which image will trigger such scenario. Kindly let me know if you know the image type and I will add the testcase. However, this part of code will not cause any diff because this code only helps initialize the storage and disk when they are nil, which is a protection for the existing code.

mbfrahry · 2024-05-20T23:59:38Z

internal/services/compute/virtual_machine_implicit_data_disk_from_source.go

+						}
+
+						// delete data disk if delete_option is set to delete
+						if toBeDeletedDisk != nil && pointer.From(toBeDeletedDisk.DeleteOption) == virtualmachines.DiskDeleteOptionTypesDelete &&


Because Terraform is creating this resource, I believe we can make the assumption that Terraform can delete this resource? If there is value in keeping the disk around, we can add a new variable to the features block that will hang on to the disk instead of deleting it. Or vice versa if the default should be to keep the disk around instead

This resource is a little special. It manages the data disk of a VM such as azurerm_virtual_machine_data_disk_attachment. When creating this resource, what is really done is to add a data disk configuration to the existing VM, which will trigger the Azure service to create a new data disk for the VM. When deleting this resource, what is really done is to remove the data disk configuration from the existing VM. However, the physical data disk will not be removed from the resource group, which may cause the resource group cannot be deleted later. That's why I add this ugly delete option. According to your comment, do you mean we can hang on to the disk by default and should add a new variable to the provider features block to control whether to delete the physical data disk when deleting this resource?

This is tough because from the users perspective, Terraform is creating a disk (I do see that Azure is creating the disk from what we are sending to the api but the user will see that it is Terraform that is creating the disk) so running destroy should delete the disk that was created.

My thought is to have the default be to delete this disk and then in a features block, we add an option to detach the disk instead of deleting it.

That does make sense. I have removed the delete_option and added a variable in the feature block.

=== RUN TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk
=== PAUSE TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk
=== CONT TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk
--- PASS: TestAccVirtualMachineDataDiskAttachment_importImplicitDataDisk (447.13s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/compute 498.662s

--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_destroy (395.01s)
--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_basic (414.26s)
--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_requiresImport (418.42s)
--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_managedServiceIdentity (423.98s)
--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_multipleDisks (472.35s)
--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_virtualMachineExtension (499.92s)
--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_updatingWriteAccelerator (509.00s)
--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_updatingCaching (511.20s)
--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_virtualMachineApplication (1033.91s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/compute 1045.466s

=== RUN TestAccVirtualMachineImplicitDataDiskFromSource_detachImplicitDataDisk
=== PAUSE TestAccVirtualMachineImplicitDataDiskFromSource_detachImplicitDataDisk
=== CONT TestAccVirtualMachineImplicitDataDiskFromSource_detachImplicitDataDisk
--- PASS: TestAccVirtualMachineImplicitDataDiskFromSource_detachImplicitDataDisk (462.70s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/compute 510.088s

ms-zhenhua · 2024-05-21T07:38:25Z

Hi @mbfrahry , thank you for your review. I have updated the code and replied your comments. Please help take another review and leave your questions and comments. Thanks.

mbfrahry

Hey @ms-zhenhua I've responded and really do think that we should add a flag in the features block to keep the disk rather than doing the DeleteOption you've written out

… block

…aDisk

ms-zhenhua · 2024-05-22T09:03:18Z

Hi @mbfrahry, thank you for your advice. I removed DeleteOption and made Delete as the default operation. I added a variable in the features block to control whether to detach the data disk when destroying the resource. Please take another review. Thanks.

website/docs/guides/features-block.html.markdown

mbfrahry

LGTM! Thanks for working through the reviews @ms-zhenhua !

github-actions · 2024-07-01T02:13:20Z

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.

add code

b12d940

github-actions bot added documentation size/XL labels Apr 9, 2024

tombuildsstuff suggested changes Apr 9, 2024

View reviewed changes

github-actions bot added the waiting-response label Apr 9, 2024

ms-zhenhua marked this pull request as draft April 15, 2024 05:45

resolve comments

bf34f34

github-actions bot added waiting-response and removed waiting-response labels Apr 16, 2024

fix website lint

d6debd5

github-actions bot removed the waiting-response label Apr 16, 2024

fix acc test

42b931f

ms-zhenhua marked this pull request as ready for review April 17, 2024 02:57

ms-zhenhua requested a review from tombuildsstuff April 17, 2024 03:35

ms-zhenhua added 2 commits April 25, 2024 13:21

Merge branch 'main' of https://github.com/hashicorp/terraform-provide…

a751e45

…r-azurerm into upgrade-lighthouse-version

add code

1f5778f

ms-zhenhua changed the title ~~New Resource: azurerm_virtual_machine_implicit_data_disk~~ New Resource: azurerm_virtual_machine_implicit_data_disk_from_source Apr 25, 2024

katbyte assigned tombuildsstuff May 7, 2024

github-actions bot added the waiting-response label May 20, 2024

mbfrahry requested changes May 21, 2024

View reviewed changes

resolve comments

a8d1298

github-actions bot removed the waiting-response label May 21, 2024

add test case

19fa7f2

ms-zhenhua requested a review from mbfrahry May 21, 2024 07:40

katbyte assigned mbfrahry May 21, 2024

github-actions bot added the waiting-response label May 21, 2024

mbfrahry requested changes May 21, 2024

View reviewed changes

add virtual_machine.detach_implicit_data_disk_on_deletion in features…

49bd69a

… block

github-actions bot added waiting-response and removed waiting-response labels May 22, 2024

fix website lint

e219132

github-actions bot removed the waiting-response label May 22, 2024

add TestAccVirtualMachineImplicitDataDiskFromSource_detachImplicitDat…

4438fbf

…aDisk

ms-zhenhua requested a review from mbfrahry May 22, 2024 09:03

mbfrahry reviewed May 28, 2024

View reviewed changes

website/docs/guides/features-block.html.markdown Outdated Show resolved Hide resolved

Update website/docs/guides/features-block.html.markdown

e05ce4b

github-actions bot added waiting-response and removed waiting-response labels May 28, 2024

mbfrahry approved these changes May 28, 2024

View reviewed changes

mbfrahry merged commit 3ec5554 into hashicorp:main May 29, 2024
33 checks passed

mbfrahry added a commit that referenced this pull request May 29, 2024

Update CHANGELOG.md for #25537

1f5be69

github-actions bot added this to the v3.106.0 milestone May 29, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jul 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_virtual_machine_implicit_data_disk_from_source` #25537

New Resource: `azurerm_virtual_machine_implicit_data_disk_from_source` #25537

ms-zhenhua commented Apr 9, 2024 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff Apr 9, 2024

ms-zhenhua Apr 9, 2024 •

edited

Loading

mbfrahry May 20, 2024

ms-zhenhua May 21, 2024 •

edited

Loading

tombuildsstuff Apr 9, 2024

ms-zhenhua Apr 16, 2024

tombuildsstuff Apr 9, 2024

ms-zhenhua Apr 16, 2024

tombuildsstuff Apr 9, 2024

ms-zhenhua Apr 17, 2024

ms-zhenhua commented Apr 17, 2024

mbfrahry left a comment

mbfrahry May 20, 2024

ms-zhenhua May 21, 2024

mbfrahry May 20, 2024

ms-zhenhua May 21, 2024

mbfrahry May 20, 2024

ms-zhenhua May 21, 2024

mbfrahry May 20, 2024

ms-zhenhua May 21, 2024 •

edited

Loading

mbfrahry May 20, 2024

ms-zhenhua May 21, 2024

mbfrahry May 21, 2024

ms-zhenhua May 22, 2024

ms-zhenhua commented May 21, 2024

mbfrahry left a comment

ms-zhenhua commented May 22, 2024

mbfrahry left a comment

github-actions bot commented Jul 1, 2024

New Resource: azurerm_virtual_machine_implicit_data_disk_from_source #25537

New Resource: azurerm_virtual_machine_implicit_data_disk_from_source #25537

Conversation

ms-zhenhua commented Apr 9, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-zhenhua Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-zhenhua May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-zhenhua commented Apr 17, 2024

mbfrahry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-zhenhua May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-zhenhua commented May 21, 2024

mbfrahry left a comment

Choose a reason for hiding this comment

ms-zhenhua commented May 22, 2024

mbfrahry left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 1, 2024

New Resource: `azurerm_virtual_machine_implicit_data_disk_from_source` #25537

New Resource: `azurerm_virtual_machine_implicit_data_disk_from_source` #25537

ms-zhenhua commented Apr 9, 2024 •

edited

Loading

ms-zhenhua Apr 9, 2024 •

edited

Loading

ms-zhenhua May 21, 2024 •

edited

Loading

ms-zhenhua May 21, 2024 •

edited

Loading