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

[Daniel Kral <d.kral@proxmox.com>]

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