[pve-devel] [PATCH v4 storage 0/6] support moving volumes between storages

[pve-devel] [PATCH v4 storage 0/6] support moving volumes between storages

[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.

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

proxmox-vma-to-pbs needs to be bumped to 0.1.0 before applying
"support moving VMA backups to PBS"

Changes since v3:
* Split changes into multiple commits
* Remove superfluous parentheses from post-ifs
* Move vma_to_pbs branch from move_volume into its own helper inside
  PBSPlugin
* Use $! instead of $@ to retrieve unlink error in move_volume
* Also support content type 'rootdir'
* Rework permission checks on the move API method
* Fix permissions description on move API method
* Add error for unimplemented content types

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

Filip Schauer (6):
  plugin: allow volume import of iso, snippets and vztmpl
  api: content: implement moving a volume between storages
  api: content: support moving backups between path based storages
  storage: introduce decompress_archive_into_pipe helper
  support moving VMA backups to PBS
  pvesm: add a move-volume command

 debian/control                  |   1 +
 src/PVE/API2/Storage/Content.pm | 190 ++++++++++++++++++++++++++------
 src/PVE/CLI/pvesm.pm            |   2 +
 src/PVE/Storage.pm              |  74 ++++++++-----
 src/PVE/Storage/PBSPlugin.pm    |  65 +++++++++++
 src/PVE/Storage/Plugin.pm       | 107 +++++++++++++-----
 6 files changed, 354 insertions(+), 85 deletions(-)

-- 
2.39.2



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

[pve-devel] [PATCH v4 storage 1/6] plugin: allow volume import of iso, snippets and vztmpl

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 67 +++++++++++++++++++++++++--------------
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 8cc693c..57536c6 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1667,14 +1667,18 @@ sub volume_import {
 
     # 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' || $vtype eq 'rootdir') && $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' && $vtype ne 'rootdir');
 	warn "file '$file' already exists - importing with a different name\n";
 	$name = undef;
     }
@@ -1682,29 +1686,44 @@ 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' || $vtype eq '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') {
+	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 (my $err = $@) {
-	eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) };
-	warn $@ if $@;
-	die $err;
+    } else {
+	die "importing volume of type '$vtype' not implemented\n";
     }
 
     return "$storeid:$volname";
-- 
2.39.2



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

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

[Filip Schauer]

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

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',
     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 }),
 	    volume => {
 		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 +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});
-	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});
+		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);
-- 
2.39.2



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

[pve-devel] [PATCH v4 storage 3/6] api: content: support moving backups between path based storages

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/API2/Storage/Content.pm | 41 ++++++++++++++++++++++++++++++--
 src/PVE/Storage.pm              | 10 +++++++-
 src/PVE/Storage/Plugin.pm       | 42 ++++++++++++++++++++++++++++++---
 3 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index 6819eca..4a8fe33 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -506,10 +506,17 @@ sub volume_move {
 
     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');
+	if ($vtype ne 'iso' && $vtype ne 'vztmpl' && $vtype ne 'snippets' && $vtype ne 'backup');
 
     PVE::Storage::activate_storage($cfg, $source_storeid);
 
+    if ($vtype eq 'backup') {
+	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);
@@ -525,6 +532,32 @@ sub volume_move {
     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 {
@@ -601,7 +634,7 @@ __PACKAGE__->register_method ({
 
 	my $cfg = PVE::Storage::config();
 
-	my ($vtype) = PVE::Storage::parse_volname($cfg, $src_volid);
+	my ($vtype, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $src_volid);
 	die "use pct move-volume or qm disk move" if $vtype eq 'images' || $vtype eq 'rootdir';
 
 	my $rpcenv = PVE::RPCEnvironment::get();
@@ -615,6 +648,10 @@ __PACKAGE__->register_method ({
 	    $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.AllocateSpace"]);
 	}
 
+	if ($vtype eq 'backup' && $ownervm) {
+	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
+	}
+
 	my $worker = sub  {
 	    if ($src_node eq $dst_node) {
 		volume_move($cfg, $src_volid, $dst_storeid, $delete);
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 57b2038..12f7b3f 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();
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 57536c6..fc8fe40 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1579,6 +1579,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.
@@ -1627,6 +1629,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;
@@ -1639,6 +1651,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 ();
@@ -1654,7 +1670,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"
@@ -1711,7 +1727,18 @@ sub volume_import {
 	    warn $@ if $@;
 	    die $err;
 	}
-    } elsif ($vtype eq 'iso' || $vtype eq 'snippets' || $vtype eq 'vztmpl') {
+    } 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));
 	};
@@ -1722,6 +1749,11 @@ sub volume_import {
 	    }
 	    die $err;
 	}
+
+	if ($vtype eq 'backup') {
+	    $class->update_volume_attribute($scfg, $storeid, $volname, 'protected', $protected);
+	    $class->update_volume_attribute($scfg, $storeid, $volname, 'notes', $notes);
+	}
     } else {
 	die "importing volume of type '$vtype' not implemented\n";
     }
@@ -1732,7 +1764,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

[pve-devel] [PATCH v4 storage 4/6] storage: introduce decompress_archive_into_pipe helper

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/Storage.pm | 64 +++++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 12f7b3f..e5f5326 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1682,6 +1682,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) = @_;
 
@@ -1693,30 +1732,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);
     }
-- 
2.39.2



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

[pve-devel] [PATCH v4 storage 5/6] support moving VMA backups to PBS

[Filip Schauer]

Extend the move API to support moving VMA backups to a Proxmox Backup
Server.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 debian/control                  |  1 +
 src/PVE/API2/Storage/Content.pm | 86 ++++++++++++++++++---------------
 src/PVE/Storage/PBSPlugin.pm    | 65 +++++++++++++++++++++++++
 3 files changed, 112 insertions(+), 40 deletions(-)

diff --git a/debian/control b/debian/control
index d7afa98..6b43557 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.1.0),
          pve-cluster (>= 5.0-32),
          smartmontools,
          smbclient,
diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index 4a8fe33..6e38a2e 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -10,6 +10,7 @@ use File::Copy qw(copy move);
 use PVE::SafeSyslog;
 use PVE::Cluster;
 use PVE::Storage;
+use PVE::Storage::PBSPlugin;
 use PVE::INotify;
 use PVE::Exception qw(raise_param_exc);
 use PVE::RPCEnvironment;
@@ -495,7 +496,7 @@ sub volume_move {
 
     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);
+    my ($vtype, undef, $vmid) = $source_plugin->parse_volname($source_volname);
 
     die "source storage '$source_storeid' does not support volumes of type '$vtype'\n"
 	if !$source_scfg->{content}->{$vtype};
@@ -522,53 +523,58 @@ sub volume_move {
     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 ($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);
+
+    if ($vtype eq 'backup' && $target_scfg->{type} eq 'pbs') {
+	PVE::Storage::PBSPlugin::vma_to_pbs(
+	    $source_scfg, $source_volid, $target_scfg, $target_storeid);
+    } 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');
+		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 ($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) {
-	    unlink $created_file or $!{ENOENT} or warn $!;
+	    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;
 	}
-	die $err;
     }
 
     PVE::Storage::archive_remove($source_path) if $delete;
diff --git a/src/PVE/Storage/PBSPlugin.pm b/src/PVE/Storage/PBSPlugin.pm
index 0808bcc..4f8a05d 100644
--- a/src/PVE/Storage/PBSPlugin.pm
+++ b/src/PVE/Storage/PBSPlugin.pm
@@ -6,6 +6,7 @@ use strict;
 use warnings;
 
 use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
+use File::Basename;
 use IO::File;
 use JSON;
 use MIME::Base64 qw(decode_base64);
@@ -971,4 +972,68 @@ sub volume_has_feature {
     return undef;
 }
 
+sub vma_to_pbs {
+    my ($source_scfg, $source_volid, $target_scfg, $target_storeid) = @_;
+
+    my $source_plugin = PVE::Storage::Plugin->lookup($source_scfg->{type});
+    my $target_plugin = PVE::Storage::Plugin->lookup($target_scfg->{type});
+    my ($source_storeid, $source_volname) = PVE::Storage::parse_volume_id($source_volid, 0);
+    my $source_path = $source_plugin->path($source_scfg, $source_volname, $source_storeid);
+    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 $source_dirname = dirname($source_path);
+    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);
+    }
+
+    return;
+}
+
 1;
-- 
2.39.2



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

[pve-devel] [PATCH v4 storage 6/6] pvesm: add a move-volume command

[Filip Schauer]

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

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/CLI/pvesm.pm | 2 ++
 1 file changed, 2 insertions(+)

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;
 
-- 
2.39.2



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

Re: [pve-devel] [PATCH v4 storage 0/6] support moving volumes between storages

[Daniel Kral]

On 9/18/24 16:49, Filip Schauer wrote:
> 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.

Nice feature, I'd look forward to seeing vma-to-pbs integrated this way! 
I've reviewed and tested the patch series as far as I could and 
annotated the changes with my tests.

On 9/18/24 16:49, Filip Schauer wrote:
> proxmox-vma-to-pbs needs to be bumped to 0.1.0 before applying
> "support moving VMA backups to PBS"

Unfortunately, I wasn't entirely sure about what constituted the v0.1.0 
of proxmox-vma-to-pbs, therefore I haven't reviewed and/or tested that 
patch.

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-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

Re: [pve-devel] [PATCH v4 storage 1/6] plugin: allow volume import of iso, snippets and vztmpl

[Daniel Kral]

On 9/18/24 16:49, Filip Schauer wrote:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>

This would surely benefit from a more detailed description about what 
was changed, in what way and which API endpoints (i.e. `pvesm import`) 
it affects, so that other people could find and understand the context 
of the change more easily later on.

Also it would be helpful for others to know that nothing was 
functionally changed for the `$vtype eq 'images' || $vtype eq 'rootdir'` 
eval block when they're not using `git diff --ignore-all-space`, as that 
is not totally clear from the raw git diff.

> ---
>   src/PVE/Storage/Plugin.pm | 67 +++++++++++++++++++++++++--------------
>   1 file changed, 43 insertions(+), 24 deletions(-)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 8cc693c..57536c6 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1667,14 +1667,18 @@ sub volume_import {
>   
>       # 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' || $vtype eq 'rootdir') && $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' && $vtype ne 'rootdir');
>   	warn "file '$file' already exists - importing with a different name\n";
>   	$name = undef;
>       }
> @@ -1682,29 +1686,44 @@ 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' || $vtype eq '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') {
> +	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 (my $err = $@) {
> -	eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) };
> -	warn $@ if $@;
> -	die $err;
> +    } else {
> +	die "importing volume of type '$vtype' not implemented\n";
>       }
>   
>       return "$storeid:$volname";

I tested this API endpoint with importing ISO images, Cloudinit snippets 
and VM templates from local to CephFS and local to local as a copy. This 
worked as expected for all of those content types.

---

I also tested this with importing raw VM images out of curiosity, even 
though it was not part of the change, but this failed for me. I haven't 
looked yet how this is implemented, but this works:

```
pvesm import local:108/vm-108-disk-0.raw raw+size 
/var/lib/vz/images/107/vm-107-disk-0.raw
```

But something like this will not:

```
pvesm import local-lvm:vm-108-disk-0 raw+size 
/var/lib/vz/images/107/vm-107-disk-0.raw
```

as it will report that the virtual size is zero:

```
   --virtualsize may not be zero.
lvcreate 'pve/vm-108-disk-0' error:   Run `lvcreate --help' for more 
information.
```

---

Otherwise, this looks good to me as far as I can tell and the changes 
that should work, work for me in my tests.

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

Re: [pve-devel] [PATCH v4 storage 2/6] api: content: implement moving a volume between storages

[Daniel Kral]

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

Re: [pve-devel] [PATCH v4 storage 3/6] api: content: support moving backups between path based storages

[Daniel Kral]

On 9/18/24 16:49, Filip Schauer wrote:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>

The commit message would benefit that 'backup+size' as an import/export 
format was added here and where the functionality was added to (i.e. 
pvesm move, pvesm import, pvesm export).

> ---
>   src/PVE/API2/Storage/Content.pm | 41 ++++++++++++++++++++++++++++++--
>   src/PVE/Storage.pm              | 10 +++++++-
>   src/PVE/Storage/Plugin.pm       | 42 ++++++++++++++++++++++++++++++---
>   3 files changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
> index 6819eca..4a8fe33 100644
> --- a/src/PVE/API2/Storage/Content.pm
> +++ b/src/PVE/API2/Storage/Content.pm
> @@ -506,10 +506,17 @@ sub volume_move {
>   
>       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');
> +	if ($vtype ne 'iso' && $vtype ne 'vztmpl' && $vtype ne 'snippets' && $vtype ne 'backup');
>   
>       PVE::Storage::activate_storage($cfg, $source_storeid);
>   
> +    if ($vtype eq 'backup') {
> +	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);
> @@ -525,6 +532,32 @@ sub volume_move {
>       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 {
> @@ -601,7 +634,7 @@ __PACKAGE__->register_method ({
>   
>   	my $cfg = PVE::Storage::config();
>   
> -	my ($vtype) = PVE::Storage::parse_volname($cfg, $src_volid);
> +	my ($vtype, undef, $ownervm) = PVE::Storage::parse_volname($cfg, $src_volid);
>   	die "use pct move-volume or qm disk move" if $vtype eq 'images' || $vtype eq 'rootdir';
>   
>   	my $rpcenv = PVE::RPCEnvironment::get();
> @@ -615,6 +648,10 @@ __PACKAGE__->register_method ({
>   	    $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.AllocateSpace"]);
>   	}
>   
> +	if ($vtype eq 'backup' && $ownervm) {
> +	    $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> +	}
> +
>   	my $worker = sub  {
>   	    if ($src_node eq $dst_node) {
>   		volume_move($cfg, $src_volid, $dst_storeid, $delete);
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 57b2038..12f7b3f 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'
> +];

note: this is also used in `PVE::Storage::volume_export_start`, which 
seems to be only used in `PVE::StorageTunnel::storage_migrate`, but as 
far as I can tell this won't allow any unwanted behavior, because the 
export format is not user-defined.

>   
>   # load standard plugins
>   PVE::Storage::DirPlugin->register();
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 57536c6..fc8fe40 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1579,6 +1579,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.
> @@ -1627,6 +1629,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;

Great solution to transfer that information over pipes!

I tested moving non-protected and protected backups as well with various 
notes in them and they were moved correctly.

The only case that failed, which isn't supported here is when I use 
multi-byte characters inside the backup note (I used "Aݔ" for testing), 
where I'm not sure if that is checked at other points. If we want to 
support that, we could encode them beforehand, e.g.:

```
my $notes = Encode::encode('UTF-8', $class->get_volume_attribute($scfg, 
$storeid, $volname, 'notes')) // "";
```

Otherwise the migration between two nodes or the export of the backup 
volume will fail as `syswrite` doesn't allow non-encoded strings by 
default [0]. Also `length` would also only output the logical number of 
characters, but not the physical amount [1].

Otherwise, everything else worked as expected and I checked the notes 
file with `xxd` and it was exactly the same.

>   	}
>       }
>       die $err_msg;
> @@ -1639,6 +1651,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 ();
> @@ -1654,7 +1670,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"
> @@ -1711,7 +1727,18 @@ sub volume_import {
>   	    warn $@ if $@;
>   	    die $err;
>   	}
> -    } elsif ($vtype eq 'iso' || $vtype eq 'snippets' || $vtype eq 'vztmpl') {
> +    } 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));
>   	};
> @@ -1722,6 +1749,11 @@ sub volume_import {
>   	    }
>   	    die $err;
>   	}
> +
> +	if ($vtype eq 'backup') {
> +	    $class->update_volume_attribute($scfg, $storeid, $volname, 'protected', $protected);
> +	    $class->update_volume_attribute($scfg, $storeid, $volname, 'notes', $notes);
> +	}
>       } else {
>   	die "importing volume of type '$vtype' not implemented\n";
>       }
> @@ -1732,7 +1764,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);

This fixed the undefined $format warning from patch #2, if possible we 
could already fix that there so that potential bisects won't cause 
false-positives

> +
>   	if ($with_snapshots) {
>   	    return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
>   	    return ();

Otherwise, moving backups between different storages on the same node 
and between different nodes have worked flawlessly. Great work!

[0] https://perldoc.perl.org/functions/syswrite
[1] https://perldoc.perl.org/functions/length

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-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

Re: [pve-devel] [PATCH v4 storage 4/6] storage: introduce decompress_archive_into_pipe helper

[Daniel Kral]

On 9/18/24 16:49, Filip Schauer wrote:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>

Great cleanup! It would only be helpful to have a little more context 
information what has been done here. As far as I can tell, there are no 
functional changes (except that the newly introduced subroutine will die 
in case of an uncompressed archive) but just making the code more 
readable and adhere to the Perl Style Guide.

> ---
>   src/PVE/Storage.pm | 64 +++++++++++++++++++++++++++++-----------------
>   1 file changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 12f7b3f..e5f5326 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1682,6 +1682,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) = @_;
>   
> @@ -1693,30 +1732,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);
>       }

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

Re: [pve-devel] [PATCH v4 storage 6/6] pvesm: add a move-volume command

[Daniel Kral]

On 9/18/24 16:49, Filip Schauer wrote:
> 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
> ```
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>   src/PVE/CLI/pvesm.pm | 2 ++
>   1 file changed, 2 insertions(+)
> 
> 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;
>   

nit: In my opinion, this patch could be moved up at least where the 
'move-volume' API method was introduced, so that the 'move-volume' can 
be invoked in whatever 'style' people like, but this is something more 
minor.

Nevertheless, the CLI utility works just like the curl/pvesh API call as 
far as I can tell by testing this with the test cases I have pointed out 
in the previous patches.

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-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

Re: [pve-devel] [PATCH v4 storage 2/6] api: content: implement moving a volume between storages

[Fiona Ebner]

Am 18.09.24 um 16:49 schrieb Filip Schauer:
> +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);
> +

What if a volume with the same name is created after this check, but
before the move/copy? I guess you'll need to lock the storage (see e.g.
vdisk_alloc() in Storage.pm for comparison).

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


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