From: Fiona Ebner <f.ebner@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Filip Schauer <f.schauer@proxmox.com>
Subject: Re: [pve-devel] [PATCH storage v6 2/7] api: content: implement moving a volume between storages
Date: Thu, 13 Feb 2025 18:21:01 +0100 [thread overview]
Message-ID: <ac7d78a7-8570-4f32-aca6-4b970289851f@proxmox.com> (raw)
In-Reply-To: <20250120112842.36450-3-f.schauer@proxmox.com>
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
next prev parent reply other threads:[~2025-02-13 17:21 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-20 11:28 [pve-devel] [PATCH storage v6 0/7] support moving volumes " 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-02-13 17:21 ` Fiona Ebner
2025-02-14 9:50 ` Fiona Ebner
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 [this message]
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
2025-01-20 11:28 ` [pve-devel] [PATCH storage v6 4/7] storage: introduce decompress_archive_into_pipe helper 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
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
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
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ac7d78a7-8570-4f32-aca6-4b970289851f@proxmox.com \
--to=f.ebner@proxmox.com \
--cc=f.schauer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal