From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 18F6C1FF15E
for <inbox@lore.proxmox.com>; 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: <d58f826b-4007-489a-a521-fee9b533bc88@proxmox.com>
Date: Fri, 20 Sep 2024 16:27:26 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Daniel Kral <d.kral@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Filip Schauer <f.schauer@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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