Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add the ability to clone to non-shared storage on different nodes #178

Merged
merged 13 commits into from
Dec 12, 2022
4 changes: 4 additions & 0 deletions docs/resources/virtual_environment_vm.md
Original file line number Diff line number Diff line change
Expand Up @@ -325,3 +325,7 @@ output "ubuntu_vm_public_key" {

When cloning an existing virtual machine, whether it's a template or not, the resource will only detect changes to the
arguments which are not set to their default values.

Furthermore, when cloning from one node to a different one, the behavior changes depening on the datastores of the source VM. If at least one non-shared datastore is used, the VM is first cloned to the source node before being migrated to the target node. This circumvents a limitation in the Proxmox clone API.

**Note:** Because the migration step after the clone tries to preserve the used datastores by their name, it may fail if a datastore used in the source VM is not available on the target node (e.g. `local-lvm` is used on the source node in the VM but no `local-lvm` datastore is availabel on the target node). In this case, it is recommended to set the `datastore_id` argument in the `clone` block to force the migration step to migrate all disks to a specific datastore on the target node. If you need certain disks to be on specific datastores, set the `datastore_id` argument of the disks in the `disks` block to move the disks to the correct datastore after the cloning and migrating succeeded.
19 changes: 18 additions & 1 deletion proxmox/virtual_environment_datastores.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@ import (
"context"
"errors"
"fmt"
"github.com/hashicorp/terraform-plugin-log/tflog"
"io"
"mime/multipart"
"net/url"
"os"
"sort"
"strings"

"github.com/hashicorp/terraform-plugin-log/tflog"

"github.com/pkg/sftp"
)

Expand All @@ -30,6 +31,22 @@ func (c *VirtualEnvironmentClient) DeleteDatastoreFile(ctx context.Context, node
return nil
}

// GetDatastoreStatus gets status information for a given datastore.
func (c *VirtualEnvironmentClient) GetDatastoreStatus(ctx context.Context, nodeName, datastoreID string) (*VirtualEnvironmentDatastoreGetStatusResponseData, error) {
resBody := &VirtualEnvironmentDatastoreGetStatusResponseBody{}
err := c.DoRequest(ctx, hmGET, fmt.Sprintf("nodes/%s/storage/%s/status", url.PathEscape(nodeName), url.PathEscape(datastoreID)), nil, resBody)

if err != nil {
return nil, err
}

if resBody.Data == nil {
return nil, errors.New("the server did not include a data object in the response")
}

return resBody.Data, nil
}

// ListDatastoreFiles retrieves a list of the files in a datastore.
func (c *VirtualEnvironmentClient) ListDatastoreFiles(ctx context.Context, nodeName, datastoreID string) ([]*VirtualEnvironmentDatastoreFileListResponseData, error) {
resBody := &VirtualEnvironmentDatastoreFileListResponseBody{}
Expand Down
17 changes: 17 additions & 0 deletions proxmox/virtual_environment_datastores_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,23 @@ type VirtualEnvironmentDatastoreFileListResponseData struct {
VolumeID string `json:"volid"`
}

// VirtualEnvironmentDatastoreGetStatusResponseBody contains the body from a datastore status get request.
type VirtualEnvironmentDatastoreGetStatusResponseBody struct {
Data *VirtualEnvironmentDatastoreGetStatusResponseData `json:"data,omitempty"`
}

// VirtualEnvironmentDatastoreGetStatusResponseBody contains the data from a datastore status get request.
type VirtualEnvironmentDatastoreGetStatusResponseData struct {
Active *CustomBool `json:"active,omitempty"`
AvailableBytes *int64 `json:"avail,omitempty"`
Content *CustomCommaSeparatedList `json:"content,omitempty" url:"content,omitempty,comma"`
Enabled *CustomBool `json:"enabled,omitempty"`
Shared *CustomBool `json:"shared,omitempty"`
TotalBytes *int64 `json:"total,omitempty"`
Type *string `json:"type,omitempty"`
UsedBytes *int64 `json:"used,omitempty"`
}

// VirtualEnvironmentDatastoreListRequestBody contains the body for a datastore list request.
type VirtualEnvironmentDatastoreListRequestBody struct {
ContentTypes CustomCommaSeparatedList `json:"content,omitempty" url:"content,omitempty,comma"`
Expand Down
36 changes: 35 additions & 1 deletion proxmox/virtual_environment_vm.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@ import (
"context"
"errors"
"fmt"
"github.com/hashicorp/terraform-plugin-log/tflog"
"net"
"net/url"
"strings"
"sync"
"time"

"github.com/hashicorp/terraform-plugin-log/tflog"
)

const (
Expand Down Expand Up @@ -163,6 +164,39 @@ func (c *VirtualEnvironmentClient) GetVMStatus(ctx context.Context, nodeName str
return resBody.Data, nil
}

// MigrateVM migrates a virtual machine.
func (c *VirtualEnvironmentClient) MigrateVM(ctx context.Context, nodeName string, vmID int, d *VirtualEnvironmentVMMigrateRequestBody, timeout int) error {
taskID, err := c.MigrateVMAsync(ctx, nodeName, vmID, d)

if err != nil {
return err
}

err = c.WaitForNodeTask(ctx, nodeName, *taskID, timeout, 5)

if err != nil {
return err
}

return nil
}

// MigrateVMAsync migrates a virtual machine asynchronously.
func (c *VirtualEnvironmentClient) MigrateVMAsync(ctx context.Context, nodeName string, vmID int, d *VirtualEnvironmentVMMigrateRequestBody) (*string, error) {
resBody := &VirtualEnvironmentVMMigrateResponseBody{}
err := c.DoRequest(ctx, hmPOST, fmt.Sprintf("nodes/%s/qemu/%d/migrate", url.PathEscape(nodeName), vmID), d, resBody)

if err != nil {
return nil, err
}

if resBody.Data == nil {
return nil, errors.New("the server did not include a data object in the response")
}

return resBody.Data, nil
}

// MoveVMDisk moves a virtual machine disk.
func (c *VirtualEnvironmentClient) MoveVMDisk(ctx context.Context, nodeName string, vmID int, d *VirtualEnvironmentVMMoveDiskRequestBody, timeout int) error {
taskID, err := c.MoveVMDiskAsync(ctx, nodeName, vmID, d)
Expand Down
13 changes: 13 additions & 0 deletions proxmox/virtual_environment_vm_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,19 @@ type VirtualEnvironmentVMListResponseData struct {
ACPI *CustomBool `json:"acpi,omitempty" url:"acpi,omitempty,int"`
}

// VirtualEnvironmentVMMigrateRequestBody contains the body for a VM migration request.
type VirtualEnvironmentVMMigrateRequestBody struct {
OnlineMigration *CustomBool `json:"online,omitempty" url:"online,omitempty"`
TargetNode string `json:"target" url:"target"`
TargetStorage *string `json:"targetstorage,omitempty" url:"targetstorage,omitempty"`
WithLocalDisks *CustomBool `json:"with-local-disks,omitempty" url:"with-local-disks,omitempty,int"`
}

// VirtualEnvironmentVMMigrateResponseBody contains the body from a VM migrate response.
type VirtualEnvironmentVMMigrateResponseBody struct {
Data *string `json:"data,omitempty"`
}

// VirtualEnvironmentVMMoveDiskRequestBody contains the body for a VM move disk request.
type VirtualEnvironmentVMMoveDiskRequestBody struct {
BandwidthLimit *int `json:"bwlimit,omitempty" url:"bwlimit,omitempty"`
Expand Down
70 changes: 66 additions & 4 deletions proxmoxtf/resource_virtual_environment_vm.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ import (
"context"
"errors"
"fmt"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
"sort"
"strconv"
"strings"
"time"

"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"

"github.com/bpg/terraform-provider-proxmox/proxmox"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
Expand Down Expand Up @@ -1164,9 +1165,70 @@ func resourceVirtualEnvironmentVMCreateClone(ctx context.Context, d *schema.Reso
cloneTimeout := d.Get(mkResourceVirtualEnvironmentVMTimeoutClone).(int)

if cloneNodeName != "" && cloneNodeName != nodeName {
cloneBody.TargetNodeName = &nodeName
// Check if any used datastores of the source VM are not shared
vmConfig, err := veClient.GetVM(ctx, cloneNodeName, cloneVMID)
if err != nil {
return diag.FromErr(err)
}

err = veClient.CloneVM(ctx, cloneNodeName, cloneVMID, cloneRetries, cloneBody, cloneTimeout)
datastores := getDiskDatastores(vmConfig, d)

onlySharedDatastores := true
for _, datastore := range datastores {
datastoreStatus, err := veClient.GetDatastoreStatus(ctx, cloneNodeName, datastore)
if err != nil {
return diag.FromErr(err)
}

if datastoreStatus.Shared != nil && *datastoreStatus.Shared == proxmox.CustomBool(false) {
onlySharedDatastores = false
break
}
}

if onlySharedDatastores {
// If the source and the target node are not the same, only clone directly to the target node if
// all used datastores in the source VM are shared. Directly cloning to non-shared storage
// on a different node is currently not supported by proxmox.
cloneBody.TargetNodeName = &nodeName
err = veClient.CloneVM(ctx, cloneNodeName, cloneVMID, cloneRetries, cloneBody, cloneTimeout)
if err != nil {
return diag.FromErr(err)
}
} else {
// If the source and the target node are not the same and any used datastore in the source VM is
// not shared, clone to the source node and then migrate to the target node. This is a workaround
// for missing functionality in the proxmox api as recommended per
// https://forum.proxmox.com/threads/500-cant-clone-to-non-shared-storage-local.49078/#post-229727

// Temporarily clone to local node
err = veClient.CloneVM(ctx, cloneNodeName, cloneVMID, cloneRetries, cloneBody, cloneTimeout)
if err != nil {
return diag.FromErr(err)
}

// Wait for the virtual machine to be created and its configuration lock to be released before migrating.
err = veClient.WaitForVMConfigUnlock(ctx, cloneNodeName, vmID, 600, 5, true)
if err != nil {
return diag.FromErr(err)
}

// Migrate to target node
withLocalDisks := proxmox.CustomBool(true)
migrateBody := &proxmox.VirtualEnvironmentVMMigrateRequestBody{
TargetNode: nodeName,
WithLocalDisks: &withLocalDisks,
}

if cloneDatastoreID != "" {
migrateBody.TargetStorage = &cloneDatastoreID
}

err = veClient.MigrateVM(ctx, cloneNodeName, vmID, migrateBody, cloneTimeout)
Copy link
Contributor Author

@wbpascal wbpascal Dec 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My local linter says that this is an ineffectual assignment to err. I don't get this because if I understand this linter error correctly, it indicates that err is definitely overwritten after this assignment before it is read the next time (the if in line 1232). I can't see how this is possible. Can you please help me with this @bpg?

Copy link
Owner

@bpg bpg Dec 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means the value of err is not used/checked. Just add

	if err != nil {
		return diag.FromErr(err)
	}

on line 1228, and it should be good to go.

if err != nil {
return diag.FromErr(err)
}
}
} else {
err = veClient.CloneVM(ctx, nodeName, cloneVMID, cloneRetries, cloneBody, cloneTimeout)
}
Expand Down
27 changes: 25 additions & 2 deletions proxmoxtf/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ package proxmoxtf
import (
"errors"
"fmt"
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"math"
"regexp"
"strconv"
Expand All @@ -17,6 +15,9 @@ import (
"time"
"unicode"

"github.com/hashicorp/go-multierror"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"

"github.com/bpg/terraform-provider-proxmox/proxmox"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
Expand Down Expand Up @@ -463,6 +464,28 @@ func getDiskInfo(vm *proxmox.VirtualEnvironmentVMGetResponseData, d *schema.Reso
return storageDevices
}

// getDiskDatastores returns a list of the used datastores in a VM
func getDiskDatastores(vm *proxmox.VirtualEnvironmentVMGetResponseData, d *schema.ResourceData) []string {
storageDevices := getDiskInfo(vm, d)
datastoresSet := map[string]int{}

for _, diskInfo := range storageDevices {
// Ignore empty storage devices and storage devices (like ide) which may not have any media mounted
if diskInfo == nil || diskInfo.FileVolume == "none" {
continue
}
fileIDParts := strings.Split(diskInfo.FileVolume, ":")
datastoresSet[fileIDParts[0]] = 1
}

datastores := []string{}
for datastore := range datastoresSet {
datastores = append(datastores, datastore)
}

return datastores
}

func parseDiskSize(size *string) (int, error) {
var diskSize int
var err error
Expand Down