Re: [pve-devel] [PATCH v3 storage] fix #5191: api, cli: implement moving a volume between storages

[Fiona Ebner <f.ebner@proxmox.com>]

Am 03.07.24 um 14:59 schrieb Filip Schauer:
> Add the ability to move a backup, ISO, container template or snippet
> between storages and nodes via an API method. Moving a VMA backup to a
> Proxmox Backup Server requires the proxmox-vma-to-pbs package to be
> installed. Currently only VMA backups can be moved to a Proxmox Backup
> Server and moving backups from a Proxmox Backup Server is not yet
> supported. 

Can we split this? I.e. first add the "easy moves", export/import
preparations, then support moving backups without PBS, introduce the
decompress_archive_into_pipe() helper, finally support moving VMA to
PBS. API/CLI could be separate too.

> @@ -483,15 +485,173 @@ __PACKAGE__->register_method ({
>  	return $upid;
>      }});
>  
> +sub volume_move {

Should this even be a new top-level method? Or can/should we extend
export/import instead, to not only cover guest images? Because with this
top-level method we block the way for external storage plugins to
support this functionality too if they don't adhere to our assumptions.

> +    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);

Style nit: superfluous parentheses for post-if (there are other
instances of the same below)

---snip 8<---

> +
> +	if ($vtype eq 'backup' && $target_scfg->{type} eq 'pbs') {

IMHO this branch should be factored out into a helper. Either inside
PBSPlugin or inside PBSClient, not sure which one is the best fit.

---snip 8<---

> +	    if (my $err = $@) {
> +		for my $created_file (@created_files) {
> +		    eval { unlink($created_file) };
> +		    warn $@ if $@;

I think you need to check the return value for unlink. And if it failed,
use $! (don't log anything if it's already ENOENT).

> +		}
> +		die $err;
> +	    }
> +	}
> +
> +        PVE::Storage::archive_remove($source_path) if $delete;
> +    } elsif ($vtype eq 'images') {
> +	die "use pct move-volume or qm disk move\n";
> +    } elsif ($vtype eq 'rootdir') {
> +	die "cannot move OpenVZ rootdir\n";
Maybe put these on top as early exits? Then you could save one level of
indentation. And I'd also catch the case when you encounter a new
content type with something like "not implemented yet".

Note that content type 'rootdir' is nowadays used for all container
images (that's how it's used in the storage configuration and
list_images()). It's just that the default implementation of
parse_volname() (wrongly) classifies those as 'images'.

> +    }
> +
> +    return;
> +}
> +
>  __PACKAGE__->register_method ({
> -    name => 'copy',
> +    name => 'move',
>      path => '{volume}',
>      method => 'POST',
> -    description => "Copy a volume. This is experimental code - do not use.",
> +    description => "Move a volume.",
> +    permissions => {
> +	description => "You need the 'Datastore.Allocate' privilege on the storages.",

The documentation says:

Datastore.Allocate: create/modify/remove a datastore and delete volumes
Datastore.AllocateSpace: allocate space on a datastore

and the DELTE method for volumes has:

You need 'Datastore.Allocate' privilege on the storage (or
'Datastore.AllocateSpace' for backup volumes if you have VM.Backup
privilege on the VM)

I think we can use that too here. Require Datastore.AllocateSpace if not
using --delete and require Datastore.Allocate on the source if using
--delete, as well as special casing the backup case.

And writing this later, after looking at the actual checks below: the
documentation could be much more precise here ;)

> +	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}),

---snip 8<---

> +	if ($delete) {
> +	    $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);

Aha, so this is only required when using delete ;)

> +	} else {
> +	    $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Audit"]);

But Datastore.Audit does not entail permission to read the volume
contents. There is check_volume_access() for that.

> +	}
>  
> -	    my ($target_sid, $target_volname) = PVE::Storage::parse_volume_id($dst_volid);
> -	    #my $target_ip = PVE::Cluster::remote_node_ip($target_node);
> +	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'

---snip 8<---

> @@ -1661,16 +1677,21 @@ sub volume_import {
>      my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
>  	$class->parse_volname($volname);
>  
> +    die "cannot import OpenVZ rootdir\n" if $vtype eq 'rootdir';
> +

Same as above, this check should go away.

>      # XXX: Should we bother with conversion routines at this level? This won't
>      # happen without manual CLI usage, so for now we just error out...
> -    die "cannot import format $format into a file of format $file_format\n"
> -	if $data_format ne $file_format && !($data_format eq 'tar' && $file_format eq 'subvol');
> +    if ($vtype eq 'images' && $data_format ne $file_format &&
> +	!($data_format eq 'tar' && $file_format eq 'subvol')
> +    ) {

Should also test for 'rootdir'

> +	die "cannot import format $format into a file of format $file_format\n";
> +    }
>  
>      # Check for an existing file first since interrupting alloc_image doesn't
>      # free it.
>      my $file = $class->path($scfg, $volname, $storeid);
>      if (-e $file) {
> -	die "file '$file' already exists\n" if !$allow_rename;
> +	die "file '$file' already exists\n" if !$allow_rename || $vtype ne 'images';

Should also test for 'rootdir'

>  	warn "file '$file' already exists - importing with a different name\n";
>  	$name = undef;
>      }
> @@ -1678,29 +1699,58 @@ sub volume_import {
>      my ($size) = read_common_header($fh);
>      $size = int($size/1024);
>  
> -    eval {
> -	my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size);
> -	my $oldname = $volname;
> -	$volname = $allocname;
> -	if (defined($name) && $allocname ne $oldname) {
> -	    die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
> +    if ($vtype eq 'images') {

Should also test for 'rootdir'

> +	eval {
> +	    my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size);
> +	    my $oldname = $volname;
> +	    $volname = $allocname;
> +	    if (defined($name) && $allocname ne $oldname) {
> +		die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
> +	    }
> +	    my $file = $class->path($scfg, $volname, $storeid)
> +		or die "internal error: failed to get path to newly allocated volume $volname\n";
> +	    if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') {
> +		run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'],
> +		    input => '<&'.fileno($fh));
> +	    } elsif ($data_format eq 'tar') {
> +		run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'],
> +		    input => '<&'.fileno($fh));
> +	    } else {
> +		die "volume import format '$format' not available for $class";
> +	    }
> +	};
> +	if (my $err = $@) {
> +	    eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) };
> +	    warn $@ if $@;
> +	    die $err;
>  	}
> -	my $file = $class->path($scfg, $volname, $storeid)
> -	    or die "internal error: failed to get path to newly allocated volume $volname\n";
> -	if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') {
> -	    run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'],
> -	                input => '<&'.fileno($fh));
> -	} elsif ($data_format eq 'tar') {
> -	    run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'],
> -	                input => '<&'.fileno($fh));
> -	} else {
> -	    die "volume import format '$format' not available for $class";
> +    } elsif ($vtype eq 'iso' || $vtype eq 'snippets' || $vtype eq 'vztmpl' || $vtype eq 'backup') {

And please add an "else" to have a clean error (e.g. not yet
implemented) for unknown content types


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