Support server side copy/move of custom storage volumes in clusters #12386

monstermunchkin · 2023-10-13T18:37:24Z

Fixes #11445

tomponline · 2023-10-16T10:38:18Z

As discussed, lets lose the target group selection logic and always expect a specific cluster member name for target.

lxd/db/node.go

lxd/storage_volumes.go

client/lxd_storage_volumes.go

tomponline · 2023-10-23T08:58:11Z

@monstermunchkin please can you rebase and push again as the tests didnt run for some reason

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

tomponline · 2023-10-25T12:19:37Z

lxd/storage_volumes.go

+				if err != nil {
+					// Check if the user provided an incorrect target query parameter and return a helpful error message.
+					_, volumeNotFound := api.StatusErrorMatch(err, http.StatusNotFound)
+					targetIsSet := r.URL.Query().Get("target") != ""


Isn't the same as testing target := queryParam(r, "target") from line 1136?

tomponline · 2023-10-25T12:21:34Z

lxd/storage_volumes.go

+					serverIsClustered, _ := lxdCluster.Enabled(s.DB.Node)
+
+					if serverIsClustered && targetIsSet && volumeNotFound {
+						return response.NotFound(fmt.Errorf("Storage volume not found on this cluster member"))


Im a bit confused by this error as its only for ceph volumes that are not member specific?

tomponline · 2023-10-25T12:22:42Z

lxd/storage_volumes.go

+					// Check if the user provided an incorrect target query parameter and return a helpful error message.
+					_, volumeNotFound := api.StatusErrorMatch(err, http.StatusNotFound)
+					targetIsSet := r.URL.Query().Get("target") != ""
+					serverIsClustered, _ := lxdCluster.Enabled(s.DB.Node)


why are we doing this again here, it was ascertain on line 1139?

tomponline · 2023-10-25T12:31:09Z

client/lxd_storage_volumes.go

@@ -555,8 +555,10 @@ func (r *ProtocolLXD) CopyStoragePoolVolume(pool string, source InstanceServer,
 		return nil, fmt.Errorf("Failed to get destination connection info: %w", err)
 	}

+	clusterInternalVolumeCopy := r.CheckExtension("cluster_internal_custom_volume_copy") == nil
+
 	// Copy the storage pool volume locally.


comment needs updating now

tomponline

Please can you follow up with those requested changes as a separate PR?

monstermunchkin requested a review from tomponline as a code owner October 13, 2023 18:37

github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Oct 13, 2023

monstermunchkin force-pushed the issues/11445-server-side-move-for-custom-storage-volumes branch from 5166e54 to 8db2b54 Compare October 16, 2023 08:17