[pve-devel] [PATCH storage v6 2/7] api: content: implement moving a volume between storages

[Fiona Ebner]

Am 20.01.25 um 12:28 schrieb Filip Schauer:
> Add the ability to move an iso, snippet or vztmpl between storages and
> nodes.
> 
> Use either 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"
> ```
> 
> Or use pvesh:
> 
> ```
> pvesh create /nodes/$SOURCENODE/storage/$SOURCESTORAGE/content/$SOURCEVOLUME \
>     --target-storage $TARGETSTORAGE --target-node $TARGETNODE
> ```

I'd not put this in the commit message, because it is nothing notable.
It's the same for all API calls. And also not recommended for non-test
environments with the "--insecure". You could still put it below the
"---" of course.

> 
> Signed-off-by: Filip Schauer <f.schauer at proxmox.com>
> ---
>  src/PVE/API2/Storage/Content.pm | 111 +++++++++++++++++++++-----------
>  1 file changed, 75 insertions(+), 36 deletions(-)
> 
> diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
> index fe0ad4a..ac451dc 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);

Are these imports still used or are they left-over from a previous version?

>  
>  use PVE::SafeSyslog;
>  use PVE::Cluster;
> @@ -484,29 +487,47 @@ __PACKAGE__->register_method ({
>      }});
>  
>  __PACKAGE__->register_method ({
> -    name => 'copy',
> +    name => 'move',

I'd not rename it, because the default is "--delete 0".

>      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.",

Style nit:
Usually multi-line strings are wrapped with the dot and space in front
of subsequent lines in our code base. I know I deviated from that a few
times in the past until I wrote a section about it to remember :P
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Strings

Nit: the last line should be added by the patch adding support for backups

> +	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,
> +		completion => \&PVE::Storage::complete_storage_enabled,
> +	    }),
>  	    volume => {
>  		description => "Source volume identifier",
>  		type => 'string',
> +		completion => \&PVE::Storage::complete_volume,
>  	    },
> -	    target => {
> -		description => "Target volume identifier",
> -		type => 'string',
> -	    },
> -	    target_node => get_standard_option('pve-node',  {
> +	    'target-storage' => get_standard_option('pve-storage-id', {
> +		description => "Target storage",
> +		completion => \&PVE::Storage::complete_storage_enabled,
> +	    }),
> +	    '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.",

Style nit: string wrapping

> +		optional => 1,
> +		default => 0,
> +	    },
>  	},
>      },
>      returns => {
> @@ -515,43 +536,61 @@ __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});
> +	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};
>  
> -	my $src_volid = &$real_volume_id($param->{storage}, $param->{volume});
> -	my $dst_volid = &$real_volume_id($param->{storage}, $param->{target});
> -
> -	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
> +	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';
> +	die "moving volume of type '$vtype' not implemented\n"
> +	    if (!grep { $vtype eq $_ } qw(import iso snippets vztmpl));

Style nit: parentheses with post-if

>  
> -	# then do all short running task (to raise errors before we go to background)
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $user = $rpcenv->get_user();
>  
> -	# then start the worker task
> -	my $worker = sub  {
> -	    my $upid = shift;
> +	PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $src_volid);
>  
> -	    print "DEBUG: starting worker $upid\n";
> +	if ($delete) {
> +	    $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
> +	} else {
> +	    $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.AllocateSpace"]);

This check should not be in the else branch but unconditional

> +	}
>  
> -	    my ($target_sid, $target_volname) = PVE::Storage::parse_volume_id($dst_volid);
> -	    #my $target_ip = PVE::Cluster::remote_node_ip($target_node);
> +	my $worker = sub {
> +	    PVE::Storage::storage_check_enabled($cfg, $dst_storeid, $dst_node);

Don't we need to also check the source storage?
What about activating the source volume?

> +	    my $sshinfo;
> +
> +	    if ($src_node eq $dst_node) {
> +		$sshinfo = {
> +		    ip => "localhost",
> +		    name => $dst_node,
> +		};

See my comment for patch 7/7

> +	    } else {
> +		$sshinfo = PVE::SSHInfo::get_ssh_info($dst_node);
> +	    }
>  
> -	    # 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});
> +	    my $opts = { 'target_volname' => $volname };
> +	    PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $dst_storeid, $opts);
>  
> -	    print "DEBUG: end worker $upid\n";
> +	    if ($delete) {
> +		my $src_path = PVE::Storage::abs_filesystem_path($cfg, $src_volid);
> +		PVE::Storage::archive_remove($src_path, 1);

That function doesn't have two arguments.

I'd align the behavior with what the DELETE endpoint does, i.e. turn the
implementation of the worker sub there into a helper and call that here
and there.

archive_remove() was introduced as a helper for pruning on directories
and is not generally correct for other storages or content types.

> +	    }
>  
> +	    if ($src_node eq $dst_node) {
> +		print "Moved volume '$src_volid' to '$dst_storeid'\n";
> +	    } else {
> +		print "Moved volume '$src_volid' on node '$src_node'"
> +		    ." to '$dst_storeid' on node '$dst_node'\n";
> +	    }
>  	};
>  
>  	return $rpcenv->fork_worker('imgcopy', undef, $user, $worker);