VM Disks are getting reordered on apply causing VM re-creation

[bpg]

Describe the bug If VM has multiple disks defined, applying the same template second time may cause VM recreation, as provider thinks that the disks have changed.

To Reproduce Steps to reproduce the behavior:

1. Define a VM template with multiple disks, for example

   disk {
   datastore_id = local.datastore_id
   file_id      = proxmox_virtual_environment_file.ubuntu_cloud_image.id
   interface    = "virtio0"
   iothread     = true
   }
   
   disk {
   datastore_id = local.datastore_id
   interface    = "scsi0"
   discard      = "on"
   ssd          = true
   }
   
   disk {
   datastore_id = "nfs"
   interface    = "scsi1"
   discard      = "ignore"
   file_format  = "raw"
   }

2. Apply the template with terraform apply
3. Without any changes to the template, run apply again
4. Notice that terraform detected changes in the template:

Expected behavior A second apply of the template without any changes should cause no-op from terraform.

Additional context Order of disks in PVE UI after appy:

This seems to be a reason for the mismatch.

Workaround Manually re-order disks in the template to match the order expected by PVE (alphabetically by interface name)

[github-actions[bot]]

Marking this issue as stale due to inactivity in the past 180 days. This helps us focus on the active issues. If this issue is reproducible with the latest version of the provider, please comment. If this issue receives no comments in the next 30 days it will automatically be closed. If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

[bpg]

Still reproducible (i think 🤔)

[luis-guimaraes-exoawk]

Any news regarding this? It's really annoying

[bpg]

I've spent quite a lot of time trying to debug this issue and applying the "consistent order" approach used in the "numa feature" referenced above. No luck so far, and the situation is really messy.

Terraform does not always return the list of disk elements from its state (i.e. on refresh) in a consistent order. Once in a while those items got re-ordered, and stop matching the order defined in the plan. The provider uses this order of items from the state when reading the disc info back from PVE to create a representation of the current remote state. Then Terraform matches the remote state with the planned state to check if there is any difference.

The reordering won't be a big problem, as we can "custom diff" and ignore the order. Except, there is a computed attribute path_in_datastore in the disks. If its value is missing in the plan (and the planned state), Terraform uses the one that was previously stored in state. But if the order changes (as above), the "previous" value becomes the one from a different disk 🤦🏼

resource "proxmox_virtual_environment_vm" "test_disk_order-185" {
  node_name = "pve"
  started   = false
  name      = "test-disk4"
  template  = "true"

  disk {
    file_format  = "raw"
    datastore_id = "local-lvm"
    interface    = "virtio0"
    size         = 8
  }
  disk {
    file_format  = "raw"
    datastore_id = "local-lvm"
    interface    = "scsi0"
    size         = 8
  }
}

TF_LOG=DEBUG terraform refresh

2024-04-15T11:57:39.689-0400 [WARN]  Provider "registry.terraform.io/bpg/proxmox" produced an unexpected new value for proxmox_virtual_environment_vm.test_disk_order-185 during refresh.
      - .disk[0].interface: was cty.StringVal("scsi0"), but now cty.StringVal("virtio0")
      - .disk[0].path_in_datastore: was cty.StringVal("vm-100-disk-0"), but now cty.StringVal("vm-100-disk-1")
      - .disk[1].path_in_datastore: was cty.StringVal("vm-100-disk-1"), but now cty.StringVal("vm-100-disk-0")
      - .disk[1].interface: was cty.StringVal("virtio0"), but now cty.StringVal("scsi0")

[bpg]

Ended up just excluding path_in_datastore from comparison in the diff suppress function

[luis-guimaraes-exoawk]

Thanks for the work on this issue. Just to give my two cents:

That seems like the best possible way to solve this right now. As even when we write the map alphabetically, sometimes the path_in_datastore value still changes and forces a replacement.

Does the path_in_datastore change due to Terraform or Proxmox? Edit 1: I reread the above message and saw that Terraform is the culprit Edit 2: Sorry if I'm stating something obvious you already tried. Is there no way to sort the Terraform state output before comparing it to the state from Proxmox? @bpg

[bpg]

Edit 2: Sorry if I'm stating something obvious you already tried. Is there no way to sort the Terraform state output before comparing it to the state from Proxmox?

It is more about about the planned state. The current code (before the fix) is actually sorting the disks before storing them into the local state. But planned state handling is completely on the Terraform's side, not the provider's. So it gets the current "remote" state from the provider (we read it from PVE and have full control on how disks are ordered there), then calculates the "planned" state using the plan and the stored state, and then compares them. That calculation of the planned state is fully on the Terraform side, and provider has no control over it (on purpose, i guess).

[bpg]

The issue is still reproducible, see more details in https://github.com/bpg/terraform-provider-proxmox/pull/1215#issuecomment-2103942810

[AtaLuZiK]

@bpg Maybe need ordered interfaces here? I checked the Go documentation and maps.Keys may return unordered results（https://pkg.go.dev/golang.org/x/exp/maps#Keys） disk.go#L628C4-L628C14

    if !isClone || len(currentDiskList) > 0 {
        var diskList []interface{}

        if len(currentDiskList) > 0 {
            resMap := utils.MapResourceList(currentDiskList, mkDiskInterface)
            interfaces := maps.Keys[map[string]interface{}](resMap)
            sort.Strings(interfaces)  // <-- I always get the right result by adding this line
            diskList = utils.OrderedListFromMapByKeyValues(diskMap, interfaces)
        } else {
            diskList = utils.OrderedListFromMap(diskMap)
        }

        err := d.Set(MkDisk, diskList)
        diags = append(diags, diag.FromErr(err)...)
    }

[bpg]

Sorting in that place breaks another use case when adding a new disk out of order of disk interfaces. The MapResourceList implementation has diverged from its original design, which introduced this sorting instability. The func shouldn't return a map, but rather a list of the key attribute values (interface values in this case) in the order of the original list.

UPDATE:

Sorting in that place breaks another use case when adding a new disk out of order of disk interfaces.

Well, it doesn't anymore 🤦🏼 That was addressed in SuppressIfListsOfMapsAreEqualIgnoringOrderByKey func. The last update to this func also changed behaviour MapResourceList and introduced this new bug, which only pops out if you have more than 3 disks in the VM 😞

[bpg]

@all-contributors please add @AtaLuZiK for testing, ideas

[allcontributors[bot]]

@bpg

I've put up a pull request to add @AtaLuZiK! :tada: