fix(vm): state drift due to disk re-ordering (#1215)

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>

.vscode/extensions.json

@@ -1,8 +1,11 @@
 {
     "recommendations": [
+        "britesnow.vscode-toggle-quotes",
         "davidanson.vscode-markdownlint",
+        "EditorConfig.editorconfig",
         "golang.go",
         "hashicorp.terraform",
         "joshbolduc.commitlint",
+        "PKief.material-icon-theme",
     ]
 }

.vscode/settings.json

@@ -95,4 +95,19 @@
     ],
     "go.lintOnSave": "workspace",
     "go.testEnvFile": "${workspaceFolder}/testacc.env",
+    "material-icon-theme.folders.associations": {
+        ".github/workflows": "robot",
+        "fwprovider": "middleware",
+        "proxmox": "api",
+        "proxmoxtf": "container",
+        "structure": "utils",
+        "access": "admin",
+        "firewall": "secure",
+        "nodes": "server",
+        "storage": "database",
+        "pools": "batch",
+        "ssh": "connection",
+        "version": "include",
+        "datasource": "database",
+    },
 }

fwprovider/tests/resource_vm_test.go

@@ -573,6 +573,42 @@ func TestAccResourceVMDisks(t *testing.T) {
 				RefreshState: true,
 			},
 		}},
+		{"multiple disks", []resource.TestStep{
+			{
+				Config: te.renderConfig(`
+				resource "proxmox_virtual_environment_vm" "test_disk4" {
+					node_name = "{{.NodeName}}"
+					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
+					}
+				}`),
+				Check: testResourceAttributes("proxmox_virtual_environment_vm.test_disk4", map[string]string{
+					// I'd love to test this, but the order of the list items is not guaranteed, apparently
+					//     resource_vm_test.go:669: Step 1/2 error: Check failed: proxmox_virtual_environment_vm.test_disk4: Attribute "disk.1.interface" value: expected 'virtio0' to match 'scsi0'
+					//    --- FAIL: TestAccResourceVMDisks/multiple_disks (5.24s)
+					// "disk.1.interface":         "scsi0",
+					// "disk.1.path_in_datastore": `vm-\d+-disk-0`,
+					// "disk.0.interface":         "virtio0",
+					// "disk.0.path_in_datastore": `vm-\d+-disk-1`,
+				}),
+			},
+			{
+				RefreshState: true,
+			},
+		}},
 		{"clone disk with overrides", []resource.TestStep{
 			{
 				SkipFunc: func() (bool, error) {
@@ -610,18 +646,16 @@ func TestAccResourceVMDisks(t *testing.T) {
 						//size = 10
 					}
 				}`),
-				Check: resource.ComposeTestCheckFunc(
+				Check: testResourceAttributes("proxmox_virtual_environment_vm.test_disk3", map[string]string{
-					testResourceAttributes("proxmox_virtual_environment_vm.test_disk3", map[string]string{
+					"disk.0.datastore_id":      "local-lvm",
-						"disk.0.datastore_id":      "local-lvm",
+					"disk.0.discard":           "on",
-						"disk.0.discard":           "on",
+					"disk.0.file_format":       "raw",
-						"disk.0.file_format":       "raw",
+					"disk.0.interface":         "scsi0",
-						"disk.0.interface":         "scsi0",
+					"disk.0.iothread":          "true",
-						"disk.0.iothread":          "true",
+					"disk.0.path_in_datastore": `vm-\d+-disk-\d+`,
-						"disk.0.path_in_datastore": `vm-\d+-disk-\d+`,
+					"disk.0.size":              "8",
-						"disk.0.size":              "8",
+					"disk.0.ssd":               "true",
-						"disk.0.ssd":               "true",
+				}),
-					}),
-				),
 			},
 			{
 				RefreshState: true,

proxmox/nodes/storage/download_url.go

@@ -34,7 +34,7 @@ func (c *Client) DownloadFileByURL(
 	taskErr := c.Tasks().WaitForTask(ctx, *resBody.TaskID, int(uploadTimeout), 5)
 	if taskErr != nil {
 		err = fmt.Errorf(
-			"error download file to datastore %s: failed waiting for url download - %w",
+			"error download file to datastore %s: failed waiting for url download: %w",
 			c.StorageName,
 			taskErr,
 		)

proxmoxtf/resource/vm/disk/disk.go

@@ -12,6 +12,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-log/tflog"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"golang.org/x/exp/maps"
 
 	"github.com/bpg/terraform-provider-proxmox/proxmox"
 	"github.com/bpg/terraform-provider-proxmox/proxmox/nodes/vms"
@@ -24,18 +25,6 @@ import (
 
 // GetInfo returns the disk information for a VM.
 func GetInfo(resp *vms.GetResponseData, d *schema.ResourceData) vms.CustomStorageDevices {
-	currentDisk := d.Get(MkDisk)
-
-	currentDiskList := currentDisk.([]interface{})
-	currentDiskMap := map[string]map[string]interface{}{}
-
-	for _, v := range currentDiskList {
-		diskMap := v.(map[string]interface{})
-		diskInterface := diskMap[mkDiskInterface].(string)
-
-		currentDiskMap[diskInterface] = diskMap
-	}
-
 	storageDevices := vms.CustomStorageDevices{}
 
 	storageDevices["ide0"] = resp.IDEDevice0
@@ -82,11 +71,14 @@ func GetInfo(resp *vms.GetResponseData, d *schema.ResourceData) vms.CustomStorag
 	storageDevices["virtio14"] = resp.VirtualIODevice14
 	storageDevices["virtio15"] = resp.VirtualIODevice15
 
+	currentDisk := d.Get(MkDisk)
+
+	diskMap := utils.MapResourceList(currentDisk.([]interface{}), mkDiskInterface)
+
 	for k, v := range storageDevices {
-		if v != nil {
+		if v != nil && diskMap[k] != nil {
-			if currentDiskMap[k] != nil {
+			if d, ok := diskMap[k].(map[string]interface{}); ok {
-				if currentDiskMap[k][mkDiskFileID] != nil {
+				if fileID, ok := d[mkDiskFileID].(string); ok && fileID != "" {
-					fileID := currentDiskMap[k][mkDiskFileID].(string)
 					v.FileID = &fileID
 				}
 			}
@@ -749,8 +741,17 @@ func Read(
 	}
 
 	if !isClone || len(currentDiskList) > 0 {
-		orderedDiskList := utils.OrderedListFromMap(diskMap)
+		var diskList []interface{}
-		err := d.Set(MkDisk, orderedDiskList)
+
+		if len(currentDiskList) > 0 {
+			resMap := utils.MapResourceList(currentDiskList, mkDiskInterface)
+			interfaces := maps.Keys[map[string]interface{}](resMap)
+			diskList = utils.OrderedListFromMapByKeyValues(diskMap, interfaces)
+		} else {
+			diskList = utils.OrderedListFromMap(diskMap)
+		}
+
+		err := d.Set(MkDisk, diskList)
 		diags = append(diags, diag.FromErr(err)...)
 	}
 

proxmoxtf/resource/vm/disk/schema.go

@@ -5,6 +5,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 
 	"github.com/bpg/terraform-provider-proxmox/proxmoxtf/resource/validators"
+	"github.com/bpg/terraform-provider-proxmox/proxmoxtf/structure"
 )
 
 const (
@@ -71,6 +72,10 @@ func Schema() map[string]*schema.Schema {
 					},
 				}, nil
 			},
+			DiffSuppressFunc: structure.SuppressIfListsOfMapsAreEqualIgnoringOrderByKey(
+				mkDiskInterface, mkDiskPathInDatastore,
+			),
+			DiffSuppressOnRefresh: true,
 			Elem: &schema.Resource{
 				Schema: map[string]*schema.Schema{
 					mkDiskInterface: {

proxmoxtf/resource/vm/vm.go

@@ -3723,7 +3723,7 @@ func vmReadCustom(
 		diags = append(diags, diag.FromErr(err)...)
 	}
 
-	allDiskInfo := disk.GetInfo(vmConfig, d) // from the cloned VM
+	allDiskInfo := disk.GetInfo(vmConfig, d)
 
 	diags = append(diags, disk.Read(ctx, d, allDiskInfo, vmID, api, nodeName, len(clone) > 0)...)
 

proxmoxtf/structure/schema.go

@@ -12,8 +12,9 @@ import (
 	"sort"
 	"strings"
 
-	"github.com/bpg/terraform-provider-proxmox/utils"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+
+	"github.com/bpg/terraform-provider-proxmox/utils"
 )
 
 // MergeSchema merges the map[string]*schema.Schema from src into dst.
@@ -127,9 +128,15 @@ func SuppressIfListsAreEqualIgnoringOrder(key, _, _ string, d *schema.ResourceDa
 // elements.
 // It will be called for each resource attribute, so it is not super efficient. It is
 // recommended to use it only for small lists / small resources.
-// Note: The order of the attributes within the resource is still important.
+// The keyAttr is the attribute that will be used as the key to compare the resources.
+// The ignoreKeys are the keys that will be ignored when comparing the resources. Include computed read-only
+// attributes here.
+//
 // Ref: https://github.com/hashicorp/terraform-plugin-sdk/issues/477
-func SuppressIfListsOfMapsAreEqualIgnoringOrderByKey(keyAttr string) schema.SchemaDiffSuppressFunc {
+func SuppressIfListsOfMapsAreEqualIgnoringOrderByKey(
+	keyAttr string,
+	ignoreKeys ...string,
+) schema.SchemaDiffSuppressFunc {
 	// the attr is a path to the item's attribute, not the list itself, e.g. "numa.0.device"
 	return func(attr, _, _ string, d *schema.ResourceData) bool {
 		lastDotIndex := strings.LastIndex(attr, ".")
@@ -147,22 +154,34 @@ func SuppressIfListsOfMapsAreEqualIgnoringOrderByKey(keyAttr string) schema.Sche
 			return false
 		}
 
-		oldArray := oldData.([]interface{})
+		oldArray, ok := oldData.([]interface{})
-		newArray := newData.([]interface{})
+		if !ok {
+			return false
+		}
+
+		newArray, ok := newData.([]interface{})
+		if !ok {
+			return false
+		}
 
 		if len(oldArray) != len(newArray) {
 			return false
 		}
 
-		oldKeys := utils.MapResourceList(oldArray, keyAttr)
+		oldMap := utils.MapResourceList(oldArray, keyAttr)
-		newKeys := utils.MapResourceList(newArray, keyAttr)
+		newMap := utils.MapResourceList(newArray, keyAttr)
 
-		for k, v := range oldKeys {
+		for k, v := range oldMap {
-			if _, ok := newKeys[k]; !ok {
+			if _, ok := newMap[k]; !ok {
 				return false
 			}
 
-			if !reflect.DeepEqual(v, newKeys[k]) {
+			for _, ignoreKey := range ignoreKeys {
+				delete(v.(map[string]interface{}), ignoreKey)
+				delete(newMap[k].(map[string]interface{}), ignoreKey)
+			}
+
+			if !reflect.DeepEqual(v, newMap[k]) {
 				return false
 			}
 		}

utils/maps.go

@@ -32,8 +32,9 @@ func MapResourceList(resourceList []interface{}, attrName string) map[string]int
 		}
 
 		r := resource.(map[string]interface{})
-		key := r[attrName].(string)
+		if key, ok := r[attrName].(string); ok {
-		m[key] = r
+			m[key] = r
+		}
 	}
 
 	return m