VM Disks are getting reordered on apply causing VM re-creation about terraform-provider-proxmox HOT 12 CLOSED

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

from terraform-provider-proxmox.

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

from terraform-provider-proxmox.

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

from terraform-provider-proxmox.

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!

from terraform-provider-proxmox.

Still reproducible (i think 🤔)

from terraform-provider-proxmox.

Any news regarding this? It's really annoying

from terraform-provider-proxmox.

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

from terraform-provider-proxmox.

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

from terraform-provider-proxmox.

The issue is still reproducible, see more details in #1215 (comment)

from terraform-provider-proxmox.

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 😞

from terraform-provider-proxmox.

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

from terraform-provider-proxmox.

@bpg

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

from terraform-provider-proxmox.