public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH v3 storage] fix #5191: api, cli: implement moving a volume between storages
@ 2024-07-03 12:59 Filip Schauer
  2024-09-05 12:12 ` Fiona Ebner
  0 siblings, 1 reply; 3+ messages in thread
From: Filip Schauer @ 2024-07-03 12:59 UTC (permalink / raw)
  To: pve-devel

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.

The method can be called from the PVE shell with `pvesm move-volume`:

```
pvesm move-volume <source volume> <target storage> [--target-node <node>] [--delete]
```

For example to move a VMA backup to a Proxmox Backup Server:

```
pvesm move-volume \
    local:backup/vzdump-qemu-100-2024_06_25-13_08_56.vma.zst pbs
```

Or move a container template to another node and delete the source:

```
pvesm move-volume \
    local:vztmpl/devuan-4.0-standard_4.0_amd64.tar.gz local \
    --target-node pvenode2 --delete
```

Or 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"
```

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
This patch depends on
[PATCH backup-qemu/vma-to-pbs 0/2] add support for notes and logs
https://lists.proxmox.com/pipermail/pbs-devel/2024-May/009445.html

As of the time of writing this the version of proxmox-vma-to-pbs
specified in debian/control does not exist.

Changes since v2:
* Specify permissions for move method
* Add success message to move method
* Limit the move method to non-vdisk volumes
* Check that source and target are not the same in the move method
* Remove the unnecessary movevolume method from pvesm and make the
  move-volume command call the move API method directly
* Fail when trying to move a protected volume with the delete option
  enabled, instead of ignoring the protection
* Change "not yet supported" to "not supported" in messages indicating
  unimplemented features
* Process auxiliary files first when moving a volume locally on a node
* Move a volume instead of copying it when trying to move a volume
  locally on a node with the delete option enabled.
* Use the more general `path` function instead of `filesystem_path` to
  get the path of a volume
* Loosen the required privileges to move an ISO or a container template,
  or when the delete option is not set.
* Move the volume_move sub from PVE::Storage to
  PVE::API2::Storage::Content since it is only used there.
* Explicitly check that storages are path-based in volume_move,
  except when moving a vma to a Proxmox Backup Server

Changes since v1:
* Rename pvesm command to move-volume
* Add a delete option to control whether the source volume should be
  kept
* Move the API method to the POST endpoint of
  /nodes/{node}/storage/{storage}/content/{volume}, replacing the
  experimental copy method that has not been used since its introduction
  in October 2011 883eeea6.
* Implement migrating volumes between nodes

 debian/control                  |   1 +
 src/PVE/API2/Storage/Content.pm | 245 +++++++++++++++++++++++++++-----
 src/PVE/CLI/pvesm.pm            |   2 +
 src/PVE/Storage.pm              |  74 ++++++----
 src/PVE/Storage/Plugin.pm       | 106 ++++++++++----
 5 files changed, 342 insertions(+), 86 deletions(-)

diff --git a/debian/control b/debian/control
index d7afa98..d45b1f6 100644
--- a/debian/control
+++ b/debian/control
@@ -42,6 +42,7 @@ Depends: ceph-common (>= 12.2~),
          nfs-common,
          proxmox-backup-client (>= 2.1.10~),
          proxmox-backup-file-restore,
+         proxmox-vma-to-pbs (>= 0.0.2),
          pve-cluster (>= 5.0-32),
          smartmontools,
          smbclient,
diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index fe0ad4a..7d789d8 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -2,7 +2,9 @@ package PVE::API2::Storage::Content;
 
 use strict;
 use warnings;
-use Data::Dumper;
+
+use File::Basename;
+use File::Copy qw(copy move);
 
 use PVE::SafeSyslog;
 use PVE::Cluster;
@@ -483,15 +485,173 @@ __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);
+
+    PVE::Storage::activate_storage($cfg, $source_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};
+
+    PVE::Storage::activate_storage($cfg, $target_storeid);
+    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};
+
+    if ($vtype eq 'backup' || $vtype eq 'iso' || $vtype eq 'vztmpl' || $vtype eq 'snippets') {
+	my $target_plugin = PVE::Storage::Plugin->lookup($target_scfg->{type});
+
+	if ($vtype eq 'backup') {
+	    die "moving a backup from a Proxmox Backup Server is not supported\n"
+		if $source_scfg->{type} eq 'pbs';
+
+	    my $protected = $source_plugin->get_volume_attribute(
+		$source_scfg, $source_storeid, $source_volname, 'protected');
+
+	    die "cannot delete protected backup\n"
+		if ($delete && $protected);
+	}
+
+	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);
+
+	if ($vtype eq 'backup' && $target_scfg->{type} eq 'pbs') {
+	    my $info = PVE::Storage::archive_info($source_path);
+	    die "moving non-VMA backups to a Proxmox Backup Server is not supported\n"
+		if ($info->{format} ne 'vma');
+
+	    my $repo = PVE::PBSClient::get_repository($target_scfg);
+	    my $vmid = ($source_plugin->parse_volname($source_volname))[2];
+	    my $fingerprint = $target_scfg->{fingerprint};
+	    my $password = PVE::Storage::PBSPlugin::pbs_password_file_name(
+		$target_scfg, $target_storeid);
+	    my $namespace = $target_scfg->{namespace};
+	    my $keyfile = PVE::Storage::PBSPlugin::pbs_encryption_key_file_name(
+		$target_scfg, $target_storeid);
+	    my $master_keyfile = PVE::Storage::PBSPlugin::pbs_master_pubkey_file_name(
+		$target_scfg, $target_storeid);
+
+	    my $comp = $info->{compression};
+	    my $backup_time = $info->{ctime};
+	    my $log_file_path = "$source_dirname/$info->{logfilename}";
+	    my $notes_file_path = "$source_dirname/$info->{notesfilename}";
+
+	    my $vma_to_pbs_cmd = [
+		"vma-to-pbs",
+		"--repository", $repo,
+		"--vmid", $vmid,
+		"--fingerprint", $fingerprint,
+		"--password-file", $password,
+		"--backup-time", $backup_time,
+		"--compress",
+	    ];
+
+	    push @$vma_to_pbs_cmd, "--ns", $namespace if $namespace;
+	    push @$vma_to_pbs_cmd, "--log-file", $log_file_path if -e $log_file_path;
+	    push @$vma_to_pbs_cmd, "--notes-file", $notes_file_path if -e $notes_file_path;
+	    push @$vma_to_pbs_cmd, "--encrypt", "--keyfile", $keyfile if -e $keyfile;
+	    push @$vma_to_pbs_cmd, "--master-keyfile", $master_keyfile if -e $master_keyfile;
+
+	    if ($comp) {
+		PVE::Storage::decompress_archive_into_pipe($source_path, $vma_to_pbs_cmd);
+	    } else {
+		push @$vma_to_pbs_cmd, $source_path;
+		run_command($vma_to_pbs_cmd);
+	    }
+
+	    my $protected = $source_plugin->get_volume_attribute(
+		$source_scfg, $source_storeid, $source_volname, 'protected');
+
+	    if ($protected) {
+		my $target_volid = PVE::Storage::PBSPlugin::print_volid(
+		    $target_storeid, 'vm', $vmid, $backup_time);
+		my (undef, $target_volname) = PVE::Storage::parse_volume_id($target_volid, 0);
+		$target_plugin->update_volume_attribute(
+		    $target_scfg, $target_storeid, $target_volname, 'protected', 1);
+	    }
+	} else {
+	    die "expected storage '$target_storeid' to be path based\n" if !$target_scfg->{path};
+	    my $target_path = $target_plugin->path($target_scfg, $source_volname, $target_storeid);
+
+	    die "$target_path already exists" if (-e $target_path);
+
+	    my @created_files = ();
+
+	    eval {
+		if ($vtype eq 'backup') {
+		    my $target_dirname = dirname($target_path);
+		    my $info = PVE::Storage::archive_info($source_path);
+
+		    for my $type (qw(log notes)) {
+			my $filename = $info->{"${type}filename"} or next;
+			my $auxiliary_source_path = "$source_dirname/$filename";
+			my $auxiliary_target_path = "$target_dirname/$filename";
+			if (-e $auxiliary_source_path) {
+			    copy($auxiliary_source_path, $auxiliary_target_path)
+				or die "copying backup $type file failed: $!";
+			    push(@created_files, $auxiliary_target_path);
+			}
+		    }
+
+		    my $protected = $source_plugin->get_volume_attribute(
+			$source_scfg, $source_storeid, $source_volname, 'protected');
+
+		    if ($protected) {
+			my $protection_target_path = PVE::Storage::protection_file_path(
+                            $target_path);
+			$target_plugin->update_volume_attribute(
+			    $target_scfg, $target_storeid, $source_volname, 'protected', 1);
+			push(@created_files, $protection_target_path);
+		    }
+		}
+
+		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) {
+		    eval { unlink($created_file) };
+		    warn $@ if $@;
+		}
+		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";
+    }
+
+    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.",
+	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}),
@@ -499,14 +659,21 @@ __PACKAGE__->register_method ({
 		description => "Source volume identifier",
 		type => 'string',
 	    },
-	    target => {
-		description => "Target volume identifier",
+	    'target-storage' => {
+		description => "Target storage",
 		type => 'string',
 	    },
-	    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,
+	    },
 	},
     },
     returns => {
@@ -515,43 +682,51 @@ __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_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();
+	my $rpcenv = PVE::RPCEnvironment::get();
+	my $user = $rpcenv->get_user();
 
-	# do all parameter checks first
-
-	# then do all short running task (to raise errors before we go to background)
-
-	# then start the worker task
-	my $worker = sub  {
-	    my $upid = shift;
-
-	    print "DEBUG: starting worker $upid\n";
+	if ($delete) {
+	    $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
+	} else {
+	    $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Audit"]);
+	}
 
-	    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';
 
-	    # 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 ($vtype eq 'iso' || $vtype eq 'vztmpl') {
+	    $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.AllocateTemplate"]);
+	} else {
+	    $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.Allocate"]);
+	}
 
-	    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});
+		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";
 	};
 
 	return $rpcenv->fork_worker('imgcopy', undef, $user, $worker);
diff --git a/src/PVE/CLI/pvesm.pm b/src/PVE/CLI/pvesm.pm
index 9b9676b..8031fc1 100755
--- a/src/PVE/CLI/pvesm.pm
+++ b/src/PVE/CLI/pvesm.pm
@@ -690,6 +690,8 @@ our $cmddef = {
 	print "APIVER $res->{apiver}\n";
 	print "APIAGE $res->{apiage}\n";
     }],
+    'move-volume' => [ "PVE::API2::Storage::Content", 'move', ['volume', 'target-storage'],
+		{ node => $nodename } ],
     'prune-backups' => [ __PACKAGE__, 'prunebackups', ['storage'], { node => $nodename }, sub {
 	my $res = shift;
 
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 739fd6d..18319ef 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -48,7 +48,15 @@ use constant APIVER => 10;
 # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
 use constant APIAGE => 1;
 
-our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
+our $KNOWN_EXPORT_FORMATS = [
+    'raw+size',
+    'tar+size',
+    'qcow2+size',
+    'vmdk+size',
+    'zfs',
+    'btrfs',
+    'backup+size'
+];
 
 # load standard plugins
 PVE::Storage::DirPlugin->register();
@@ -1680,6 +1688,45 @@ sub extract_vzdump_config_tar {
     return wantarray ? ($raw, $file) : $raw;
 }
 
+sub decompress_archive_into_pipe {
+    my ($archive, $cmd, $outfunc) = @_;
+
+    my $info = archive_info($archive);
+    die "archive is not compressed\n" if !$info->{compression};
+    my $decompressor = $info->{decompressor};
+    my $full_cmd = [ [@$decompressor, $archive], $cmd ];
+
+    # lzop/zcat exits with 1 when the pipe is closed early,
+    # detect this and ignore the exit code later
+    my $broken_pipe;
+    my $errstring;
+    my $err = sub {
+	my $output = shift;
+	if (
+	    $output =~ m/lzop: Broken pipe: <stdout>/
+	    || $output =~ m/gzip: stdout: Broken pipe/
+	    || $output =~ m/zstd: error 70 : Write error.*Broken pipe/
+	) {
+	    $broken_pipe = 1;
+	} elsif (!defined ($errstring) && $output !~ m/^\s*$/) {
+	    $errstring = "failed to decompress archive: $output\n";
+	}
+    };
+
+    my $rc = eval { run_command($full_cmd, outfunc => $outfunc, errfunc => $err, noerr => 1) };
+    my $rerr = $@;
+
+    $broken_pipe ||= $rc == 141; # broken pipe from cmd POV
+
+    if (!$errstring && !$broken_pipe && $rc != 0) {
+	die "$rerr\n" if $rerr;
+	die "archive decompression failed with exit code $rc\n";
+    }
+    die "$errstring\n" if $errstring;
+
+    return;
+}
+
 sub extract_vzdump_config_vma {
     my ($archive, $comp) = @_;
 
@@ -1691,30 +1738,7 @@ sub extract_vzdump_config_vma {
     my $decompressor = $info->{decompressor};
 
     if ($comp) {
-	my $cmd = [ [@$decompressor, $archive], ["vma", "config", "-"] ];
-
-	# lzop/zcat exits with 1 when the pipe is closed early by vma, detect this and ignore the exit code later
-	my $broken_pipe;
-	my $errstring;
-	my $err = sub {
-	    my $output = shift;
-	    if ($output =~ m/lzop: Broken pipe: <stdout>/ || $output =~ m/gzip: stdout: Broken pipe/ || $output =~ m/zstd: error 70 : Write error.*Broken pipe/) {
-		$broken_pipe = 1;
-	    } elsif (!defined ($errstring) && $output !~ m/^\s*$/) {
-		$errstring = "Failed to extract config from VMA archive: $output\n";
-	    }
-	};
-
-	my $rc = eval { run_command($cmd, outfunc => $out, errfunc => $err, noerr => 1) };
-	my $rerr = $@;
-
-	$broken_pipe ||= $rc == 141; # broken pipe from vma POV
-
-	if (!$errstring && !$broken_pipe && $rc != 0) {
-	    die "$rerr\n" if $rerr;
-	    die "config extraction failed with exit code $rc\n";
-	}
-	die "$errstring\n" if $errstring;
+	decompress_archive_into_pipe($archive, ["vma", "config", "-"], $out);
     } else {
 	run_command(["vma", "config", $archive], outfunc => $out);
     }
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 6444390..7308f33 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1575,6 +1575,8 @@ sub prune_backups {
 #     unprivileged container. In other words, this is from the root user
 #     namespace's point of view with no uid-mapping in effect.
 #     As produced via `tar -C vm-100-disk-1.subvol -cpf TheOutputFile.dat .`
+#   backup+size: (backups only)
+#     A raw binary data stream prefixed with a protection flag and notes.
 
 # Plugins may reuse these helpers. Changes to the header format should be
 # reflected by changes to the function prototypes.
@@ -1623,6 +1625,16 @@ sub volume_export {
 	    run_command(['tar', @COMMON_TAR_FLAGS, '-cf', '-', '-C', $file, '.'],
 	                output => '>&'.fileno($fh));
 	    return;
+	} elsif ($format eq 'backup+size') {
+	    write_common_header($fh, $size);
+	    my $protected = $class->get_volume_attribute(
+		$scfg, $storeid, $volname, 'protected') ? 1 : 0;
+	    my $notes = $class->get_volume_attribute($scfg, $storeid, $volname, 'notes') // "";
+	    syswrite($fh, pack("C", $protected), 1);
+	    syswrite($fh, pack("Q<", length($notes)), 8);
+	    syswrite($fh, $notes, length($notes));
+	    run_command(['dd', "if=$file", "bs=4k", "status=progress"], output => '>&'.fileno($fh));
+	    return;
 	}
     }
     die $err_msg;
@@ -1635,6 +1647,10 @@ sub volume_export_formats {
 	    or return;
 	my ($size, $format) = file_size_info($file);
 
+	my ($vtype) = $class->parse_volname($volname);
+
+	return ('backup+size') if $vtype eq 'backup';
+
 	if ($with_snapshots) {
 	    return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
 	    return ();
@@ -1650,7 +1666,7 @@ sub volume_import {
     my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots, $allow_rename) = @_;
 
     die "volume import format '$format' not available for $class\n"
-	if $format !~ /^(raw|tar|qcow2|vmdk)\+size$/;
+	if $format !~ /^(raw|tar|qcow2|vmdk|backup)\+size$/;
     my $data_format = $1;
 
     die "format $format cannot be imported without snapshots\n"
@@ -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';
+
     # 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')
+    ) {
+	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';
 	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') {
+	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') {
+	my $protected;
+	my $notes;
+
+	if ($vtype eq 'backup') {
+	    sysread($fh, $protected, 1);
+	    $protected = unpack('C', $protected);
+	    sysread($fh, my $notes_len, 8);
+	    $notes_len = unpack('Q<', $notes_len);
+	    sysread($fh, $notes, $notes_len);
+	}
+
+	eval {
+	    run_command(['dd', "of=$file", 'bs=64k'], input => '<&'.fileno($fh));
+	};
+	if (my $err = $@) {
+	    if (-e $file) {
+		eval { unlink($file) };
+		warn $@ if $@;
+	    }
+	    die $err;
+	}
+
+	if ($vtype eq 'backup') {
+	    $class->update_volume_attribute($scfg, $storeid, $volname, 'protected', $protected);
+	    $class->update_volume_attribute($scfg, $storeid, $volname, 'notes', $notes);
 	}
-    };
-    if (my $err = $@) {
-	eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) };
-	warn $@ if $@;
-	die $err;
     }
 
     return "$storeid:$volname";
@@ -1709,7 +1759,11 @@ sub volume_import {
 sub volume_import_formats {
     my ($class, $scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots) = @_;
     if ($scfg->{path} && !defined($base_snapshot)) {
-	my $format = ($class->parse_volname($volname))[6];
+	my ($vtype, $format) = ($class->parse_volname($volname))[0, 6];
+
+	return ('backup+size') if $vtype eq 'backup';
+	return ('raw+size') if !defined($format);
+
 	if ($with_snapshots) {
 	    return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
 	    return ();
-- 
2.39.2



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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH v3 storage] fix #5191: api, cli: implement moving a volume between storages
  2024-07-03 12:59 [pve-devel] [PATCH v3 storage] fix #5191: api, cli: implement moving a volume between storages Filip Schauer
@ 2024-09-05 12:12 ` Fiona Ebner
  2024-09-18 14:56   ` Filip Schauer
  0 siblings, 1 reply; 3+ messages in thread
From: Fiona Ebner @ 2024-09-05 12:12 UTC (permalink / raw)
  To: Proxmox VE development discussion, Filip Schauer

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH v3 storage] fix #5191: api, cli: implement moving a volume between storages
  2024-09-05 12:12 ` Fiona Ebner
@ 2024-09-18 14:56   ` Filip Schauer
  0 siblings, 0 replies; 3+ messages in thread
From: Filip Schauer @ 2024-09-18 14:56 UTC (permalink / raw)
  To: Fiona Ebner, Proxmox VE development discussion

On 05/09/2024 14:12, Fiona Ebner wrote:
>> @@ -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.


We do not use the existing export/import because we want to use `move`
instead of copying when `--delete` is passed. This improves performance
when moving between storages on the same storage backend.

Other than that the feedback was incorporated in patch v4:
https://lists.proxmox.com/pipermail/pve-devel/2024-September/065380.html



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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-09-18 14:56 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-03 12:59 [pve-devel] [PATCH v3 storage] fix #5191: api, cli: implement moving a volume between storages Filip Schauer
2024-09-05 12:12 ` Fiona Ebner
2024-09-18 14:56   ` Filip Schauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal