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

wbpascal · 2022-12-09T21:19:16Z

Introduction

As mentioned in various different forum posts on places like the Proxmox forums (e.g. https://forum.proxmox.com/threads/500-cant-clone-to-non-shared-storage-local.49078/#post-229727), the Promox API currently does not support using the Clone API to clone a VM to a different node if the target datastore is non-shared. The above forum post also mentions a workaround, which can be used to circumvent this limitation which is to first clone the VM to the local node (with a non-shared datastore) and then migrate it to the target node and datastore using the Migrate API.

I have added this workaround to the provider under the specific condition that the user wants to clone a VM to a different node than the source node and the target datastore is non-shared. Because we first have to clone the VM to the source node, I have also added a new argument temp_datastore_id to the clone block. In this argument, the user can enter the local datastore on the source VM to which the VM is cloned in the first step before migration.

Please be aware that this is my first time writing code in Go, so I am happy for any ideas and suggestions for improvements.

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #0000

bpg · 2022-12-09T22:36:31Z

Awesome! Thanks for the PR @wbpascal, I'll review it in the next few days.

In the meantime, would you mind to address the linter issue(s)?

bpg

Great work @wbpascal! I like the comments in the code, and the way you structured it. The provider's code base is not the best to work with, but that's what we have.

I can't really test this change on multiple nodes, as I don't have a clustered PVE in my lab, but my test on a single node has failed. Can you please take a look at my comments to the PR?

bpg · 2022-12-10T17:46:02Z

proxmox/virtual_environment_vm.go

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

+// MigrateVMAsync migrates a virtual machine.


Suggested change

// MigrateVMAsync migrates a virtual machine.

// MigrateVM migrates a virtual machine.

bpg · 2022-12-10T17:48:53Z

proxmoxtf/resource_virtual_environment_vm.go

@@ -373,6 +376,13 @@ func resourceVirtualEnvironmentVM() *schema.Resource {
 							ForceNew:    true,
 							Default:     dvResourceVirtualEnvironmentVMCloneNodeName,
 						},
+						mkResourceVirtualEnvironmentVMCloneTempDatastoreID: {


Could you pls add the new argument to the doc?
https://github.com/bpg/terraform-provider-proxmox/blob/main/docs/resources/virtual_environment_vm.md

bpg · 2022-12-10T18:09:04Z

proxmox/virtual_environment_datastores.go

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

+// ListDatastoreFiles gets status information for a given datastore.


Suggested change

// ListDatastoreFiles gets status information for a given datastore.

// GetDatastoreStatus gets status information for a given datastore.

bpg · 2022-12-10T18:23:13Z

proxmoxtf/resource_virtual_environment_vm.go

@@ -1163,10 +1174,52 @@ func resourceVirtualEnvironmentVMCreateClone(ctx context.Context, d *schema.Reso

 	cloneTimeout := d.Get(mkResourceVirtualEnvironmentVMTimeoutClone).(int)

-	if cloneNodeName != "" && cloneNodeName != nodeName {
+	datastoreStatus, err := veClient.GetDatastoreStatus(ctx, cloneNodeName, cloneDatastoreID)


If cloneNodeName is empty the veClient.GetDatastoreStatus call will fail. I would leave the if statement that was here before and put the logic under it. This will also allow to remove duplicate condition on lines 1189 and 1196.
I.e.

if cloneNodeName != "" && cloneNodeName != nodeName { ... if cloneDatastoreShared { // simple clone here } else { // clone to temp then migrate } } else { // simple clone on the same node }

bpg · 2022-12-10T19:24:24Z

proxmoxtf/resource_virtual_environment_vm.go

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


Just a small style thing, I would keep error check right on the next line after the call that produced the error, here and line 1178.
I think there is already a linting rule for that, but I have most of them disabled at the moment. You know, the original code is not super great.

wbpascal · 2022-12-10T21:55:52Z

Thanks for the review @bpg! So this kind of opened another can of worms because I remembered that source VMs may use multiple different datastores (of which only some may be shared while others may not). Therefore to properly decide if the workaround needs to be used, all datastores used in the source VM have to be iterated over.

Also it is probably in our interest to not move any disks to different storages until we absolutely have to (e.g. disks that are already on a shared storage), so I modified the workaround to not use specific target datastores where possible. This should work in most cases, except in some where local datastores may differ between nodes. I have documented this case and how to work around it.

This also means that the temp_datastore_id argument is no longer necessary.

wbpascal · 2022-12-10T22:01:53Z

proxmoxtf/resource_virtual_environment_vm.go

+				migrateBody.TargetStorage = &cloneDatastoreID
+			}
+
+			err = veClient.MigrateVM(ctx, cloneNodeName, vmID, migrateBody, cloneTimeout)


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?

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.

wbpascal · 2022-12-12T20:29:25Z

I think I have fixed every outstanding issue now. Could you please take another look when you have time, @bpg?

product-auto-label bot added size: m labels Dec 9, 2022

wbpascal added 3 commits December 9, 2022 22:26

feat: add workaround for cloning to non-shared storage

0f2ad30

fix: fix wrong API params used

39c0628

test: add new var to tests

72756e0

wbpascal force-pushed the main branch from 7d8d0be to 72756e0 Compare December 9, 2022 21:27

wbpascal changed the title ~~add the ability to clone to non-shared storage on different nodes~~ feat: add the ability to clone to non-shared storage on different nodes Dec 9, 2022

wbpascal added 2 commits December 10, 2022 09:22

fix: lint issues

4b21c19

Merge branch 'main' into main

436036e

bpg requested changes Dec 10, 2022

View reviewed changes

wbpascal added 5 commits December 10, 2022 20:42

docs: add new argument to docs

3d69285

docs: fix function documentation

fc1951c

fix: better work with heterogeneous datastores

f4d6091

docs: clarify clone behavior

ecbd407

Merge branch 'main' of github.com:wbpascal/terraform-provider-proxmox

c335554

wbpascal commented Dec 10, 2022

View reviewed changes

bpg and others added 2 commits December 11, 2022 11:24

Merge branch 'main' into main

e8ad704

fix: go lint issues

e79720d

Merge branch 'main' into main

e34aa0a

bpg approved these changes Dec 12, 2022

View reviewed changes

bpg merged commit 0df14f9 into bpg:main Dec 12, 2022

ghost mentioned this pull request Dec 12, 2022

chore(main): release 0.8.0 #170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wbpascal commented Dec 9, 2022 •

edited

Loading

bpg commented Dec 9, 2022 •

edited

Loading

bpg left a comment

bpg Dec 10, 2022

bpg Dec 10, 2022

bpg Dec 10, 2022

bpg Dec 10, 2022

bpg Dec 10, 2022

wbpascal commented Dec 10, 2022 •

edited

Loading

wbpascal Dec 10, 2022 •

edited

Loading

bpg Dec 11, 2022 •

edited

Loading

wbpascal commented Dec 12, 2022

	// MigrateVMAsync migrates a virtual machine.
	// MigrateVM migrates a virtual machine.

	// ListDatastoreFiles gets status information for a given datastore.
	// GetDatastoreStatus gets status information for a given datastore.

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

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

Conversation

wbpascal commented Dec 9, 2022 • edited Loading

Introduction

Community Note

bpg commented Dec 9, 2022 • edited Loading

bpg left a comment

Choose a reason for hiding this comment

bpg Dec 10, 2022

Choose a reason for hiding this comment

bpg Dec 10, 2022

Choose a reason for hiding this comment

bpg Dec 10, 2022

Choose a reason for hiding this comment

bpg Dec 10, 2022

Choose a reason for hiding this comment

bpg Dec 10, 2022

Choose a reason for hiding this comment

wbpascal commented Dec 10, 2022 • edited Loading

wbpascal Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

bpg Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

wbpascal commented Dec 12, 2022

wbpascal commented Dec 9, 2022 •

edited

Loading

bpg commented Dec 9, 2022 •

edited

Loading

wbpascal commented Dec 10, 2022 •

edited

Loading

wbpascal Dec 10, 2022 •

edited

Loading

bpg Dec 11, 2022 •

edited

Loading