Re: [pve-devel] [PATCH v4 storage 2/6] api: content: implement moving a volume between storages

[Daniel Kral <d.kral@proxmox.com>]

On 9/18/24 16:49, Filip Schauer wrote:
> Add the ability to move an iso, snippet or vztmpl between storages and
> nodes.
> 
> Use curl to call the API method:
> 
> ```
> curl https://$APINODE:8006/api2/json/nodes/$SOURCENODE/storage/$SOURCESTORAGE/content/$SOURCEVOLUME \
>      --insecure --cookie "$(<cookie)" -H "$(<csrftoken)" -X POST \
>      --data-raw "target-storage=$TARGETSTORAGE&target-node=$TARGETNODE"
> ```

I've tested this with the pvesh and with curl, but used an API token for 
authentication. This worked as expected with iso images, VM templates 
and Cloudinit snippets for moving between storages on the same node.

I tested changing the permissions the API token had for my testing 
source and target storage by separately giving them Datastore.Allocate 
and Datastore.AllocateSpace permissions and the checks worked as 
expected. This worked between local and a cephfs storage as expected.

> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>   src/PVE/API2/Storage/Content.pm | 149 ++++++++++++++++++++++++--------
>   1 file changed, 114 insertions(+), 35 deletions(-)
> 
> diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
> index fe0ad4a..6819eca 100644
> --- a/src/PVE/API2/Storage/Content.pm
> +++ b/src/PVE/API2/Storage/Content.pm
> @@ -2,7 +2,10 @@ package PVE::API2::Storage::Content;
>   
>   use strict;
>   use warnings;
> -use Data::Dumper;
> +
> +use Errno qw(ENOENT);
> +use File::Basename;
> +use File::Copy qw(copy move);
>   
>   use PVE::SafeSyslog;
>   use PVE::Cluster;
> @@ -483,30 +486,101 @@ __PACKAGE__->register_method ({
>   	return $upid;
>       }});
>   
> +sub volume_move {
> +    my ($cfg, $source_volid, $target_storeid, $delete) = @_;
> +
> +    my ($source_storeid, $source_volname) = PVE::Storage::parse_volume_id($source_volid, 0);
> +
> +    die "source and target storage cannot be the same\n" if $source_storeid eq $target_storeid;
> +
> +    my $source_scfg = PVE::Storage::storage_config($cfg, $source_storeid);
> +    my $source_plugin = PVE::Storage::Plugin->lookup($source_scfg->{type});
> +    my ($vtype) = $source_plugin->parse_volname($source_volname);
> +
> +    die "source storage '$source_storeid' does not support volumes of type '$vtype'\n"
> +	if !$source_scfg->{content}->{$vtype};
> +
> +    my $target_scfg = PVE::Storage::storage_config($cfg, $target_storeid);
> +    die "target storage '$target_storeid' does not support volumes of type '$vtype'\n"
> +	if !$target_scfg->{content}->{$vtype};
> +
> +    die "use pct move-volume or qm disk move\n" if $vtype eq 'images' || $vtype eq 'rootdir';
> +    die "moving volume of type '$vtype' not implemented\n"
> +	if ($vtype ne 'iso' && $vtype ne 'vztmpl' && $vtype ne 'snippets');
> +
> +    PVE::Storage::activate_storage($cfg, $source_storeid);
> +
> +    die "expected storage '$source_storeid' to be path based\n" if !$source_scfg->{path};
> +    my $source_path = $source_plugin->path($source_scfg, $source_volname, $source_storeid);
> +    die "$source_path does not exist" if (!-e $source_path);
> +    my $source_dirname = dirname($source_path);
> +
> +    die "expected storage '$target_storeid' to be path based\n" if !$target_scfg->{path};
> +    my $target_plugin = PVE::Storage::Plugin->lookup($target_scfg->{type});
> +    my $target_path = $target_plugin->path($target_scfg, $source_volname, $target_storeid);
> +
> +    PVE::Storage::activate_storage($cfg, $target_storeid);
> +    die "$target_path already exists" if (-e $target_path);
> +
> +    my @created_files = ();
> +
> +    eval {
> +	if ($delete) {
> +	    move($source_path, $target_path) or die "failed to move $vtype: $!";
> +	} else {
> +	    copy($source_path, $target_path) or die "failed to copy $vtype: $!";
> +	}
> +    };
> +    if (my $err = $@) {
> +	for my $created_file (@created_files) {
> +	    unlink $created_file or $!{ENOENT} or warn $!;
> +	}
> +	die $err;
> +    }
> +
> +    PVE::Storage::archive_remove($source_path) if $delete;
> +
> +    return;
> +}
> +
>   __PACKAGE__->register_method ({
> -    name => 'copy',
> +    name => 'move',

nit: just a thought, but since it isn't the default to remove the volume 
from the source storage, is the name 'move' a good fit? As a user, I 
would expect a move to delete the source on success, but not a copy by 
default. I can see that this would have the semantics of explicitly 
calling something like `rsync --remove-source-files` now. Otherwise, I 
think this is a great addition to have in pvesm!

>       path => '{volume}',
>       method => 'POST',
> -    description => "Copy a volume. This is experimental code - do not use.",
> +    description => "Move a volume.",
> +    permissions => {
> +	description => "If the --delete option is used, the 'Datastore.Allocate' privilege is " .
> +	    "required on the source storage. " .
> +	    "Without --delete, 'Datastore.AllocateSpace' is required on the target storage. " .
> +	    "When moving a backup, 'VM.Backup' is required on the VM or container.",
> +	user => 'all',
> +    },
>       protected => 1,
>       proxyto => 'node',
>       parameters => {
> -    	additionalProperties => 0,
> +	additionalProperties => 0,
>   	properties => {
>   	    node => get_standard_option('pve-node'),
> -	    storage => get_standard_option('pve-storage-id', { optional => 1}),
> +	    storage => get_standard_option('pve-storage-id', { optional => 1 }),

Users could benefit from a
`completion => \&PVE::Storage::complete_storage_enabled`
here

>   	    volume => {
>   		description => "Source volume identifier",
>   		type => 'string',
>   	    },

Users could benefit from a
`completion => \&PVE::Storage::complete_volume`
here, as writing volume names could be a bit tedious ;)

Also if I'm not missing something, this could also use a
`format => 'pve-volume-id'`, but I can see that it isn't used in any 
other route in that module and is also only used in 
`PVE::Storage::Plugin::LVMPlugin`, `PVE::Storage::CLI::pvesm` and 
`pve-container` as well as `qemu-server`.

> -	    target => {
> -		description => "Target volume identifier",
> +	    'target-storage' => {
> +		description => "Target storage",
>   		type => 'string',
>   	    },

same as 'storage' above, maybe even something like this:

```
'target-storage' => get_standard_option('pve-storage-id', {
     description => "Target storage",
     completion => \&PVE::Storage::complete_storage_enabled,
})
```

> -	    target_node => get_standard_option('pve-node',  {
> +	    'target-node' => get_standard_option('pve-node',  {
>   		description => "Target node. Default is local node.",
>   		optional => 1,
>   	    }),
> +	    delete => {
> +		type => 'boolean',
> +		description => "Delete the original volume after a successful copy. " .
> +		    "By default the original is kept.",
> +		optional => 1,
> +		default => 0,
> +	    },

Another option that could be great here would be a `allow-rename` option 
similar to that of `import`, as one cannot declare that when trying to 
move a volume from a node to another, which will fail if the content 
already exists there.

>   	},
>       },
>       returns => {
> @@ -515,43 +589,48 @@ __PACKAGE__->register_method ({
>       code => sub {
>   	my ($param) = @_;
>   
> -	my $rpcenv = PVE::RPCEnvironment::get();
> -
> -	my $user = $rpcenv->get_user();
> -
> -	my $target_node = $param->{target_node} || PVE::INotify::nodename();
> -	# pvesh examples
> -	# cd /nodes/localhost/storage/local/content
> -	# pve:/> create local:103/vm-103-disk-1.raw -target local:103/vm-103-disk-2.raw
> -	# pve:/> create 103/vm-103-disk-1.raw -target 103/vm-103-disk-3.raw
> -
>   	my $src_volid = &$real_volume_id($param->{storage}, $param->{volume});

nit: would be nice to change this to `$real_volume_id->(...)`

> -	my $dst_volid = &$real_volume_id($param->{storage}, $param->{target});
> +	my $dst_storeid = $param->{'target-storage'};
> +	my ($src_storeid, $volname) = PVE::Storage::parse_volume_id($src_volid);
> +	my $src_node = PVE::INotify::nodename();
> +	my $dst_node = $param->{'target-node'} || $src_node;
> +	my $delete = $param->{delete};
>   
> -	print "DEBUG: COPY $src_volid TO $dst_volid\n";
> +	die "source and target cannot be the same\n"
> +	    if $src_node eq $dst_node && $src_storeid eq $dst_storeid;
>   
>   	my $cfg = PVE::Storage::config();
>   
> -	# do all parameter checks first
> -
> -	# then do all short running task (to raise errors before we go to background)
> +	my ($vtype) = PVE::Storage::parse_volname($cfg, $src_volid);
> +	die "use pct move-volume or qm disk move" if $vtype eq 'images' || $vtype eq 'rootdir';
>   
> -	# then start the worker task
> -	my $worker = sub  {
> -	    my $upid = shift;
> -
> -	    print "DEBUG: starting worker $upid\n";
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
>   
> -	    my ($target_sid, $target_volname) = PVE::Storage::parse_volume_id($dst_volid);
> -	    #my $target_ip = PVE::Cluster::remote_node_ip($target_node);
> +	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $src_volid);
>   
> -	    # you need to get this working (fails currently, because storage_migrate() uses
> -	    # ssh to connect to local host (which is not needed
> -	    my $sshinfo = PVE::SSHInfo::get_ssh_info($target_node);
> -	    PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $target_sid, {'target_volname' => $target_volname});
> +	if ($delete) {
> +	    $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
> +	} else {
> +	    $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.AllocateSpace"]);
> +	}
>   
> -	    print "DEBUG: end worker $upid\n";
> +	my $worker = sub  {
> +	    if ($src_node eq $dst_node) {
> +		volume_move($cfg, $src_volid, $dst_storeid, $delete);
> +	    } else {
> +		PVE::Storage::storage_check_enabled($cfg, $dst_storeid, $dst_node);
> +		my $sshinfo = PVE::SSHInfo::get_ssh_info($dst_node);
> +		PVE::Storage::storage_migrate(
> +		    $cfg, $src_volid, $sshinfo, $dst_storeid, {'target_volname' => $volname});

I tested this scenario, where I wanted to move a debian ISO from one 
local storage to another node's storage. I patched both nodes with patch 
#1 and #2, and

```
pvesh create 
/nodes/node1/storage/local/content/local:iso/debian-12.7.0-amd64-netinst.iso 
--target-storage local --target-node node2
```

copied the ISO successfully from node1 to node2, even though with the 
following 'warning':

```
Use of uninitialized value $format in string eq at 
/usr/share/perl5/PVE/Storage/Plugin.pm line 1740.
```

Which happens in `PVE::Storage::Plugin::volume_import_formats`, where 
the format is taken from `PVE::Storage::Plugin::parse_volname`, but 
there is no format returned for ISOs, VM templates, rootdirs, backups 
and snippets as far as I can tell.

---

This could just be because of the rapid deletion and moving of volumes 
while testing, but sometimes the following command

```
pvesh create 
/nodes/node1/storage/local/content/iso/debian-12.7.0-amd64-netinst.iso 
--target-storage local --target-node node2
```

will fail with the following output:

```
Use of uninitialized value $format in string eq at 
/usr/share/perl5/PVE/Storage/Plugin.pm line 1740.
command 'set -o pipefail && pvesm export 
local:iso/debian-12.7.0-amd64-netinst.iso raw+size - -with-snapshots 0 | 
/usr/bin/ssh -e none -o 'BatchMode=yes' -o 'HostKeyAlias=node2' -o 
'UserKnownHostsFile=/etc/pve/nodes/node2/ssh_known_hosts' -o 
'GlobalKnownHostsFile=none' root@192.168.xxx.xxx -- pvesm import 
local:iso/debian-12.7.0-amd64-netinst.iso raw+size - -with-snapshots 0 
-allow-rename 0' failed: exit code 2
```

I first suspected that it was the missing "local:" before the source 
volume, but this is handled by `PVE::API2::Storage::real_volume_id(...)` 
as expected. I think it could be just a case where the the file is still 
being deleted on node2's local storage, even though `pvesm free` already 
has finished.

---

I tested the above for other local <-> cephfs cases, which also worked 
correctly for ISOs, VM templates and snippets + with the `--delete` 
option set. It also worked correctly for me when I moved those from 
node2 to node1, when issuing the command from node1.

But for all those cases, it consistently had the warning I mentioned 
above and the "race condition" where the content wasn't yet completely 
freed and so couldn't be moved to the other storage with 
`PVE::Storage::storage_migrate`.

---

One last thing: I just tested moving a backup from local to CephFS, 
before that is implemented in patch #3 (and before applying). When I 
move that between storages on the same node it will die as expected with 
the message:

```
moving volume of type 'backup' not implemented
```

but when I do the same between two different nodes, than it will not 
work as expected, but will try so and die with:

```
$ pvesh create 
/nodes/node1/storage/local/content/local:backup/vzdump-qemu-100-2024_09_17-15_06_38.vma 
--target-storage cephfs --target-node node2

Use of uninitialized value $format in string eq at 
/usr/share/perl5/PVE/Storage/Plugin.pm line 1740.
command 'set -o pipefail && pvesm export 
local:backup/vzdump-qemu-100-2024_09_17-15_06_38.vma raw+size - 
-with-snapshots 0 | /usr/bin/ssh -e none -o 'BatchMode=yes' -o 
'HostKeyAlias=node2' -o 
'UserKnownHostsFile=/etc/pve/nodes/node2/ssh_known_hosts' -o 
'GlobalKnownHostsFile=none' root@192.168.xxx.xxx -- pvesm import 
cephfs:backup/vzdump-qemu-100-2024_09_17-15_06_38.vma raw+size - 
-with-snapshots 0 -allow-rename 0' failed: exit code 255
```

> +		if ($delete) {
> +		    my $src_path = PVE::Storage::abs_filesystem_path($cfg, $src_volid);
> +		    PVE::Storage::archive_remove($src_path, 1);
> +		}
> +	    }
>   
> +	    print "Moved volume '$src_volid' on node '$src_node'"
> +		." to '$dst_storeid' on node '$dst_node'\n";

nit: This could be less verbose for callers that don't specify the 
node/target-node.

---

Otherwise, and as far as I can tell, this looks good to me. The tests 
that I've done are in the first annotation right below the commit message.

Tested-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Daniel Kral <d.kral@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel