* [pve-devel] [PATCH storage v6 1/7] plugin: allow volume import of iso, snippets, vztmpl and import
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Filip Schauer
@ 2025-01-20 11:28 ` Filip Schauer
2025-02-13 17:21 ` Fiona Ebner
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 2/7] api: content: implement moving a volume between storages Filip Schauer
` (7 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Filip Schauer @ 2025-01-20 11:28 UTC (permalink / raw)
To: pve-devel
Extend volume import functionality to support 'iso', 'snippets',
'vztmpl', and 'import' types, in addition to the existing support for
'images' and 'rootdir'. This is a prerequisite for the ability to move
ISOs, snippets and container templates between nodes.
Existing behavior for importing VM disks and container volumes remains
unchanged.
Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
src/PVE/Storage/Plugin.pm | 72 ++++++++++++++++++++++++++-------------
1 file changed, 48 insertions(+), 24 deletions(-)
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 65cf43f..b682362 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1738,6 +1738,8 @@ sub volume_export_formats {
my $format = ($class->parse_volname($volname))[6];
my $size = file_size_info($file, undef, $format);
+ return ('raw+size') if !defined($format);
+
if ($with_snapshots) {
return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
return ();
@@ -1766,14 +1768,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;
}
@@ -1781,29 +1787,44 @@ sub volume_import {
my ($size) = read_common_header($fh);
$size = PVE::Storage::Common::align_size_up($size, 1024) / 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 (grep { $vtype eq $_ } qw(import iso snippets 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";
@@ -1813,6 +1834,9 @@ 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];
+
+ return ('raw+size') if !defined($format);
+
if ($with_snapshots) {
return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
return ();
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH storage v6 1/7] plugin: allow volume import of iso, snippets, vztmpl and import
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 1/7] plugin: allow volume import of iso, snippets, vztmpl and import Filip Schauer
@ 2025-02-13 17:21 ` Fiona Ebner
2025-02-14 9:50 ` Fiona Ebner
0 siblings, 1 reply; 18+ messages in thread
From: Fiona Ebner @ 2025-02-13 17:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Filip Schauer
Am 20.01.25 um 12:28 schrieb Filip Schauer:
> Extend volume import functionality to support 'iso', 'snippets',
> 'vztmpl', and 'import' types, in addition to the existing support for
> 'images' and 'rootdir'. This is a prerequisite for the ability to move
> ISOs, snippets and container templates between nodes.
>
> Existing behavior for importing VM disks and container volumes remains
> unchanged.
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
> src/PVE/Storage/Plugin.pm | 72 ++++++++++++++++++++++++++-------------
> 1 file changed, 48 insertions(+), 24 deletions(-)
>
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 65cf43f..b682362 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1738,6 +1738,8 @@ sub volume_export_formats {
> my $format = ($class->parse_volname($volname))[6];
> my $size = file_size_info($file, undef, $format);
>
> + return ('raw+size') if !defined($format);
Can we rather make this explicitly check for vtype being one of the ones
we care about? That reflects more precisely what we want to test for.
I'm also thinking whether we want to explicitly include the vtype in the
new export format name to avoid potential type confusion down the line.
I.e. if a storage wants to export an iso, it should not be importable as
a snippet on the other side. We can keep the current formats for
backwards-compat, but the formats for the newly supported vtypes could
then be $vtype+meta, similar to the backup+size type you add. This would
be more explicit and less likely to be(come) exploitable. And we could
write the vtype into the header of the stream itself for extra safety.
Maybe in a way that allows for some forwards-compatibility, e.g. what if
ISO files get notes too at some point.
Just a crude outline what the stream could be from the top of my head:
number of meta-properties
for each meta-property: length of property name, property name, length
of property data, property data
length of volume, volume data
For compatibility/extensibility, the stream consumer can then handle all
meta properties it knows and skip the rest (with some informational
logging about what was skipped). Or we can add a marker to each property
whether it is required or optional and mark all important properties
that should rather lead to failure than skipping. Then outdated nodes
might not be able to receive certain streams, but we have the
flexibility to decide for each property.
I'm sorry, it would expand the scope of the series a bit more, but I do
think this will save us some headaches and give us more flexibility in
the future. And it seems to be a natural generalization of what you
already do for backup+size.
@other devs: opinions?
> +
> if ($with_snapshots) {
> return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
> return ();
> @@ -1766,14 +1768,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')
Style nit: multiline ifs are written differently:
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Post-If
> + ) {
> + 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;
> }
> @@ -1781,29 +1787,44 @@ sub volume_import {
> my ($size) = read_common_header($fh);
> $size = PVE::Storage::Common::align_size_up($size, 1024) / 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 (grep { $vtype eq $_ } qw(import iso snippets vztmpl)) {
> + eval {
> + run_command(['dd', "of=$file", 'bs=64k'], input => '<&'.fileno($fh));
Please use 'conv=excl' to avoid the race of somebody/something else
creating the file between the existence check above and here.
> + };
> + 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";
> @@ -1813,6 +1834,9 @@ 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];
> +
> + return ('raw+size') if !defined($format);
Similar here, I'd like to be more precise and also encode the vtype in
the transport format to avoid shenanigans with potential for importing
with a different vtype.
> +
> if ($with_snapshots) {
> return ($format.'+size') if ($format eq 'qcow2' || $format eq 'vmdk');
> return ();
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH storage v6 1/7] plugin: allow volume import of iso, snippets, vztmpl and import
2025-02-13 17:21 ` Fiona Ebner
@ 2025-02-14 9:50 ` Fiona Ebner
0 siblings, 0 replies; 18+ messages in thread
From: Fiona Ebner @ 2025-02-14 9:50 UTC (permalink / raw)
To: Proxmox VE development discussion, Filip Schauer
Am 13.02.25 um 18:21 schrieb Fiona Ebner:
> Am 20.01.25 um 12:28 schrieb Filip Schauer:
>> Extend volume import functionality to support 'iso', 'snippets',
>> 'vztmpl', and 'import' types, in addition to the existing support for
>> 'images' and 'rootdir'. This is a prerequisite for the ability to move
>> ISOs, snippets and container templates between nodes.
>>
>> Existing behavior for importing VM disks and container volumes remains
>> unchanged.
>>
>> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
>> ---
>> src/PVE/Storage/Plugin.pm | 72 ++++++++++++++++++++++++++-------------
>> 1 file changed, 48 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>> index 65cf43f..b682362 100644
>> --- a/src/PVE/Storage/Plugin.pm
>> +++ b/src/PVE/Storage/Plugin.pm
>> @@ -1738,6 +1738,8 @@ sub volume_export_formats {
>> my $format = ($class->parse_volname($volname))[6];
>> my $size = file_size_info($file, undef, $format);
>>
>> + return ('raw+size') if !defined($format);
>
> Can we rather make this explicitly check for vtype being one of the ones
> we care about? That reflects more precisely what we want to test for.
> I'm also thinking whether we want to explicitly include the vtype in the
> new export format name to avoid potential type confusion down the line.
> I.e. if a storage wants to export an iso, it should not be importable as
> a snippet on the other side. We can keep the current formats for
> backwards-compat, but the formats for the newly supported vtypes could
> then be $vtype+meta, similar to the backup+size type you add. This would
> be more explicit and less likely to be(come) exploitable. And we could
> write the vtype into the header of the stream itself for extra safety.
> Maybe in a way that allows for some forwards-compatibility, e.g. what if
> ISO files get notes too at some point.
>
> Just a crude outline what the stream could be from the top of my head:
> number of meta-properties
> for each meta-property: length of property name, property name, length
> of property data, property data
> length of volume, volume data
Or more easily
length of JSON with metadata, JSON with metadata
length of volume, volume data
>
> For compatibility/extensibility, the stream consumer can then handle all
> meta properties it knows and skip the rest (with some informational
> logging about what was skipped). Or we can add a marker to each property
> whether it is required or optional and mark all important properties
> that should rather lead to failure than skipping. Then outdated nodes
> might not be able to receive certain streams, but we have the
> flexibility to decide for each property.
>
> I'm sorry, it would expand the scope of the series a bit more, but I do
> think this will save us some headaches and give us more flexibility in
> the future. And it seems to be a natural generalization of what you
> already do for backup+size.
>
> @other devs: opinions?
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pve-devel] [PATCH storage v6 2/7] api: content: implement moving a volume between storages
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Filip Schauer
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 1/7] plugin: allow volume import of iso, snippets, vztmpl and import Filip Schauer
@ 2025-01-20 11:28 ` Filip Schauer
2025-02-13 17:21 ` Fiona Ebner
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 3/7] api: content: support moving backups between path based storages Filip Schauer
` (6 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Filip Schauer @ 2025-01-20 11:28 UTC (permalink / raw)
To: pve-devel
Add the ability to move an iso, snippet or vztmpl between storages and
nodes.
Use either curl to call the API method:
```
curl https://$APINODE:8006/api2/json/nodes/$SOURCENODE/storage/$SOURCESTORAGE/content/$SOURCEVOLUME \
--insecure --cookie "$(<cookie)" -H "$(<csrftoken)" -X POST \
--data-raw "target-storage=$TARGETSTORAGE&target-node=$TARGETNODE"
```
Or use pvesh:
```
pvesh create /nodes/$SOURCENODE/storage/$SOURCESTORAGE/content/$SOURCEVOLUME \
--target-storage $TARGETSTORAGE --target-node $TARGETNODE
```
Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
src/PVE/API2/Storage/Content.pm | 111 +++++++++++++++++++++-----------
1 file changed, 75 insertions(+), 36 deletions(-)
diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index fe0ad4a..ac451dc 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -2,7 +2,10 @@ package PVE::API2::Storage::Content;
use strict;
use warnings;
-use Data::Dumper;
+
+use Errno qw(ENOENT);
+use File::Basename;
+use File::Copy qw(copy move);
use PVE::SafeSyslog;
use PVE::Cluster;
@@ -484,29 +487,47 @@ __PACKAGE__->register_method ({
}});
__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,
+ completion => \&PVE::Storage::complete_storage_enabled,
+ }),
volume => {
description => "Source volume identifier",
type => 'string',
+ completion => \&PVE::Storage::complete_volume,
},
- target => {
- description => "Target volume identifier",
- type => 'string',
- },
- target_node => get_standard_option('pve-node', {
+ 'target-storage' => get_standard_option('pve-storage-id', {
+ description => "Target storage",
+ completion => \&PVE::Storage::complete_storage_enabled,
+ }),
+ 'target-node' => get_standard_option('pve-node', {
description => "Target node. Default is local node.",
optional => 1,
}),
+ delete => {
+ type => 'boolean',
+ description => "Delete the original volume after a successful copy. " .
+ "By default the original is kept.",
+ optional => 1,
+ default => 0,
+ },
},
},
returns => {
@@ -515,43 +536,61 @@ __PACKAGE__->register_method ({
code => sub {
my ($param) = @_;
- my $rpcenv = PVE::RPCEnvironment::get();
-
- my $user = $rpcenv->get_user();
-
- my $target_node = $param->{target_node} || PVE::INotify::nodename();
- # pvesh examples
- # cd /nodes/localhost/storage/local/content
- # pve:/> create local:103/vm-103-disk-1.raw -target local:103/vm-103-disk-2.raw
- # pve:/> create 103/vm-103-disk-1.raw -target 103/vm-103-disk-3.raw
+ my $src_volid = $real_volume_id->($param->{storage}, $param->{volume});
+ my $dst_storeid = $param->{'target-storage'};
+ my ($src_storeid, $volname) = PVE::Storage::parse_volume_id($src_volid);
+ my $src_node = PVE::INotify::nodename();
+ my $dst_node = $param->{'target-node'} || $src_node;
+ my $delete = $param->{delete};
- my $src_volid = &$real_volume_id($param->{storage}, $param->{volume});
- my $dst_volid = &$real_volume_id($param->{storage}, $param->{target});
-
- print "DEBUG: COPY $src_volid TO $dst_volid\n";
+ die "source and target cannot be the same\n"
+ if $src_node eq $dst_node && $src_storeid eq $dst_storeid;
my $cfg = PVE::Storage::config();
- # do all parameter checks first
+ my ($vtype) = PVE::Storage::parse_volname($cfg, $src_volid);
+ die "use pct move-volume or qm disk move" if $vtype eq 'images' || $vtype eq 'rootdir';
+ die "moving volume of type '$vtype' not implemented\n"
+ if (!grep { $vtype eq $_ } qw(import iso snippets vztmpl));
- # then do all short running task (to raise errors before we go to background)
+ my $rpcenv = PVE::RPCEnvironment::get();
+ my $user = $rpcenv->get_user();
- # then start the worker task
- my $worker = sub {
- my $upid = shift;
+ PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $src_volid);
- print "DEBUG: starting worker $upid\n";
+ if ($delete) {
+ $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
+ } else {
+ $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.AllocateSpace"]);
+ }
- my ($target_sid, $target_volname) = PVE::Storage::parse_volume_id($dst_volid);
- #my $target_ip = PVE::Cluster::remote_node_ip($target_node);
+ my $worker = sub {
+ PVE::Storage::storage_check_enabled($cfg, $dst_storeid, $dst_node);
+ my $sshinfo;
+
+ if ($src_node eq $dst_node) {
+ $sshinfo = {
+ ip => "localhost",
+ name => $dst_node,
+ };
+ } else {
+ $sshinfo = PVE::SSHInfo::get_ssh_info($dst_node);
+ }
- # you need to get this working (fails currently, because storage_migrate() uses
- # ssh to connect to local host (which is not needed
- my $sshinfo = PVE::SSHInfo::get_ssh_info($target_node);
- PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $target_sid, {'target_volname' => $target_volname});
+ my $opts = { 'target_volname' => $volname };
+ PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $dst_storeid, $opts);
- print "DEBUG: end worker $upid\n";
+ if ($delete) {
+ my $src_path = PVE::Storage::abs_filesystem_path($cfg, $src_volid);
+ PVE::Storage::archive_remove($src_path, 1);
+ }
+ if ($src_node eq $dst_node) {
+ print "Moved volume '$src_volid' to '$dst_storeid'\n";
+ } else {
+ print "Moved volume '$src_volid' on node '$src_node'"
+ ." to '$dst_storeid' on node '$dst_node'\n";
+ }
};
return $rpcenv->fork_worker('imgcopy', undef, $user, $worker);
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH storage v6 2/7] api: content: implement moving a volume between storages
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 2/7] api: content: implement moving a volume between storages Filip Schauer
@ 2025-02-13 17:21 ` Fiona Ebner
0 siblings, 0 replies; 18+ messages in thread
From: Fiona Ebner @ 2025-02-13 17:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Filip Schauer
Am 20.01.25 um 12:28 schrieb Filip Schauer:
> Add the ability to move an iso, snippet or vztmpl between storages and
> nodes.
>
> Use either curl to call the API method:
>
> ```
> curl https://$APINODE:8006/api2/json/nodes/$SOURCENODE/storage/$SOURCESTORAGE/content/$SOURCEVOLUME \
> --insecure --cookie "$(<cookie)" -H "$(<csrftoken)" -X POST \
> --data-raw "target-storage=$TARGETSTORAGE&target-node=$TARGETNODE"
> ```
>
> Or use pvesh:
>
> ```
> pvesh create /nodes/$SOURCENODE/storage/$SOURCESTORAGE/content/$SOURCEVOLUME \
> --target-storage $TARGETSTORAGE --target-node $TARGETNODE
> ```
I'd not put this in the commit message, because it is nothing notable.
It's the same for all API calls. And also not recommended for non-test
environments with the "--insecure". You could still put it below the
"---" of course.
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
> src/PVE/API2/Storage/Content.pm | 111 +++++++++++++++++++++-----------
> 1 file changed, 75 insertions(+), 36 deletions(-)
>
> diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
> index fe0ad4a..ac451dc 100644
> --- a/src/PVE/API2/Storage/Content.pm
> +++ b/src/PVE/API2/Storage/Content.pm
> @@ -2,7 +2,10 @@ package PVE::API2::Storage::Content;
>
> use strict;
> use warnings;
> -use Data::Dumper;
> +
> +use Errno qw(ENOENT);
> +use File::Basename;
> +use File::Copy qw(copy move);
Are these imports still used or are they left-over from a previous version?
>
> use PVE::SafeSyslog;
> use PVE::Cluster;
> @@ -484,29 +487,47 @@ __PACKAGE__->register_method ({
> }});
>
> __PACKAGE__->register_method ({
> - name => 'copy',
> + name => 'move',
I'd not rename it, because the default is "--delete 0".
> path => '{volume}',
> method => 'POST',
> - description => "Copy a volume. This is experimental code - do not use.",
> + description => "Move a volume.",
> + permissions => {
> + description => "If the --delete option is used, the 'Datastore.Allocate' privilege is " .
> + "required on the source storage. " .
> + "Without --delete, 'Datastore.AllocateSpace' is required on the target storage. " .
> + "When moving a backup, 'VM.Backup' is required on the VM or container.",
Style nit:
Usually multi-line strings are wrapped with the dot and space in front
of subsequent lines in our code base. I know I deviated from that a few
times in the past until I wrote a section about it to remember :P
https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Strings
Nit: the last line should be added by the patch adding support for backups
> + user => 'all',
> + },
> protected => 1,
> proxyto => 'node',
> parameters => {
> - additionalProperties => 0,
> + additionalProperties => 0,
> properties => {
> node => get_standard_option('pve-node'),
> - storage => get_standard_option('pve-storage-id', { optional => 1}),
> + storage => get_standard_option('pve-storage-id', {
> + optional => 1,
> + completion => \&PVE::Storage::complete_storage_enabled,
> + }),
> volume => {
> description => "Source volume identifier",
> type => 'string',
> + completion => \&PVE::Storage::complete_volume,
> },
> - target => {
> - description => "Target volume identifier",
> - type => 'string',
> - },
> - target_node => get_standard_option('pve-node', {
> + 'target-storage' => get_standard_option('pve-storage-id', {
> + description => "Target storage",
> + completion => \&PVE::Storage::complete_storage_enabled,
> + }),
> + 'target-node' => get_standard_option('pve-node', {
> description => "Target node. Default is local node.",
> optional => 1,
> }),
> + delete => {
> + type => 'boolean',
> + description => "Delete the original volume after a successful copy. " .
> + "By default the original is kept.",
Style nit: string wrapping
> + optional => 1,
> + default => 0,
> + },
> },
> },
> returns => {
> @@ -515,43 +536,61 @@ __PACKAGE__->register_method ({
> code => sub {
> my ($param) = @_;
>
> - my $rpcenv = PVE::RPCEnvironment::get();
> -
> - my $user = $rpcenv->get_user();
> -
> - my $target_node = $param->{target_node} || PVE::INotify::nodename();
> - # pvesh examples
> - # cd /nodes/localhost/storage/local/content
> - # pve:/> create local:103/vm-103-disk-1.raw -target local:103/vm-103-disk-2.raw
> - # pve:/> create 103/vm-103-disk-1.raw -target 103/vm-103-disk-3.raw
> + my $src_volid = $real_volume_id->($param->{storage}, $param->{volume});
> + my $dst_storeid = $param->{'target-storage'};
> + my ($src_storeid, $volname) = PVE::Storage::parse_volume_id($src_volid);
> + my $src_node = PVE::INotify::nodename();
> + my $dst_node = $param->{'target-node'} || $src_node;
> + my $delete = $param->{delete};
>
> - my $src_volid = &$real_volume_id($param->{storage}, $param->{volume});
> - my $dst_volid = &$real_volume_id($param->{storage}, $param->{target});
> -
> - print "DEBUG: COPY $src_volid TO $dst_volid\n";
> + die "source and target cannot be the same\n"
> + if $src_node eq $dst_node && $src_storeid eq $dst_storeid;
>
> my $cfg = PVE::Storage::config();
>
> - # do all parameter checks first
> + my ($vtype) = PVE::Storage::parse_volname($cfg, $src_volid);
> + die "use pct move-volume or qm disk move" if $vtype eq 'images' || $vtype eq 'rootdir';
> + die "moving volume of type '$vtype' not implemented\n"
> + if (!grep { $vtype eq $_ } qw(import iso snippets vztmpl));
Style nit: parentheses with post-if
>
> - # then do all short running task (to raise errors before we go to background)
> + my $rpcenv = PVE::RPCEnvironment::get();
> + my $user = $rpcenv->get_user();
>
> - # then start the worker task
> - my $worker = sub {
> - my $upid = shift;
> + PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $src_volid);
>
> - print "DEBUG: starting worker $upid\n";
> + if ($delete) {
> + $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
> + } else {
> + $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.AllocateSpace"]);
This check should not be in the else branch but unconditional
> + }
>
> - my ($target_sid, $target_volname) = PVE::Storage::parse_volume_id($dst_volid);
> - #my $target_ip = PVE::Cluster::remote_node_ip($target_node);
> + my $worker = sub {
> + PVE::Storage::storage_check_enabled($cfg, $dst_storeid, $dst_node);
Don't we need to also check the source storage?
What about activating the source volume?
> + my $sshinfo;
> +
> + if ($src_node eq $dst_node) {
> + $sshinfo = {
> + ip => "localhost",
> + name => $dst_node,
> + };
See my comment for patch 7/7
> + } else {
> + $sshinfo = PVE::SSHInfo::get_ssh_info($dst_node);
> + }
>
> - # you need to get this working (fails currently, because storage_migrate() uses
> - # ssh to connect to local host (which is not needed
> - my $sshinfo = PVE::SSHInfo::get_ssh_info($target_node);
> - PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $target_sid, {'target_volname' => $target_volname});
> + my $opts = { 'target_volname' => $volname };
> + PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $dst_storeid, $opts);
>
> - print "DEBUG: end worker $upid\n";
> + if ($delete) {
> + my $src_path = PVE::Storage::abs_filesystem_path($cfg, $src_volid);
> + PVE::Storage::archive_remove($src_path, 1);
That function doesn't have two arguments.
I'd align the behavior with what the DELETE endpoint does, i.e. turn the
implementation of the worker sub there into a helper and call that here
and there.
archive_remove() was introduced as a helper for pruning on directories
and is not generally correct for other storages or content types.
> + }
>
> + if ($src_node eq $dst_node) {
> + print "Moved volume '$src_volid' to '$dst_storeid'\n";
> + } else {
> + print "Moved volume '$src_volid' on node '$src_node'"
> + ." to '$dst_storeid' on node '$dst_node'\n";
> + }
> };
>
> return $rpcenv->fork_worker('imgcopy', undef, $user, $worker);
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pve-devel] [PATCH storage v6 3/7] api: content: support moving backups between path based storages
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Filip Schauer
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 1/7] plugin: allow volume import of iso, snippets, vztmpl and import Filip Schauer
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 2/7] api: content: implement moving a volume between storages Filip Schauer
@ 2025-01-20 11:28 ` Filip Schauer
2025-02-13 17:21 ` Fiona Ebner
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 4/7] storage: introduce decompress_archive_into_pipe helper Filip Schauer
` (5 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Filip Schauer @ 2025-01-20 11:28 UTC (permalink / raw)
To: pve-devel
This commit adds the "backup+size" export format. When this format is
used, the data stream starts with metadata of the backup (protected flag
& notes) followed by the contents of the backup archive.
Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
src/PVE/API2/Storage/Content.pm | 15 ++++++++++--
src/PVE/Storage.pm | 10 +++++++-
src/PVE/Storage/Plugin.pm | 42 +++++++++++++++++++++++++++++----
3 files changed, 60 insertions(+), 7 deletions(-)
diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index ac451dc..9ee3c51 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -548,10 +548,10 @@ __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';
die "moving volume of type '$vtype' not implemented\n"
- if (!grep { $vtype eq $_ } qw(import iso snippets vztmpl));
+ if (!grep { $vtype eq $_ } qw(backup import iso snippets vztmpl));
my $rpcenv = PVE::RPCEnvironment::get();
my $user = $rpcenv->get_user();
@@ -560,10 +560,21 @@ __PACKAGE__->register_method ({
if ($delete) {
$rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
+
+ if ($vtype eq 'backup') {
+ my $src_cfg = PVE::Storage::storage_config($cfg, $src_storeid);
+ my $src_plugin = PVE::Storage::Plugin->lookup($src_cfg->{type});
+ my $protected = $src_plugin->get_volume_attribute($src_cfg, $src_storeid, $volname, 'protected');
+ die "cannot delete protected backup\n" if $protected;
+ }
} else {
$rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.AllocateSpace"]);
}
+ if ($vtype eq 'backup' && $ownervm) {
+ $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
+ }
+
my $worker = sub {
PVE::Storage::storage_check_enabled($cfg, $dst_storeid, $dst_node);
my $sshinfo;
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 3b4f041..8e94979 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 b682362..f335bba 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1676,6 +1676,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.
@@ -1723,7 +1725,18 @@ sub volume_export {
die $err_msg if $file_format ne 'subvol';
write_common_header($fh, $size);
run_command(['tar', @COMMON_TAR_FLAGS, '-cf', '-', '-C', $file, '.'],
- output => '>&'.fileno($fh));
+ 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 = Encode::encode(
+ 'utf8', $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;
}
}
@@ -1738,6 +1751,9 @@ sub volume_export_formats {
my $format = ($class->parse_volname($volname))[6];
my $size = file_size_info($file, undef, $format);
+ my ($vtype) = $class->parse_volname($volname);
+
+ return ('backup+size') if $vtype eq 'backup';
return ('raw+size') if !defined($format);
if ($with_snapshots) {
@@ -1755,7 +1771,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"
@@ -1812,7 +1828,19 @@ sub volume_import {
warn $@ if $@;
die $err;
}
- } elsif (grep { $vtype eq $_ } qw(import iso snippets vztmpl)) {
+ } elsif (grep { $vtype eq $_ } qw(import iso snippets vztmpl 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);
+ $notes = Encode::decode('utf8', $notes) // "";
+ }
+
eval {
run_command(['dd', "of=$file", 'bs=64k'], input => '<&'.fileno($fh));
};
@@ -1823,6 +1851,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";
}
@@ -1833,8 +1866,9 @@ 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) {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH storage v6 3/7] api: content: support moving backups between path based storages
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 3/7] api: content: support moving backups between path based storages Filip Schauer
@ 2025-02-13 17:21 ` Fiona Ebner
0 siblings, 0 replies; 18+ messages in thread
From: Fiona Ebner @ 2025-02-13 17:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Filip Schauer
Am 20.01.25 um 12:28 schrieb Filip Schauer:
> This commit adds the "backup+size" export format. When this format is
> used, the data stream starts with metadata of the backup (protected flag
> & notes) followed by the contents of the backup archive.
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
> src/PVE/API2/Storage/Content.pm | 15 ++++++++++--
> src/PVE/Storage.pm | 10 +++++++-
> src/PVE/Storage/Plugin.pm | 42 +++++++++++++++++++++++++++++----
> 3 files changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
> index ac451dc..9ee3c51 100644
> --- a/src/PVE/API2/Storage/Content.pm
> +++ b/src/PVE/API2/Storage/Content.pm
> @@ -548,10 +548,10 @@ __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';
> die "moving volume of type '$vtype' not implemented\n"
> - if (!grep { $vtype eq $_ } qw(import iso snippets vztmpl));
> + if (!grep { $vtype eq $_ } qw(backup import iso snippets vztmpl));
>
> my $rpcenv = PVE::RPCEnvironment::get();
> my $user = $rpcenv->get_user();
> @@ -560,10 +560,21 @@ __PACKAGE__->register_method ({
>
> if ($delete) {
> $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
> +
> + if ($vtype eq 'backup') {
> + my $src_cfg = PVE::Storage::storage_config($cfg, $src_storeid);
> + my $src_plugin = PVE::Storage::Plugin->lookup($src_cfg->{type});
> + my $protected = $src_plugin->get_volume_attribute($src_cfg, $src_storeid, $volname, 'protected');
I'd prefer this to use the function from the storage module rather than
calling into the plugin itself.
> + die "cannot delete protected backup\n" if $protected;
> + }
> } else {
> $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.AllocateSpace"]);
> }
>
> + if ($vtype eq 'backup' && $ownervm) {
> + $rpcenv->check($user, "/vms/$ownervm", ['VM.Backup']);
> + }
Don't you just need to pass $ownervm to check_volume_access()? Because
having this check rules out a user with Datastore.Allocate to use the
API if they don't also have the backup permission.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pve-devel] [PATCH storage v6 4/7] storage: introduce decompress_archive_into_pipe helper
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Filip Schauer
` (2 preceding siblings ...)
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 3/7] api: content: support moving backups between path based storages Filip Schauer
@ 2025-01-20 11:28 ` Filip Schauer
2025-02-13 17:21 ` Fiona Ebner
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 5/7] support moving VMA backups to PBS Filip Schauer
` (4 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Filip Schauer @ 2025-01-20 11:28 UTC (permalink / raw)
To: pve-devel
Extract the file decompression code into its own reusable subroutine.
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 8e94979..761f612 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1707,6 +1707,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) = @_;
@@ -1718,30 +1757,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.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* [pve-devel] [PATCH storage v6 5/7] support moving VMA backups to PBS
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Filip Schauer
` (3 preceding siblings ...)
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 4/7] storage: introduce decompress_archive_into_pipe helper Filip Schauer
@ 2025-01-20 11:28 ` Filip Schauer
2025-02-13 17:20 ` Fiona Ebner
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 6/7] pvesm: add a move-volume command Filip Schauer
` (3 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Filip Schauer @ 2025-01-20 11:28 UTC (permalink / raw)
To: pve-devel
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 | 53 +++++++++++++++------------
src/PVE/Storage/PBSPlugin.pm | 65 +++++++++++++++++++++++++++++++++
3 files changed, 96 insertions(+), 23 deletions(-)
diff --git a/debian/control b/debian/control
index 4e1a046..0883777 100644
--- a/debian/control
+++ b/debian/control
@@ -46,6 +46,7 @@ Depends: bzip2,
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 9ee3c51..c0e2a4e 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;
@@ -557,12 +558,12 @@ __PACKAGE__->register_method ({
my $user = $rpcenv->get_user();
PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $src_volid);
+ my $src_cfg = PVE::Storage::storage_config($cfg, $src_storeid);
if ($delete) {
$rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
if ($vtype eq 'backup') {
- my $src_cfg = PVE::Storage::storage_config($cfg, $src_storeid);
my $src_plugin = PVE::Storage::Plugin->lookup($src_cfg->{type});
my $protected = $src_plugin->get_volume_attribute($src_cfg, $src_storeid, $volname, 'protected');
die "cannot delete protected backup\n" if $protected;
@@ -577,30 +578,36 @@ __PACKAGE__->register_method ({
my $worker = sub {
PVE::Storage::storage_check_enabled($cfg, $dst_storeid, $dst_node);
- my $sshinfo;
- if ($src_node eq $dst_node) {
- $sshinfo = {
- ip => "localhost",
- name => $dst_node,
- };
+ my $dst_cfg = PVE::Storage::storage_config($cfg, $dst_storeid);
+ if ($vtype eq 'backup' && $dst_cfg->{type} eq 'pbs') {
+ PVE::Storage::PBSPlugin::vma_to_pbs($src_cfg, $src_volid, $dst_cfg, $dst_storeid);
} else {
- $sshinfo = PVE::SSHInfo::get_ssh_info($dst_node);
- }
-
- my $opts = { 'target_volname' => $volname };
- PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $dst_storeid, $opts);
-
- if ($delete) {
- my $src_path = PVE::Storage::abs_filesystem_path($cfg, $src_volid);
- PVE::Storage::archive_remove($src_path, 1);
- }
-
- if ($src_node eq $dst_node) {
- print "Moved volume '$src_volid' to '$dst_storeid'\n";
- } else {
- print "Moved volume '$src_volid' on node '$src_node'"
- ." to '$dst_storeid' on node '$dst_node'\n";
+ my $sshinfo;
+
+ if ($src_node eq $dst_node) {
+ $sshinfo = {
+ ip => "localhost",
+ name => $dst_node,
+ };
+ } else {
+ $sshinfo = PVE::SSHInfo::get_ssh_info($dst_node);
+ }
+
+ my $opts = { 'target_volname' => $volname };
+ PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $dst_storeid, $opts);
+
+ if ($delete) {
+ my $src_path = PVE::Storage::abs_filesystem_path($cfg, $src_volid);
+ PVE::Storage::archive_remove($src_path, 1);
+ }
+
+ if ($src_node eq $dst_node) {
+ print "Moved volume '$src_volid' to '$dst_storeid'\n";
+ } else {
+ print "Moved volume '$src_volid' on node '$src_node'"
+ ." to '$dst_storeid' on node '$dst_node'\n";
+ }
}
};
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.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH storage v6 5/7] support moving VMA backups to PBS
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 5/7] support moving VMA backups to PBS Filip Schauer
@ 2025-02-13 17:20 ` Fiona Ebner
0 siblings, 0 replies; 18+ messages in thread
From: Fiona Ebner @ 2025-02-13 17:20 UTC (permalink / raw)
To: Proxmox VE development discussion, Filip Schauer
Am 20.01.25 um 12:28 schrieb 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 | 53 +++++++++++++++------------
> src/PVE/Storage/PBSPlugin.pm | 65 +++++++++++++++++++++++++++++++++
> 3 files changed, 96 insertions(+), 23 deletions(-)
>
> diff --git a/debian/control b/debian/control
> index 4e1a046..0883777 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -46,6 +46,7 @@ Depends: bzip2,
> 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 9ee3c51..c0e2a4e 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;
> @@ -557,12 +558,12 @@ __PACKAGE__->register_method ({
> my $user = $rpcenv->get_user();
>
> PVE::Storage::check_volume_access($rpcenv, $user, $cfg, undef, $src_volid);
> + my $src_cfg = PVE::Storage::storage_config($cfg, $src_storeid);
>
> if ($delete) {
> $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
>
> if ($vtype eq 'backup') {
> - my $src_cfg = PVE::Storage::storage_config($cfg, $src_storeid);
> my $src_plugin = PVE::Storage::Plugin->lookup($src_cfg->{type});
> my $protected = $src_plugin->get_volume_attribute($src_cfg, $src_storeid, $volname, 'protected');
> die "cannot delete protected backup\n" if $protected;
> @@ -577,30 +578,36 @@ __PACKAGE__->register_method ({
>
> my $worker = sub {
> PVE::Storage::storage_check_enabled($cfg, $dst_storeid, $dst_node);
> - my $sshinfo;
>
> - if ($src_node eq $dst_node) {
> - $sshinfo = {
> - ip => "localhost",
> - name => $dst_node,
> - };
> + my $dst_cfg = PVE::Storage::storage_config($cfg, $dst_storeid);
> + if ($vtype eq 'backup' && $dst_cfg->{type} eq 'pbs') {
Maybe check that the source plugin is path-based here to rule out some
other third-party backup storages early.
> + PVE::Storage::PBSPlugin::vma_to_pbs($src_cfg, $src_volid, $dst_cfg, $dst_storeid);
> } else {
> - $sshinfo = PVE::SSHInfo::get_ssh_info($dst_node);
> - }
> -
> - my $opts = { 'target_volname' => $volname };
> - PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $dst_storeid, $opts);
> -
> - if ($delete) {
> - my $src_path = PVE::Storage::abs_filesystem_path($cfg, $src_volid);
> - PVE::Storage::archive_remove($src_path, 1);
> - }
> -
> - if ($src_node eq $dst_node) {
> - print "Moved volume '$src_volid' to '$dst_storeid'\n";
> - } else {
> - print "Moved volume '$src_volid' on node '$src_node'"
> - ." to '$dst_storeid' on node '$dst_node'\n";
> + my $sshinfo;
> +
> + if ($src_node eq $dst_node) {
> + $sshinfo = {
> + ip => "localhost",
> + name => $dst_node,
> + };
> + } else {
> + $sshinfo = PVE::SSHInfo::get_ssh_info($dst_node);
> + }
> +
> + my $opts = { 'target_volname' => $volname };
> + PVE::Storage::storage_migrate($cfg, $src_volid, $sshinfo, $dst_storeid, $opts);
> +
> + if ($delete) {
> + my $src_path = PVE::Storage::abs_filesystem_path($cfg, $src_volid);
> + PVE::Storage::archive_remove($src_path, 1);
> + }
> +
> + if ($src_node eq $dst_node) {
> + print "Moved volume '$src_volid' to '$dst_storeid'\n";
> + } else {
> + print "Moved volume '$src_volid' on node '$src_node'"
> + ." to '$dst_storeid' on node '$dst_node'\n";
> + }
> }
> };
>
> 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});
I know I'm being nitpicky, but I'd kinda prefer not having to access the
source plugin here. The required parameters could be passed in.
> + 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;
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pve-devel] [PATCH storage v6 6/7] pvesm: add a move-volume command
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Filip Schauer
` (4 preceding siblings ...)
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 5/7] support moving VMA backups to PBS Filip Schauer
@ 2025-01-20 11:28 ` Filip Schauer
2025-02-13 17:21 ` Fiona Ebner
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 7/7] storage migrate: avoid ssh when moving a volume locally Filip Schauer
` (2 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Filip Schauer @ 2025-01-20 11:28 UTC (permalink / raw)
To: pve-devel
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 d308b3d..203a441 100755
--- a/src/PVE/CLI/pvesm.pm
+++ b/src/PVE/CLI/pvesm.pm
@@ -693,6 +693,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.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH storage v6 6/7] pvesm: add a move-volume command
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 6/7] pvesm: add a move-volume command Filip Schauer
@ 2025-02-13 17:21 ` Fiona Ebner
0 siblings, 0 replies; 18+ messages in thread
From: Fiona Ebner @ 2025-02-13 17:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Filip Schauer
I'd call it copy-volume, since that better fits with the default delete
behavior.
Am 20.01.25 um 12:28 schrieb 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 d308b3d..203a441 100755
> --- a/src/PVE/CLI/pvesm.pm
> +++ b/src/PVE/CLI/pvesm.pm
> @@ -693,6 +693,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;
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pve-devel] [PATCH storage v6 7/7] storage migrate: avoid ssh when moving a volume locally
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Filip Schauer
` (5 preceding siblings ...)
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 6/7] pvesm: add a move-volume command Filip Schauer
@ 2025-01-20 11:28 ` Filip Schauer
2025-02-13 17:21 ` Fiona Ebner
2025-02-13 17:23 ` [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Fiona Ebner
2025-03-11 14:25 ` Filip Schauer
8 siblings, 1 reply; 18+ messages in thread
From: Filip Schauer @ 2025-01-20 11:28 UTC (permalink / raw)
To: pve-devel
Avoid the overhead of SSH when moving a volume between storages on the
same node.
Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
src/PVE/Storage.pm | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 761f612..a2bef55 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -823,8 +823,6 @@ sub storage_migrate {
my $target_volid = "${target_storeid}:${target_volname}";
- my $target_ip = $target_sshinfo->{ip};
-
my $ssh = PVE::SSHInfo::ssh_info_to_command($target_sshinfo);
my $ssh_base = PVE::SSHInfo::ssh_info_to_command_base($target_sshinfo);
local $ENV{RSYNC_RSH} = PVE::Tools::cmd2string($ssh_base);
@@ -844,7 +842,9 @@ sub storage_migrate {
$import_fn = "tcp://$net";
}
- my $recv = [ @$ssh, '--', $volume_import_prepare->($target_volid, $format, $import_fn, $opts)->@* ];
+ my $recv = [];
+ push @$recv, (@$ssh, '--') if $target_sshinfo->{ip} ne "localhost";
+ push @$recv, ($volume_import_prepare->($target_volid, $format, $import_fn, $opts)->@*);
my $new_volid;
my $pattern = volume_imported_message(undef, 1);
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH storage v6 7/7] storage migrate: avoid ssh when moving a volume locally
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 7/7] storage migrate: avoid ssh when moving a volume locally Filip Schauer
@ 2025-02-13 17:21 ` Fiona Ebner
0 siblings, 0 replies; 18+ messages in thread
From: Fiona Ebner @ 2025-02-13 17:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Filip Schauer
Am 20.01.25 um 12:28 schrieb Filip Schauer:
> Avoid the overhead of SSH when moving a volume between storages on the
> same node.
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
> src/PVE/Storage.pm | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 761f612..a2bef55 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -823,8 +823,6 @@ sub storage_migrate {
>
> my $target_volid = "${target_storeid}:${target_volname}";
>
> - my $target_ip = $target_sshinfo->{ip};
> -
> my $ssh = PVE::SSHInfo::ssh_info_to_command($target_sshinfo);
> my $ssh_base = PVE::SSHInfo::ssh_info_to_command_base($target_sshinfo);
> local $ENV{RSYNC_RSH} = PVE::Tools::cmd2string($ssh_base);
> @@ -844,7 +842,9 @@ sub storage_migrate {
> $import_fn = "tcp://$net";
> }
>
> - my $recv = [ @$ssh, '--', $volume_import_prepare->($target_volid, $format, $import_fn, $opts)->@* ];
> + my $recv = [];
> + push @$recv, (@$ssh, '--') if $target_sshinfo->{ip} ne "localhost";
Instead of having a special value for the target IP, we could make the
$target_sshinfo parameter optional. Because right now, you still do the
lookup/command building with the PVE::SSHInfo calls above that are not
required in this case.
If you go for that, the patch is better ordered first.
> + push @$recv, ($volume_import_prepare->($target_volid, $format, $import_fn, $opts)->@*);
>
> my $new_volid;
> my $pattern = volume_imported_message(undef, 1);
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Filip Schauer
` (6 preceding siblings ...)
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 7/7] storage migrate: avoid ssh when moving a volume locally Filip Schauer
@ 2025-02-13 17:23 ` Fiona Ebner
2025-03-11 14:25 ` Filip Schauer
8 siblings, 0 replies; 18+ messages in thread
From: Fiona Ebner @ 2025-02-13 17:23 UTC (permalink / raw)
To: Proxmox VE development discussion, Filip Schauer
Am 20.01.25 um 12:28 schrieb Filip Schauer:
> Add the ability to move a backup, ISO, container template, snippet, or
> OVA/OVF 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
> currently not supported.
>
I know I left a lot of comments, but this is looking pretty good overall!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Filip Schauer
` (7 preceding siblings ...)
2025-02-13 17:23 ` [pve-devel] [PATCH storage v6 0/7] support moving volumes between storages Fiona Ebner
@ 2025-03-11 14:25 ` Filip Schauer
8 siblings, 0 replies; 18+ messages in thread
From: Filip Schauer @ 2025-03-11 14:25 UTC (permalink / raw)
To: pve-devel
Superseded by:
https://lore.proxmox.com/pve-devel/20250311142328.112538-1-f.schauer@proxmox.com/
On 20/01/2025 12:28, Filip Schauer wrote:
> Add the ability to move a backup, ISO, container template, snippet, or
> OVA/OVF 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
> currently not 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"
> ```
>
> Changes since v5:
> * Resolve merge conflicts when applying patches 1/7 & 3/7 to current
> master (e5f4af47d083).
>
> Changes since v4:
> * Remove the volume_move subroutine, instead use storage_migrate for
> moving volumes between storages on the same node
> * Avoid ssh when moving a volume between storages on the same node
> * Add command completion to move-volume parameters
> * Make the success message of move-volume less verbose for moves within
> the same node
> * utf8 encode/decode backup notes during export/import
> * Support the new "import" volume type
> * Code cleanup
> * Add descriptions to single line commit messages
>
> 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 (7):
> plugin: allow volume import of iso, snippets, vztmpl and import
> 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
> storage migrate: avoid ssh when moving a volume locally
>
> debian/control | 1 +
> src/PVE/API2/Storage/Content.pm | 131 +++++++++++++++++++++++---------
> src/PVE/CLI/pvesm.pm | 2 +
> src/PVE/Storage.pm | 80 ++++++++++++-------
> src/PVE/Storage/PBSPlugin.pm | 65 ++++++++++++++++
> src/PVE/Storage/Plugin.pm | 112 ++++++++++++++++++++-------
> 6 files changed, 299 insertions(+), 92 deletions(-)
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 18+ messages in thread