From d02dc1eb0a524d89a06b4b96b1d36c504ab737fe Mon Sep 17 00:00:00 2001 From: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Date: Wed, 29 May 2024 23:12:05 -0400 Subject: [PATCH] fix(vm): adding disks causes VM to be re-created (#1336) Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> --- .github/workflows/testacc.yml | 2 + fwprovider/test/resource_container_test.go | 2 +- fwprovider/test/resource_vm_test.go | 162 ++++++++++++++++++- fwprovider/test/test_environment.go | 18 ++- proxmoxtf/resource/vm/disk/disk.go | 177 +++++++++++---------- proxmoxtf/resource/vm/disk/schema.go | 2 - proxmoxtf/resource/vm/vm.go | 4 +- 7 files changed, 276 insertions(+), 91 deletions(-) diff --git a/.github/workflows/testacc.yml b/.github/workflows/testacc.yml index ceb3aae6..82d0bd5c 100644 --- a/.github/workflows/testacc.yml +++ b/.github/workflows/testacc.yml @@ -67,4 +67,6 @@ jobs: PROXMOX_VE_ACC_NODE_NAME: ${{ matrix.node }} PROXMOX_VE_ACC_NODE_SSH_ADDRESS: ${{ secrets.PROXMOX_VE_HOST }} PROXMOX_VE_ACC_NODE_SSH_PORT: ${{ matrix.port }} + PROXMOX_VE_ACC_CLOUD_IMAGES_SERVER: ${{ secrets.PROXMOX_VE_ACC_CLOUD_IMAGES_SERVER }} + PROXMOX_VE_ACC_CONTAINER_IMAGES_SERVER: ${{ secrets.PROXMOX_VE_ACC_CONTAINER_IMAGES_SERVER }} run: make testacc diff --git a/fwprovider/test/resource_container_test.go b/fwprovider/test/resource_container_test.go index 0e1329f0..c337b449 100644 --- a/fwprovider/test/resource_container_test.go +++ b/fwprovider/test/resource_container_test.go @@ -57,7 +57,7 @@ resource "proxmox_virtual_environment_download_file" "ubuntu_container_template" content_type = "vztmpl" datastore_id = "local" node_name = "{{.NodeName}}" - url = "http://download.proxmox.com/images/system/ubuntu-23.04-standard_23.04-1_amd64.tar.zst" + url = "{{.ContainerImagesServer}}/images/system/ubuntu-23.04-standard_23.04-1_amd64.tar.zst" overwrite_unmanaged = true } resource "proxmox_virtual_environment_container" "test_container" { diff --git a/fwprovider/test/resource_vm_test.go b/fwprovider/test/resource_vm_test.go index 5ead6a3d..2d45afc3 100644 --- a/fwprovider/test/resource_vm_test.go +++ b/fwprovider/test/resource_vm_test.go @@ -7,9 +7,11 @@ package test import ( + "regexp" "testing" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/plancheck" ) func TestAccResourceVM(t *testing.T) { @@ -295,7 +297,7 @@ func TestAccResourceVMInitialization(t *testing.T) { content_type = "iso" datastore_id = "local" node_name = "{{.NodeName}}" - url = "https://cloud-images.ubuntu.com/jammy/current/jammy-server-cloudimg-amd64.img" + url = "{{.CloudImagesServer}}/jammy/current/jammy-server-cloudimg-amd64.img" overwrite_unmanaged = true }`), }}}, @@ -390,7 +392,7 @@ func TestAccResourceVMNetwork(t *testing.T) { content_type = "iso" datastore_id = "local" node_name = "{{.NodeName}}" - url = "https://cloud-images.ubuntu.com/jammy/current/jammy-server-cloudimg-amd64.img" + url = "{{.CloudImagesServer}}/jammy/current/jammy-server-cloudimg-amd64.img" overwrite_unmanaged = true }`), Check: resource.ComposeTestCheckFunc( @@ -545,7 +547,7 @@ func TestAccResourceVMDisks(t *testing.T) { content_type = "iso" datastore_id = "local" node_name = "{{.NodeName}}" - url = "https://cloud-images.ubuntu.com/jammy/current/jammy-server-cloudimg-amd64.img" + url = "{{.CloudImagesServer}}/jammy/current/jammy-server-cloudimg-amd64.img" overwrite_unmanaged = true } resource "proxmox_virtual_environment_vm" "test_disk2" { @@ -643,6 +645,109 @@ func TestAccResourceVMDisks(t *testing.T) { RefreshState: true, }, }}, + {"adding disks", []resource.TestStep{ + { + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_disk" { + node_name = "{{.NodeName}}" + started = false + name = "test-disk" + + disk { + file_format = "raw" + datastore_id = "local-lvm" + interface = "scsi0" + size = 8 + } + }`), + Check: ResourceAttributes("proxmox_virtual_environment_vm.test_disk", map[string]string{ + "disk.0.interface": "scsi0", + "disk.0.path_in_datastore": `vm-\d+-disk-0`, + }), + }, + { + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_disk" { + node_name = "{{.NodeName}}" + started = false + name = "test-disk" + + disk { + file_format = "raw" + datastore_id = "local-lvm" + interface = "scsi0" + size = 8 + } + + disk { + file_format = "raw" + datastore_id = "local-lvm" + interface = "scsi1" + size = 8 + } + }`), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("proxmox_virtual_environment_vm.test_disk", plancheck.ResourceActionUpdate), + }, + }, + Check: ResourceAttributes("proxmox_virtual_environment_vm.test_disk", map[string]string{ + "disk.0.interface": "scsi0", + "disk.0.path_in_datastore": `vm-\d+-disk-0`, + "disk.1.interface": "scsi1", + "disk.1.path_in_datastore": `vm-\d+-disk-1`, + }), + }, + { + RefreshState: true, + }, + }}, + {"removing disks", []resource.TestStep{ + { + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_disk" { + node_name = "{{.NodeName}}" + started = false + name = "test-disk" + + disk { + file_format = "raw" + datastore_id = "local-lvm" + interface = "scsi0" + size = 8 + } + + disk { + file_format = "raw" + datastore_id = "local-lvm" + interface = "scsi1" + size = 8 + } + }`), + Check: ResourceAttributes("proxmox_virtual_environment_vm.test_disk", map[string]string{ + "disk.0.interface": "scsi0", + "disk.0.path_in_datastore": `vm-\d+-disk-0`, + "disk.1.interface": "scsi1", + "disk.1.path_in_datastore": `vm-\d+-disk-1`, + }), + }, + { + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_disk" { + node_name = "{{.NodeName}}" + started = false + name = "test-disk" + + disk { + file_format = "raw" + datastore_id = "local-lvm" + interface = "scsi0" + size = 8 + } + }`), + ExpectError: regexp.MustCompile(`deletion of disks not supported`), + }, + }}, {"cdrom", []resource.TestStep{ { @@ -833,6 +938,57 @@ func TestAccResourceVMDisks(t *testing.T) { RefreshState: true, }, }}, + {"clone with adding disk", []resource.TestStep{ + { + SkipFunc: func() (bool, error) { + // this test is failing because of "Attribute 'disk.1.size' expected to be set" + return true, nil + }, + Config: te.RenderConfig(` + resource "proxmox_virtual_environment_vm" "test_disk3_template" { + node_name = "{{.NodeName}}" + started = false + name = "test-disk3-template" + template = "true" + + disk { + file_format = "raw" + datastore_id = "local-lvm" + interface = "virtio0" + size = 8 + } + } + resource "proxmox_virtual_environment_vm" "test_disk3" { + node_name = "{{.NodeName}}" + started = false + name = "test-disk3" + + clone { + vm_id = proxmox_virtual_environment_vm.test_disk3_template.id + } + + disk { + file_format = "raw" + datastore_id = "local-lvm" + interface = "scsi0" + size = 10 + } + }`), + Check: resource.ComposeTestCheckFunc( + ResourceAttributes("proxmox_virtual_environment_vm.test_disk3", map[string]string{ + "disk.1.datastore_id": "local-lvm", + "disk.1.interface": "virtio0", + "disk.1.size": "8", + "disk.0.datastore_id": "local-lvm", + "disk.0.interface": "scsi0", + "disk.0.size": "10", + }), + ), + }, + { + RefreshState: true, + }, + }}, } for _, tt := range tests { diff --git a/fwprovider/test/test_environment.go b/fwprovider/test/test_environment.go index 22004e4c..11dc8dd7 100644 --- a/fwprovider/test/test_environment.go +++ b/fwprovider/test/test_environment.go @@ -78,12 +78,24 @@ provider "proxmox" { const datastoreID = "local" + cloudImagesServer := utils.GetAnyStringEnv("PROXMOX_VE_ACC_CLOUD_IMAGES_SERVER") + if cloudImagesServer == "" { + cloudImagesServer = "https://cloud-images.ubuntu.com" + } + + containerImagesServer := utils.GetAnyStringEnv("PROXMOX_VE_ACC_CONTAINER_IMAGES_SERVER") + if containerImagesServer == "" { + containerImagesServer = "http://download.proxmox.com" + } + return &Environment{ t: t, templateVars: map[string]any{ - "ProviderConfig": pc, - "NodeName": nodeName, - "DatastoreID": datastoreID, + "ProviderConfig": pc, + "NodeName": nodeName, + "DatastoreID": datastoreID, + "CloudImagesServer": cloudImagesServer, + "ContainerImagesServer": containerImagesServer, }, providerConfig: pc, NodeName: nodeName, diff --git a/proxmoxtf/resource/vm/disk/disk.go b/proxmoxtf/resource/vm/disk/disk.go index 0cf7086f..1373e35a 100644 --- a/proxmoxtf/resource/vm/disk/disk.go +++ b/proxmoxtf/resource/vm/disk/disk.go @@ -375,75 +375,68 @@ func CreateCustomDisks( vmID int, storageDevices map[string]vms.CustomStorageDevices, ) diag.Diagnostics { - // flatten the map of disk devices - var disks []vms.CustomStorageDevice - for _, diskDevice := range storageDevices { for _, disk := range diskDevice { - if disk != nil { - disks = append(disks, *disk) + if disk != nil && disk.FileID != nil && *disk.FileID != "" { + // only custom disks with defined file ID + err := createCustomDisk(ctx, client, nodeName, vmID, *disk) + if err != nil { + return diag.FromErr(err) + } } } } - commands := make([]string, 0, len(disks)) - resizes := make([]*vms.ResizeDiskRequestBody, 0, len(disks)) + return nil +} - for _, d := range disks { - if d.FileID == nil || *d.FileID == "" { - continue - } - - fileFormat := dvDiskFileFormat - if d.Format != nil && *d.Format != "" { - fileFormat = *d.Format - } - - //nolint:lll - commands = append( - commands, - `set -e`, - ssh.TrySudo, - fmt.Sprintf(`file_id="%s"`, *d.FileID), - fmt.Sprintf(`file_format="%s"`, fileFormat), - fmt.Sprintf(`datastore_id_target="%s"`, *d.DatastoreID), - fmt.Sprintf(`vm_id="%d"`, vmID), - fmt.Sprintf(`disk_options="%s"`, d.EncodeOptions()), - fmt.Sprintf(`disk_interface="%s"`, *d.Interface), - `source_image=$(try_sudo "pvesm path $file_id")`, - `imported_disk="$(try_sudo "qm importdisk $vm_id $source_image $datastore_id_target -format $file_format" | grep "unused0" | cut -d ":" -f 3 | cut -d "'" -f 1)"`, - `disk_id="${datastore_id_target}:$imported_disk,${disk_options}"`, - `try_sudo "qm set $vm_id -${disk_interface} $disk_id"`, - ) - - resizes = append(resizes, &vms.ResizeDiskRequestBody{ - Disk: *d.Interface, - Size: *d.Size, - }) +func createCustomDisk( + ctx context.Context, + client proxmox.Client, + nodeName string, + vmID int, + d vms.CustomStorageDevice, +) error { + fileFormat := dvDiskFileFormat + if d.Format != nil && *d.Format != "" { + fileFormat = *d.Format } - // Execute the commands on the node and wait for the result. - // This is a highly experimental approach to disk imports and is not recommended by Proxmox. - if len(commands) > 0 { - out, err := client.SSH().ExecuteNodeCommands(ctx, nodeName, commands) - if err != nil { - if matches, e := regexp.Match(`pvesm: .* not found`, out); e == nil && matches { - return diag.FromErr(ssh.NewErrUserHasNoPermission(client.SSH().Username())) - } + //nolint:lll + commands := []string{ + `set -e`, + ssh.TrySudo, + fmt.Sprintf(`file_id="%s"`, *d.FileID), + fmt.Sprintf(`file_format="%s"`, fileFormat), + fmt.Sprintf(`datastore_id_target="%s"`, *d.DatastoreID), + fmt.Sprintf(`vm_id="%d"`, vmID), + fmt.Sprintf(`disk_options="%s"`, d.EncodeOptions()), + fmt.Sprintf(`disk_interface="%s"`, *d.Interface), + `source_image=$(try_sudo "pvesm path $file_id")`, + `imported_disk="$(try_sudo "qm importdisk $vm_id $source_image $datastore_id_target -format $file_format" | grep "unused0" | cut -d ":" -f 3 | cut -d "'" -f 1)"`, + `disk_id="${datastore_id_target}:$imported_disk,${disk_options}"`, + `try_sudo "qm set $vm_id -${disk_interface} $disk_id"`, + } - return diag.FromErr(err) + out, err := client.SSH().ExecuteNodeCommands(ctx, nodeName, commands) + if err != nil { + if matches, e := regexp.Match(`pvesm: .* not found`, out); e == nil && matches { + err = ssh.NewErrUserHasNoPermission(client.SSH().Username()) } - tflog.Debug(ctx, "vmCreateCustomDisks: commands", map[string]interface{}{ - "output": string(out), - }) + return fmt.Errorf("creating custom disk: %w", err) + } - for _, resize := range resizes { - err = client.Node(nodeName).VM(vmID).ResizeVMDisk(ctx, resize) - if err != nil { - return diag.FromErr(err) - } - } + tflog.Debug(ctx, "vmCreateCustomDisks: commands", map[string]interface{}{ + "output": string(out), + }) + + err = client.Node(nodeName).VM(vmID).ResizeVMDisk(ctx, &vms.ResizeDiskRequestBody{ + Disk: *d.Interface, + Size: *d.Size, + }) + if err != nil { + return fmt.Errorf("resizing disk: %w", err) } return nil @@ -455,7 +448,7 @@ func Read( d *schema.ResourceData, diskObjects vms.CustomStorageDevices, vmID int, - api proxmox.Client, + client proxmox.Client, nodeName string, isClone bool, ) diag.Diagnostics { @@ -492,7 +485,7 @@ func Read( if datastoreID != "" { // disk format may not be returned by config API if it is default for the storage, and that may be different // from the default qcow2, so we need to read it from the storage API to make sure we have the correct value - volume, e := api.Node(nodeName).Storage(datastoreID).GetDatastoreFile(ctx, dd.FileVolume) + volume, e := client.Node(nodeName).Storage(datastoreID).GetDatastoreFile(ctx, dd.FileVolume) if e != nil { diags = append(diags, diag.FromErr(e)...) continue @@ -638,9 +631,13 @@ func Read( // Update updates the disk configuration of a VM. func Update( + ctx context.Context, + client proxmox.Client, + nodeName string, + vmID int, d *schema.ResourceData, planDisks map[string]vms.CustomStorageDevices, - allDiskInfo vms.CustomStorageDevices, + currentDisks vms.CustomStorageDevices, updateBody *vms.UpdateRequestBody, ) (bool, error) { rebootRequired := false @@ -651,32 +648,52 @@ func Update( continue } - for key, value := range diskMap { - if allDiskInfo[key] == nil { + for key, disk := range diskMap { + var tmp *vms.CustomStorageDevice + + switch { + case currentDisks[key] == nil && disk != nil: + if disk.FileID != nil && *disk.FileID != "" { + // only disks with defined file ID are custom image disks that need to be created via import. + err := createCustomDisk(ctx, client, nodeName, vmID, *disk) + if err != nil { + return false, fmt.Errorf("creating custom disk: %w", err) + } + } else { + // otherwise this is a blank disk that can be added directly via update API + tmp = disk + } + case currentDisks[key] != nil: + // update existing disk + tmp = currentDisks[key] + default: + // something went wrong return false, fmt.Errorf("missing %s device %s", prefix, key) } - tmp := allDiskInfo[key] - - if tmp.AIO != value.AIO { - rebootRequired = true - tmp.AIO = value.AIO + if tmp == nil || disk == nil { + continue } - tmp.Backup = value.Backup - tmp.BurstableReadSpeedMbps = value.BurstableReadSpeedMbps - tmp.BurstableWriteSpeedMbps = value.BurstableWriteSpeedMbps - tmp.Cache = value.Cache - tmp.Discard = value.Discard - tmp.IOThread = value.IOThread - tmp.IopsRead = value.IopsRead - tmp.IopsWrite = value.IopsWrite - tmp.MaxIopsRead = value.MaxIopsRead - tmp.MaxIopsWrite = value.MaxIopsWrite - tmp.MaxReadSpeedMbps = value.MaxReadSpeedMbps - tmp.MaxWriteSpeedMbps = value.MaxWriteSpeedMbps - tmp.Replicate = value.Replicate - tmp.SSD = value.SSD + if tmp.AIO != disk.AIO { + rebootRequired = true + tmp.AIO = disk.AIO + } + + tmp.Backup = disk.Backup + tmp.BurstableReadSpeedMbps = disk.BurstableReadSpeedMbps + tmp.BurstableWriteSpeedMbps = disk.BurstableWriteSpeedMbps + tmp.Cache = disk.Cache + tmp.Discard = disk.Discard + tmp.IOThread = disk.IOThread + tmp.IopsRead = disk.IopsRead + tmp.IopsWrite = disk.IopsWrite + tmp.MaxIopsRead = disk.MaxIopsRead + tmp.MaxIopsWrite = disk.MaxIopsWrite + tmp.MaxReadSpeedMbps = disk.MaxReadSpeedMbps + tmp.MaxWriteSpeedMbps = disk.MaxWriteSpeedMbps + tmp.Replicate = disk.Replicate + tmp.SSD = disk.SSD switch prefix { case "virtio": diff --git a/proxmoxtf/resource/vm/disk/schema.go b/proxmoxtf/resource/vm/disk/schema.go index a82aa478..96f956c8 100644 --- a/proxmoxtf/resource/vm/disk/schema.go +++ b/proxmoxtf/resource/vm/disk/schema.go @@ -50,7 +50,6 @@ func Schema() map[string]*schema.Schema { Type: schema.TypeList, Description: "The disk devices", Optional: true, - ForceNew: true, DefaultFunc: func() (interface{}, error) { return []interface{}{ map[string]interface{}{ @@ -97,7 +96,6 @@ func Schema() map[string]*schema.Schema { Type: schema.TypeString, Description: "The file format", Optional: true, - ForceNew: true, Computed: true, ValidateDiagFunc: validators.FileFormat(), }, diff --git a/proxmoxtf/resource/vm/vm.go b/proxmoxtf/resource/vm/vm.go index 96f2a981..a25f30ea 100644 --- a/proxmoxtf/resource/vm/vm.go +++ b/proxmoxtf/resource/vm/vm.go @@ -5057,7 +5057,7 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.D return diag.FromErr(err) } - rr, err := disk.Update(d, planDisks, allDiskInfo, updateBody) + rr, err := disk.Update(ctx, client, nodeName, vmID, d, planDisks, allDiskInfo, updateBody) if err != nil { return diag.FromErr(err) } @@ -5473,7 +5473,7 @@ func vmUpdateDiskLocationAndSize( for oldKey, oldDisk := range diskMap { if _, present := diskNewEntries[prefix][oldKey]; !present { return diag.Errorf( - "deletion of disks not supported. Please delete disk by hand. Old Interface was %s", + "deletion of disks not supported. Please delete disk by hand. Old interface was %q", *oldDisk.Interface, ) }