[pve-devel] [PATCH v4 storage 2/6] api: content: implement moving a volume between storages
Daniel Kral
d.kral at proxmox.com
Fri Sep 20 16:27:26 CEST 2024
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 at 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 at 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 at 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 at proxmox.com>
Reviewed-by: Daniel Kral <d.kral at proxmox.com>
More information about the pve-devel
mailing list