* Re: [pve-devel] [PATCH v2 storage] fix #5191: api, cli: implement moving a volume between storages
2024-06-25 14:53 [pve-devel] [PATCH v2 storage] fix #5191: api, cli: implement moving a volume between storages Filip Schauer
@ 2024-06-26 9:58 ` Fabian Grünbichler
2024-07-03 13:01 ` Filip Schauer
0 siblings, 1 reply; 3+ messages in thread
From: Fabian Grünbichler @ 2024-06-26 9:58 UTC (permalink / raw)
To: Proxmox VE development discussion
On June 25, 2024 4:53 pm, 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.
>
> The method can be called from the PVE shell with `pvesm move-volume`:
>
> ```
> pvesm move-volume <source volume> <target storage> [--target-node <node>] [--delete]
> ```
>
> For example to move a VMA backup to a Proxmox Backup Server:
>
> ```
> pvesm move-volume \
> local:backup/vzdump-qemu-100-2024_06_25-13_08_56.vma.zst pbs
> ```
>
> Or move a container template to another node and delete the source:
>
> ```
> pvesm move-volume \
> local:vztmpl/devuan-4.0-standard_4.0_amd64.tar.gz local \
> --target-node pvenode2 --delete
> ```
>
> Or use curl to call the API method:
>
> ```
> curl https://$APINODE:8006/api2/json/nodes/$SOURCENODE/storage/$SOURCESTORAGE/content/$SOURCEVOLUME \
> --insecure --cookie "$(<cookie)" -H "$(<csrftoken)" -X POST \
> --data-raw "target-storage=$TARGETSTORAGE&target-node=$TARGETNODE"
> ```
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
> This patch depends on
> [PATCH backup-qemu/vma-to-pbs 0/2] add support for notes and logs
> https://lists.proxmox.com/pipermail/pbs-devel/2024-May/009445.html
nit: it also needs a new dependency on proxmox-vma-to-pbs in d/control!
>
> 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
>
> src/PVE/API2/Storage/Content.pm | 69 +++++-----
> src/PVE/CLI/pvesm.pm | 60 +++++++++
> src/PVE/Storage.pm | 222 ++++++++++++++++++++++++++++----
> src/PVE/Storage/Plugin.pm | 105 +++++++++++----
> 4 files changed, 367 insertions(+), 89 deletions(-)
>
> diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
> index fe0ad4a..e4bf0cb 100644
> --- a/src/PVE/API2/Storage/Content.pm
> +++ b/src/PVE/API2/Storage/Content.pm
> @@ -484,10 +484,10 @@ __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.",
> protected => 1,
> proxyto => 'node',
> parameters => {
> @@ -499,14 +499,21 @@ __PACKAGE__->register_method ({
> description => "Source volume identifier",
> type => 'string',
> },
> - target => {
> - description => "Target volume identifier",
> + 'target-storage' => {
> + description => "Target storage",
> type => 'string',
> },
> - target_node => get_standard_option('pve-node', {
> + 'target-node' => get_standard_option('pve-node', {
> description => "Target node. Default is local node.",
> optional => 1,
> }),
> + delete => {
> + type => 'boolean',
> + description => "Delete the original volume after a successful copy. " .
> + "By default the original is kept.",
> + optional => 1,
> + default => 0,
> + },
> },
> },
this is missing `permissions => ..`, so it's root-only. is that
intentional?
> returns => {
> @@ -515,43 +522,33 @@ __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});
> -
> - print "DEBUG: COPY $src_volid TO $dst_volid\n";
> + my $dst_storeid = $param->{'target-storage'};
> + my ($src_storeid, $src_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 $cfg = PVE::Storage::config();
> + my $rpcenv = PVE::RPCEnvironment::get();
> + my $user = $rpcenv->get_user();
>
> - # do all parameter checks first
> -
> - # then do all short running task (to raise errors before we go to background)
> + $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]);
> + $rpcenv->check($user, "/storage/$dst_storeid", ["Datastore.Allocate"]);
should these be similar to the ones for freeing arbitrary volumes? i.e.,
at least reduce the required privileges for backup volumes?
should this be limited to non-vdisk volumes? those should probably never
be moved on the storage layer, but always via the guest-specific
interfaces.. right now, there is only a check in the volume_move case
inside the worker..
>
> - # then start the worker task
> my $worker = sub {
> - my $upid = shift;
> -
> - print "DEBUG: starting worker $upid\n";
> -
> - my ($target_sid, $target_volname) = PVE::Storage::parse_volume_id($dst_volid);
> - #my $target_ip = PVE::Cluster::remote_node_ip($target_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});
> -
> - print "DEBUG: end worker $upid\n";
> -
> + if ($src_node eq $dst_node) {
> + PVE::Storage::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' => $src_volname});
> + if ($delete) {
> + my $src_path = PVE::Storage::abs_filesystem_path($cfg, $src_volid);
> + PVE::Storage::archive_remove($src_path, 1);
see comment further below, but I think we might want to not ignore
protection but instead check for it if `delete` is given before doing
the copy, and abort if both are set..
> + }
> + }
> };
>
> return $rpcenv->fork_worker('imgcopy', undef, $user, $worker);
> diff --git a/src/PVE/CLI/pvesm.pm b/src/PVE/CLI/pvesm.pm
> index 9b9676b..f1be5fa 100755
> --- a/src/PVE/CLI/pvesm.pm
> +++ b/src/PVE/CLI/pvesm.pm
> @@ -480,6 +480,64 @@ __PACKAGE__->register_method ({
> }
> });
>
> +__PACKAGE__->register_method ({
> + name => 'movevolume',
> + path => 'movevolume',
why do we need this? can't it just forward to the API handler in the
command definition like other commands? the completion can be done in
the API schema for the API endpoint..
> + method => 'POST',
> + description => "Move a volume from one storage to another",
> + protected => 1,
> + proxyto => 'node',
> + parameters => {
> + additionalProperties => 0,
> + properties => {
> + node => get_standard_option('pve-node'),
> + 'source-volume' => {
> + description => "Source volume",
> + type => 'string',
> + },
> + 'target-storage' => get_standard_option('pve-storage-id', {
> + completion => \&PVE::Storage::complete_storage_enabled,
> + description => 'Target storage',
> + }),
> + '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 => { type => 'null' },
> + code => sub {
> + my ($param) = @_;
> +
> + my $node = extract_param($param, 'node');
> + my $source_volume = extract_param($param, 'source-volume');
> + my $target_storage = extract_param($param, 'target-storage');
> + my $target_node = extract_param($param, 'target-node');
> + my $delete = extract_param($param, 'delete') ? 1 : 0;
> +
> + my (undef, $source_volname) = PVE::Storage::parse_volume_id($source_volume, 0);
> +
> + my $args = {
> + node => $node,
> + volume => $source_volume,
> + 'target-storage' => $target_storage,
> + 'target-node' => $target_node,
> + delete => $delete,
> + };
> +
> + PVE::API2::Storage::Content->move($args);
> +
> + return;
> + },
> +});
> +
> __PACKAGE__->register_method ({
> name => 'prunebackups',
> path => 'prunebackups',
> @@ -690,6 +748,8 @@ our $cmddef = {
> print "APIVER $res->{apiver}\n";
> print "APIAGE $res->{apiage}\n";
> }],
> + 'move-volume' => [ __PACKAGE__, 'movevolume', ['source-volume', 'target-storage'],
> + { node => $nodename } ],
> 'prune-backups' => [ __PACKAGE__, 'prunebackups', ['storage'], { node => $nodename }, sub {
> my $res = shift;
>
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index f19a115..d52bc3f 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -9,6 +9,7 @@ use IO::File;
> use IO::Socket::IP;
> use IPC::Open3;
> use File::Basename;
> +use File::Copy qw(copy);
> use File::Path;
> use Cwd 'abs_path';
> use Socket;
> @@ -48,7 +49,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();
> @@ -1620,14 +1629,20 @@ sub archive_info {
> }
>
> sub archive_remove {
> - my ($archive_path) = @_;
> + my ($archive_path, $ignore_protected) = @_;
> +
> + my $protection_path = protection_file_path($archive_path);
>
> die "cannot remove protected archive '$archive_path'\n"
> - if -e protection_file_path($archive_path);
> + if !$ignore_protected && -e $protection_path;
>
> unlink $archive_path or $! == ENOENT or die "removing archive $archive_path failed: $!\n";
>
> archive_auxiliaries_remove($archive_path);
> +
> + if (-e $protection_path) {
> + unlink $protection_path or $! == ENOENT or log_warn("Removing protection file failed: $!");
> + }
shouldn't we rather honor protection, and either not allow moving with
deletion, or skip the deletion in that case with a warning?
> }
>
> sub archive_auxiliaries_remove {
> @@ -1680,6 +1695,45 @@ sub extract_vzdump_config_tar {
> return wantarray ? ($raw, $file) : $raw;
> }
>
> +sub decompress_archive_into_pipe {
> + my ($archive, $cmd, $outfunc) = @_;
> +
> + my $info = archive_info($archive);
> + die "archive is not compressed\n" if !$info->{compression};
> + my $decompressor = $info->{decompressor};
> + my $full_cmd = [ [@$decompressor, $archive], $cmd ];
> +
> + # lzop/zcat exits with 1 when the pipe is closed early,
> + # detect this and ignore the exit code later
> + my $broken_pipe;
> + my $errstring;
> + my $err = sub {
> + my $output = shift;
> + if (
> + $output =~ m/lzop: Broken pipe: <stdout>/
> + || $output =~ m/gzip: stdout: Broken pipe/
> + || $output =~ m/zstd: error 70 : Write error.*Broken pipe/
> + ) {
> + $broken_pipe = 1;
> + } elsif (!defined ($errstring) && $output !~ m/^\s*$/) {
> + $errstring = "failed to decompress archive: $output\n";
> + }
> + };
> +
> + my $rc = eval { run_command($full_cmd, outfunc => $outfunc, errfunc => $err, noerr => 1) };
> + my $rerr = $@;
> +
> + $broken_pipe ||= $rc == 141; # broken pipe from cmd POV
> +
> + if (!$errstring && !$broken_pipe && $rc != 0) {
> + die "$rerr\n" if $rerr;
> + die "archive decompression failed with exit code $rc\n";
> + }
> + die "$errstring\n" if $errstring;
> +
> + return;
> +}
> +
> sub extract_vzdump_config_vma {
> my ($archive, $comp) = @_;
>
> @@ -1691,30 +1745,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);
> }
> @@ -1753,6 +1784,143 @@ sub extract_vzdump_config {
> }
> }
>
> +sub volume_move {
are there plans to call this outside of the API handler from this patch?
if not, it could maybe start out as helper sub in the API handler, to
give us more flexibility w.r.t. future changes of the
interface/semantics?
PVE::Storage is our main entry point for other modules after all..
> + my ($cfg, $source_volid, $target_storeid, $delete) = @_;
> +
> + my ($source_storeid, $source_volname) = parse_volume_id($source_volid, 0);
> +
> + die "source and target storage cannot be the same\n" if ($source_storeid eq $target_storeid);
nit: shouldn't that also be checked in the API? basically there we want
a different target node or a different target storage, right?
right now, except for the vma-to-pbs case, this assumes both storages
are path-based. which is likely true at the moment (all the non-backup
vtypes handled here are only used via paths after all), but it might
make sense to make this explicit, or fall back to export+import if not?
> +
> + activate_storage($cfg, $source_storeid);
> + my $source_scfg = 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};
> +
> + activate_storage($cfg, $target_storeid);
> + my $target_scfg = storage_config($cfg, $target_storeid);
> + die "target storage '$target_storeid' does not support volumes of type '$vtype'\n"
> + if !$target_scfg->{content}->{$vtype};
> +
> + if ($vtype eq 'backup' || $vtype eq 'iso' || $vtype eq 'vztmpl' || $vtype eq 'snippets') {
> + my $target_plugin = PVE::Storage::Plugin->lookup($target_scfg->{type});
> +
> + die "moving a backup from a Proxmox Backup Server is not yet supported\n"
> + if ($vtype eq 'backup' && $source_scfg->{type} eq 'pbs');
> +
> + my $source_path = $source_plugin->filesystem_path($source_scfg, $source_volname);
this should probably be `path` or PVE::Storage::abs_filesystem_path?
filesystem_path is implementation detail inside the plugin hierarchy..
> + die "$source_path does not exist" if (!-e $source_path);
> + my $source_dirname = dirname($source_path);
> +
> + if ($vtype eq 'backup' && $target_scfg->{type} eq 'pbs') {
> + my $info = archive_info($source_path);
> + die "moving non-VMA backups to a Proxmox Backup Server is not yet supported\n"
nit: I'd remove the 'yet' here - while there shouldn't be any technical
blockers for either PBS to PBS moving, or PBS to dir-based/VMA moving,
let's not get the hopes up ;)
> + if ($info->{format} ne 'vma');
> +
> + my $repo = PVE::PBSClient::get_repository($target_scfg);
> + my $vmid = ($source_plugin->parse_volname($source_volname))[2];
> + my $fingerprint = $target_scfg->{fingerprint};
> + my $password = PVE::Storage::PBSPlugin::pbs_password_file_name(
> + $target_scfg, $target_storeid);
> + my $namespace = $target_scfg->{namespace};
> + my $keyfile = PVE::Storage::PBSPlugin::pbs_encryption_key_file_name(
> + $target_scfg, $target_storeid);
> + my $master_keyfile = PVE::Storage::PBSPlugin::pbs_master_pubkey_file_name(
> + $target_scfg, $target_storeid);
> +
> + my $comp = $info->{compression};
> + my $backup_time = $info->{ctime};
> + my $log_file_path = "$source_dirname/$info->{logfilename}";
> + my $notes_file_path = "$source_dirname/$info->{notesfilename}";
> +
> + my $vma_to_pbs_cmd = [
> + "vma-to-pbs",
> + "--repository", $repo,
> + "--vmid", $vmid,
> + "--fingerprint", $fingerprint,
> + "--password-file", $password,
> + "--backup-time", $backup_time,
> + "--compress",
> + ];
> +
> + push @$vma_to_pbs_cmd, "--ns", $namespace if $namespace;
> + push @$vma_to_pbs_cmd, "--log-file", $log_file_path if -e $log_file_path;
> + push @$vma_to_pbs_cmd, "--notes-file", $notes_file_path if -e $notes_file_path;
> + push @$vma_to_pbs_cmd, "--encrypt", "--keyfile", $keyfile if -e $keyfile;
> + push @$vma_to_pbs_cmd, "--master-keyfile", $master_keyfile if -e $master_keyfile;
> +
> + if ($comp) {
> + 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 $protection_source_path = protection_file_path($source_path);
> +
> + if (-e $protection_source_path) {
> + my $target_volid = PVE::Storage::PBSPlugin::print_volid(
> + $target_storeid, 'vm', $vmid, $backup_time);
> + my (undef, $target_volname) = parse_volume_id($target_volid, 0);
> + $target_plugin->update_volume_attribute(
> + $target_scfg, $target_storeid, $target_volname, 'protected', 1);
> + }
> + } else {
> + my $target_path = $target_plugin->filesystem_path($target_scfg, $source_volname);
same as above here as well..
> +
> + copy($source_path, $target_path) or die "failed to copy $vtype: $!";
should we maybe switch the order of operations around here, and first
copy the aux files, and then, depending on $delete, `move` or `copy` the
main file? it's possible that the target storage and the source storage
are actually sharing a file system and actually moving would be way
cheaper/faster after all.. downside is it makes the error handling a
bit more complex..
but I think we should probably use get/update_volume_attribute here for
future proofness? we don't know if every storage plugin out there has
implemented those as files after all, or whether that will be the case
in the future..
> +
> + if ($vtype eq 'backup') {
> + my $target_dirname = dirname($target_path);
> + my $info = archive_info($source_path);
> +
> + eval {
> + 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: $!";
> + }
> + }
> +
> + my $protection_source_path = protection_file_path($source_path);
> +
> + if (-e $protection_source_path) {
> + my $protection_target_path = protection_file_path($target_path);
> + copy($protection_source_path, $protection_target_path)
> + or die "copying backup protection file failed: $!";
> + }
> + };
> + if (my $err = $@) {
> + eval { unlink($target_path) };
> + warn $@ if $@;
> + for my $type (qw(log notes)) {
> + my $filename = $info->{"${type}filename"} or next;
> + my $auxiliary_target_path = "$target_dirname/$filename";
> + if (-e $auxiliary_target_path) {
> + eval { unlink($auxiliary_target_path) };
> + warn $@ if $@;
> + }
usually it's better to just keep track of the created files and unlink
them in error handling (e.g., push to $created_files, and then if an
error occurs, clean those up again). but see comment above ;)
> + }
> + die $err;
> + }
> + }
> + }
> +
> + archive_remove($source_path, 1) if $delete;
see question above for protection handling
> + } elsif ($vtype eq 'images') {
> + die "use pct move-volume or qm disk move\n";
> + } elsif ($vtype eq 'rootdir') {
> + die "cannot move OpenVZ rootdir\n";
> + }
> +
> + return;
> +}
> +
> sub prune_backups {
> my ($cfg, $storeid, $keep, $vmid, $type, $dryrun, $logfunc) = @_;
>
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index b5a54c1..176967a 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1566,6 +1566,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.
> @@ -1613,6 +1615,15 @@ sub volume_export {
> run_command(['tar', @COMMON_TAR_FLAGS, '-cf', '-', '-C', $file, '.'],
> output => '>&'.fileno($fh));
> return;
> + } elsif ($format eq 'backup+size') {
nit: vma+size would be more in line with our current naming scheme
here.. even if it contains the extra attributes, 'backup' is quite
generic..
> + 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;
> }
> }
> unsupported:
> @@ -1626,6 +1637,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 ();
> @@ -1641,7 +1656,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"
> @@ -1652,16 +1667,21 @@ sub volume_import {
> my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
> $class->parse_volname($volname);
>
> + die "cannot import OpenVZ rootdir\n" if $vtype eq 'rootdir';
> +
> # XXX: Should we bother with conversion routines at this level? This won't
> # happen without manual CLI usage, so for now we just error out...
> - die "cannot import format $format into a file of format $file_format\n"
> - if $data_format ne $file_format && !($data_format eq 'tar' && $file_format eq 'subvol');
> + if ($vtype eq 'images' && $data_format ne $file_format &&
> + !($data_format eq 'tar' && $file_format eq 'subvol')
> + ) {
> + die "cannot import format $format into a file of format $file_format\n";
> + }
>
> # Check for an existing file first since interrupting alloc_image doesn't
> # free it.
> my $file = $class->path($scfg, $volname, $storeid);
> if (-e $file) {
> - die "file '$file' already exists\n" if !$allow_rename;
> + die "file '$file' already exists\n" if !$allow_rename || $vtype ne 'images';
> warn "file '$file' already exists - importing with a different name\n";
> $name = undef;
> }
> @@ -1669,29 +1689,58 @@ sub volume_import {
> my ($size) = read_common_header($fh);
> $size = int($size/1024);
>
> - eval {
> - my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size);
> - my $oldname = $volname;
> - $volname = $allocname;
> - if (defined($name) && $allocname ne $oldname) {
> - die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
> + if ($vtype eq 'images') {
> + eval {
> + my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size);
> + my $oldname = $volname;
> + $volname = $allocname;
> + if (defined($name) && $allocname ne $oldname) {
> + die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
> + }
> + my $file = $class->path($scfg, $volname, $storeid)
> + or die "internal error: failed to get path to newly allocated volume $volname\n";
> + if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') {
> + run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'],
> + input => '<&'.fileno($fh));
> + } elsif ($data_format eq 'tar') {
> + run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'],
> + input => '<&'.fileno($fh));
> + } else {
> + die "volume import format '$format' not available for $class";
> + }
> + };
> + if (my $err = $@) {
> + eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) };
> + warn $@ if $@;
> + die $err;
> }
> - my $file = $class->path($scfg, $volname, $storeid)
> - or die "internal error: failed to get path to newly allocated volume $volname\n";
> - if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') {
> - run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'],
> - input => '<&'.fileno($fh));
> - } elsif ($data_format eq 'tar') {
> - run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'],
> - input => '<&'.fileno($fh));
> - } else {
> - die "volume import format '$format' not available for $class";
> + } elsif ($vtype eq 'iso' || $vtype eq 'snippets' || $vtype eq 'vztmpl' || $vtype eq 'backup') {
> + my $protected;
> + my $notes;
> +
> + if ($vtype eq 'backup') {
> + sysread($fh, $protected, 1);
> + $protected = unpack('C', $protected);
> + sysread($fh, my $notes_len, 8);
> + $notes_len = unpack('Q<', $notes_len);
> + sysread($fh, $notes, $notes_len);
> + }
> +
> + eval {
> + run_command(['dd', "of=$file", 'bs=64k'], input => '<&'.fileno($fh));
> + };
> + if (my $err = $@) {
> + if (-e $file) {
> + eval { unlink($file) };
> + warn $@ if $@;
> + }
> + die $err;
> + }
> +
> + if ($vtype eq 'backup') {
> + $class->update_volume_attribute($scfg, $storeid, $volname, 'protected', $protected);
> + $class->update_volume_attribute($scfg, $storeid, $volname, 'notes', $notes);
> }
> - };
> - if (my $err = $@) {
> - eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) };
> - warn $@ if $@;
> - die $err;
> }
>
> return "$storeid:$volname";
> @@ -1700,7 +1749,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 mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 3+ messages in thread