From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 3A3701FF15D for ; Thu, 5 Sep 2024 14:11:55 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3C7C23B165; Thu, 5 Sep 2024 14:12:29 +0200 (CEST) Message-ID: <276d995f-00ed-46c9-9e1b-deff755fcb5d@proxmox.com> Date: Thu, 5 Sep 2024 14:12:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Filip Schauer References: <20240703125909.168605-1-f.schauer@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20240703125909.168605-1-f.schauer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.053 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH v3 storage] fix #5191: api, cli: implement moving a volume between storages X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 03.07.24 um 14:59 schrieb Filip Schauer: > Add the ability to move a backup, ISO, container template or snippet > between storages and nodes via an API method. Moving a VMA backup to a > Proxmox Backup Server requires the proxmox-vma-to-pbs package to be > installed. Currently only VMA backups can be moved to a Proxmox Backup > Server and moving backups from a Proxmox Backup Server is not yet > supported. Can we split this? I.e. first add the "easy moves", export/import preparations, then support moving backups without PBS, introduce the decompress_archive_into_pipe() helper, finally support moving VMA to PBS. API/CLI could be separate too. > @@ -483,15 +485,173 @@ __PACKAGE__->register_method ({ > return $upid; > }}); > > +sub volume_move { Should this even be a new top-level method? Or can/should we extend export/import instead, to not only cover guest images? Because with this top-level method we block the way for external storage plugins to support this functionality too if they don't adhere to our assumptions. > + my ($cfg, $source_volid, $target_storeid, $delete) = @_; > + > + my ($source_storeid, $source_volname) = PVE::Storage::parse_volume_id($source_volid, 0); > + > + die "source and target storage cannot be the same\n" if ($source_storeid eq $target_storeid); Style nit: superfluous parentheses for post-if (there are other instances of the same below) ---snip 8<--- > + > + if ($vtype eq 'backup' && $target_scfg->{type} eq 'pbs') { IMHO this branch should be factored out into a helper. Either inside PBSPlugin or inside PBSClient, not sure which one is the best fit. ---snip 8<--- > + if (my $err = $@) { > + for my $created_file (@created_files) { > + eval { unlink($created_file) }; > + warn $@ if $@; I think you need to check the return value for unlink. And if it failed, use $! (don't log anything if it's already ENOENT). > + } > + die $err; > + } > + } > + > + PVE::Storage::archive_remove($source_path) if $delete; > + } elsif ($vtype eq 'images') { > + die "use pct move-volume or qm disk move\n"; > + } elsif ($vtype eq 'rootdir') { > + die "cannot move OpenVZ rootdir\n"; Maybe put these on top as early exits? Then you could save one level of indentation. And I'd also catch the case when you encounter a new content type with something like "not implemented yet". Note that content type 'rootdir' is nowadays used for all container images (that's how it's used in the storage configuration and list_images()). It's just that the default implementation of parse_volname() (wrongly) classifies those as 'images'. > + } > + > + return; > +} > + > __PACKAGE__->register_method ({ > - name => 'copy', > + name => 'move', > path => '{volume}', > method => 'POST', > - description => "Copy a volume. This is experimental code - do not use.", > + description => "Move a volume.", > + permissions => { > + description => "You need the 'Datastore.Allocate' privilege on the storages.", The documentation says: Datastore.Allocate: create/modify/remove a datastore and delete volumes Datastore.AllocateSpace: allocate space on a datastore and the DELTE method for volumes has: You need 'Datastore.Allocate' privilege on the storage (or 'Datastore.AllocateSpace' for backup volumes if you have VM.Backup privilege on the VM) I think we can use that too here. Require Datastore.AllocateSpace if not using --delete and require Datastore.Allocate on the source if using --delete, as well as special casing the backup case. And writing this later, after looking at the actual checks below: the documentation could be much more precise here ;) > + user => 'all', > + }, > protected => 1, > proxyto => 'node', > parameters => { > - additionalProperties => 0, > + additionalProperties => 0, > properties => { > node => get_standard_option('pve-node'), > storage => get_standard_option('pve-storage-id', { optional => 1}), ---snip 8<--- > + if ($delete) { > + $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Allocate"]); Aha, so this is only required when using delete ;) > + } else { > + $rpcenv->check($user, "/storage/$src_storeid", ["Datastore.Audit"]); But Datastore.Audit does not entail permission to read the volume contents. There is check_volume_access() for that. > + } > > - my ($target_sid, $target_volname) = PVE::Storage::parse_volume_id($dst_volid); > - #my $target_ip = PVE::Cluster::remote_node_ip($target_node); > + my ($vtype) = PVE::Storage::parse_volname($cfg, $src_volid); > + die "use pct move-volume or qm disk move" if $vtype eq 'images'; || $vtype eq 'rootdir' ---snip 8<--- > @@ -1661,16 +1677,21 @@ sub volume_import { > my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) = > $class->parse_volname($volname); > > + die "cannot import OpenVZ rootdir\n" if $vtype eq 'rootdir'; > + Same as above, this check should go away. > # XXX: Should we bother with conversion routines at this level? This won't > # happen without manual CLI usage, so for now we just error out... > - die "cannot import format $format into a file of format $file_format\n" > - if $data_format ne $file_format && !($data_format eq 'tar' && $file_format eq 'subvol'); > + if ($vtype eq 'images' && $data_format ne $file_format && > + !($data_format eq 'tar' && $file_format eq 'subvol') > + ) { Should also test for 'rootdir' > + die "cannot import format $format into a file of format $file_format\n"; > + } > > # Check for an existing file first since interrupting alloc_image doesn't > # free it. > my $file = $class->path($scfg, $volname, $storeid); > if (-e $file) { > - die "file '$file' already exists\n" if !$allow_rename; > + die "file '$file' already exists\n" if !$allow_rename || $vtype ne 'images'; Should also test for 'rootdir' > warn "file '$file' already exists - importing with a different name\n"; > $name = undef; > } > @@ -1678,29 +1699,58 @@ sub volume_import { > my ($size) = read_common_header($fh); > $size = int($size/1024); > > - eval { > - my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size); > - my $oldname = $volname; > - $volname = $allocname; > - if (defined($name) && $allocname ne $oldname) { > - die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n"; > + if ($vtype eq 'images') { Should also test for 'rootdir' > + eval { > + my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size); > + my $oldname = $volname; > + $volname = $allocname; > + if (defined($name) && $allocname ne $oldname) { > + die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n"; > + } > + my $file = $class->path($scfg, $volname, $storeid) > + or die "internal error: failed to get path to newly allocated volume $volname\n"; > + if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') { > + run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'], > + input => '<&'.fileno($fh)); > + } elsif ($data_format eq 'tar') { > + run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'], > + input => '<&'.fileno($fh)); > + } else { > + die "volume import format '$format' not available for $class"; > + } > + }; > + if (my $err = $@) { > + eval { $class->free_image($storeid, $scfg, $volname, 0, $file_format) }; > + warn $@ if $@; > + die $err; > } > - my $file = $class->path($scfg, $volname, $storeid) > - or die "internal error: failed to get path to newly allocated volume $volname\n"; > - if ($data_format eq 'raw' || $data_format eq 'qcow2' || $data_format eq 'vmdk') { > - run_command(['dd', "of=$file", 'conv=sparse', 'bs=64k'], > - input => '<&'.fileno($fh)); > - } elsif ($data_format eq 'tar') { > - run_command(['tar', @COMMON_TAR_FLAGS, '-C', $file, '-xf', '-'], > - input => '<&'.fileno($fh)); > - } else { > - die "volume import format '$format' not available for $class"; > + } elsif ($vtype eq 'iso' || $vtype eq 'snippets' || $vtype eq 'vztmpl' || $vtype eq 'backup') { And please add an "else" to have a clean error (e.g. not yet implemented) for unknown content types _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel