From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 18F6C1FF15E for ; Fri, 20 Sep 2024 16:27:53 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 79D993764E; Fri, 20 Sep 2024 16:28:02 +0200 (CEST) Message-ID: Date: Fri, 20 Sep 2024 16:27:26 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Daniel Kral To: Proxmox VE development discussion , Filip Schauer References: <20240918144953.130780-1-f.schauer@proxmox.com> <20240918144953.130780-3-f.schauer@proxmox.com> Content-Language: en-US In-Reply-To: <20240918144953.130780-3-f.schauer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.152 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v4 storage 2/6] api: content: implement moving a volume between storages X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 "$( --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 > --- > 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 Reviewed-by: Daniel Kral _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel