Create data disks and attach after VM creation by azurerm_virtual_machine_data_disk_attachment resource.

[DatsloJRel]

Describe your changes

Create data disks and attach after VM creation by azurerm_virtual_machine_data_disk_attachment resource.
This allows OS disks to be destroyed and recreated while leaving the data disks.

Checklist before requesting a review

• The pr title can be used to describe what this pr did in CHANGELOG.md file
• I have executed pre-commit on my machine
• I have passed pr-check on my machine

Thanks for your cooperation!

[lonegunmanb]

Hi @DatsloJRel , thanks for opening this pr, as azurerm_vitual_machine's document described:

Data Disks can also be attached either using this block or the azurerm_virtual_machine_data_disk_attachment resource - but not both.

Now our virtual machine has storage_data_disk block defined already, is there any special reason we need additional azurerm_virtual_machine_data_disk_attachment resource? Thanks!

[DatsloJRel]

@lonegunmanb , Thanks for looking at my PR!

Now our virtual machine has storage_data_disk block defined already, is there any special reason we need additional azurerm_virtual_machine_data_disk_attachment resource? Thanks!

My use case for this functionality is for redeployment of the VM with a newer OS disk (with latest OS patches, etc) while keeping the local data disk(s) deployed and untouched. I originally attempted this behavior by modifying the vm_os_version variable from one specific version of the OS to another and then re-running terraform apply, but it seems there is a bug such that the change in vm_os_version isn't recognized by the azurerm resources. Therefore, I have resorted to changing the vm_os_sku between gen1 and gen2 for the OS, and then running terraform apply triggers the VM and OS disk to be destroyed and recreated again with the latest OS version (with vm_os_version = "latest"). The script defined in startup_script runs, mounts the data disk(s), and reinstalls the application software, all while the data remains in place and untouched. This process allows minimal downtime for the application on a fresh new OS without having to run patching / upgrade on the production system.

Through my research and testing, I was not able to perform the same behavior through the storage_data_disk data disks. However, I am a novice with terraform, so there may be something I missed. Also, I originally implemented this about a year ago, so similar behavior might be found a different way with later updates.

[lonegunmanb]

Thanks @DatsloJRel for your explanation, I got your point. You're right, the status quo cannot support your requirement: recreating virtual machine with newer os image meanwhile keeping the data disks, but we cannot add these azurerm_virtual_machine_data_disk_attachment resources neither since it could cause unpredictable behaviors.

I would recommend a new, modern virtual machine module -- terraform-azurerm-virtual-machine, which I believe can support your scenario.

[DatsloJRel]

@lonegunmanb I can appreciate not wanting to add resources that can cause unpredictable behaviors. I added the extra text in the variables' description to help limit that behavior, but I can see more should be done. I would very much like to see this functionality added so that I don't have to maintain my separate fork, so I wonder if we can compromise?

What if I added code such that it would prevent the creation of both and raise an error instead. Terraform v1.2.0 added the precondition ability, so I could add that precondition check for the azurerm_virtual_machine_data_disk_attachment resource, as well as the azurerm_virtual_machine resource. The precondition check would be similar to the what is shown below. I will go ahead with updating the PR if you agree this this design:

Disk Attachment Resource:

resource "azurerm_virtual_machine_data_disk_attachment" "vm_data_disk_attachments_linux" {
  for_each           = local.data_disk_map_linux
  managed_disk_id    = azurerm_managed_disk.vm_data_disk[each.key].id
  virtual_machine_id = azurerm_virtual_machine.vm_linux[each.value.host_number].id
  lun                = each.value.disk_number
  caching            = "ReadWrite"
  lifecycle {
    precondition {
      condition     = var.nb_data_disk == 0 && length(var.extra_disks) == 0
      error_message = "`nb_data_disk_by_data_disk_attachment` can not be used with `nb_data_disks` or `extra_disks`!"
    }
  }
}

VM Resource:

resource "azurerm_virtual_machine" "vm_linux" {
  count = !local.is_windows ? var.nb_instances : 0

  <...>

  lifecycle {
    precondition {
      condition     = !var.is_marketplace_image || (var.vm_os_offer != null && var.vm_os_publisher != null && var.vm_os_sku != null)
      error_message = "`var.vm_os_offer`, `vm_os_publisher` and `var.vm_os_sku` are required when `var.is_marketplace_image` is `true`."
    }

    precondition {
      condition     = (var.nb_data_disk == 0 && length(var.extra_disks) == 0) || (length(local.data_disk_map_linux) == 0 && length(local.extra_disk_map_linux) == 0)
      error_message = "`nb_data_disk` and `extra_disks` can not be used with `nb_data_disk_by_data_disk_attachment` and `extra_disks_by_data_disk_attachment`"
    }
  }
}

[lonegunmanb]

Hi @DatsloJRel, I think we can make that happen, as you can see the current implementation's problem is as nested blocks, the data disk must be destroyed once we have to rebuild the vm. The solution is using standalone managed disk resources and attachment resources instead of nested blocks.

We can do that by adding a new toggle, let's say var.nested_data_disks as a boolean type variable, if it's true (it's default value should be true), we generate storage_data_disk dynamic blocks as before; if not, we generate var.nb_data_disk and var.extra_disks by using managed disk and attachment resources. Does that work for you?

[lonegunmanb]

Thanks @DatsloJRel for the update, almost LGTM and we finally get our e2e test runners work, but we have conflict in readme file. Would you please merge your branch with the latest master branch so we can merge this pr? Thanks!

[DatsloJRel]

@lonegunmanb, I updated to use the nested_data_disks variable as you suggested. This is a much better design than what I had originally done, so thanks for the suggestion!

Also, I am very interested to see how the modern virtual machine module -- terraform-azurerm-virtual-machine progresses. It looks like it will be a great option in the near future. Congratulations on it's initial release! I will most likely transition to that module once it is production ready.

Thanks again for your help!

[lonegunmanb]

Thanks @DatsloJRel, LGTM! 🚀