* [pve-devel] [PATCH v4 storage 1/9] storage: add new find_free_volname
2021-11-02 15:03 [pve-devel] [PATCH v4 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
@ 2021-11-02 15:03 ` Aaron Lauterer
2021-11-08 15:21 ` Fabian Grünbichler
2021-11-02 15:03 ` [pve-devel] [PATCH v4 storage 2/9] add disk rename feature Aaron Lauterer
` (7 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 15:03 UTC (permalink / raw)
To: pve-devel
This new method exposes the functionality to request a new, not yet
used, volname for a storage.
The default implementation will return the result from
'find_free_diskname' prefixed with "<VMID>/" if $scfg->{path} exists.
Otherwise it will only return the result from 'find_free_diskname'.
If the format suffix is added also depends on the existence of
$scfg->{path}.
$scfg->{path} will be present for directory based storage types.
Should a storage need to return different volname, it needs to override
the 'find_free_volname' method.
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
v3 -> v4:
* fix style nit
v2 -> v3:
* dropped exists() check
* fixed base image handling
* fixed smaller code style issues
v1 -> v2:
* many small fixes and improvements
* rename_volume now accepts $source_volname instead of $source_volid
helps us to avoid to parse the volid a second time
rfc -> v1:
* reduced number of parameters to minimum needed, plugins infer needed
information themselves
* added storage locking and checking if volume already exists
* parse target_volname prior to renaming to check if valid
old dedicated API endpoint -> rfc:
only do rename now but the rename function handles templates and returns
the new volid as this can be differently handled on some storages.
PVE/Storage.pm | 9 +++++++++
PVE/Storage/Plugin.pm | 8 ++++++++
2 files changed, 17 insertions(+)
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 71d6ad7..4fbeaea 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -203,6 +203,15 @@ sub storage_can_replicate {
return $plugin->storage_can_replicate($scfg, $storeid, $format);
}
+sub find_free_volname {
+ my ($cfg, $storeid, $vmid, $fmt) = @_;
+
+ my $scfg = storage_config($cfg, $storeid);
+ my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+
+ return $plugin->find_free_volname($storeid, $scfg, $vmid, $fmt);
+}
+
sub storage_ids {
my ($cfg) = @_;
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index aeb4fff..d21c874 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -732,6 +732,14 @@ sub find_free_diskname {
return get_next_vm_diskname($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix);
}
+sub find_free_volname {
+ my ($class, $storeid, $scfg, $vmid, $fmt) = @_;
+
+ my $diskname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt, $scfg->{path});
+
+ return $scfg->{path} ? "${vmid}/$diskname" : $diskname;
+}
+
sub clone_image {
my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_;
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [pve-devel] [PATCH v4 storage 1/9] storage: add new find_free_volname
2021-11-02 15:03 ` [pve-devel] [PATCH v4 storage 1/9] storage: add new find_free_volname Aaron Lauterer
@ 2021-11-08 15:21 ` Fabian Grünbichler
2021-11-08 15:53 ` Thomas Lamprecht
0 siblings, 1 reply; 15+ messages in thread
From: Fabian Grünbichler @ 2021-11-08 15:21 UTC (permalink / raw)
To: Proxmox VE development discussion
On November 2, 2021 4:03 pm, Aaron Lauterer wrote:
> This new method exposes the functionality to request a new, not yet
> used, volname for a storage.
>
> The default implementation will return the result from
> 'find_free_diskname' prefixed with "<VMID>/" if $scfg->{path} exists.
> Otherwise it will only return the result from 'find_free_diskname'.
>
> If the format suffix is added also depends on the existence of
> $scfg->{path}.
>
> $scfg->{path} will be present for directory based storage types.
>
> Should a storage need to return different volname, it needs to override
> the 'find_free_volname' method.
>
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
as discussed off-list (I am sorry I didn't read through the full
discussion on-list of all 10 versions of this and the previous series
;)) - IMHO this is not a backwards compatible addition since it's not
guarded by any feature flag, so external plugins fall into two
categories:
- derived from Plugin.pm, so automatically implement it via
find_free_diskname -> get_next_vm_diskname, which might or might not
be correct for that plugin
- not derived from Plugin.pm, don't implement it, no safeguard to handle
that
RIGHT NOW, the only callers are guarded by the rename feature so there
can't be any problems in practice - but there is no guarantee that this
will be remembered down the line, and it also breaks the
modularization/layering of the code to rely on this fact ;)
while we could argue about whether we really want to be that strict,
another point against exposing this method for me is that it's also
prone to mis-use, since it encourages code patterns like in this
series with:
get_free_volname
do_something_that_allocates_that_volname
without holding a storage lock over both calls, since the storage lock
is only available on the plugin level and not exposed in PVE::Storage
itself.
an interface like
$plugin->rename_volume($scfg, $storeid, $source, $new_owner, $new_name)
with at least $new_owner or $new_name set would also cover both cases
(reassign with just $new_owner, rename with just $new_name) and can be
feature-guarded while keeping the locking in PVE::Storage for the full
operation including finding a free slot if needed.
alternatively, just implementing
$plugin->reassign_volume($scfg, $storeid, $source, $new_owner)
for now (guarded by a 'reassign' feature), and postponing the rename
feature for a future series that handles 'custom suffix/volname'
across the board of our stack would also be an option.
> ---
>
> v3 -> v4:
> * fix style nit
>
> v2 -> v3:
> * dropped exists() check
> * fixed base image handling
> * fixed smaller code style issues
>
> v1 -> v2:
> * many small fixes and improvements
> * rename_volume now accepts $source_volname instead of $source_volid
> helps us to avoid to parse the volid a second time
>
> rfc -> v1:
> * reduced number of parameters to minimum needed, plugins infer needed
> information themselves
> * added storage locking and checking if volume already exists
> * parse target_volname prior to renaming to check if valid
>
> old dedicated API endpoint -> rfc:
> only do rename now but the rename function handles templates and returns
> the new volid as this can be differently handled on some storages.
>
> PVE/Storage.pm | 9 +++++++++
> PVE/Storage/Plugin.pm | 8 ++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 71d6ad7..4fbeaea 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -203,6 +203,15 @@ sub storage_can_replicate {
> return $plugin->storage_can_replicate($scfg, $storeid, $format);
> }
>
> +sub find_free_volname {
> + my ($cfg, $storeid, $vmid, $fmt) = @_;
> +
> + my $scfg = storage_config($cfg, $storeid);
> + my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> + return $plugin->find_free_volname($storeid, $scfg, $vmid, $fmt);
> +}
> +
> sub storage_ids {
> my ($cfg) = @_;
>
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index aeb4fff..d21c874 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -732,6 +732,14 @@ sub find_free_diskname {
> return get_next_vm_diskname($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix);
> }
>
> +sub find_free_volname {
> + my ($class, $storeid, $scfg, $vmid, $fmt) = @_;
> +
> + my $diskname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt, $scfg->{path});
> +
> + return $scfg->{path} ? "${vmid}/$diskname" : $diskname;
> +}
> +
> sub clone_image {
> my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_;
>
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [pve-devel] [PATCH v4 storage 1/9] storage: add new find_free_volname
2021-11-08 15:21 ` Fabian Grünbichler
@ 2021-11-08 15:53 ` Thomas Lamprecht
2021-11-09 14:42 ` Aaron Lauterer
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Lamprecht @ 2021-11-08 15:53 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
On 08.11.21 16:21, Fabian Grünbichler wrote:
> On November 2, 2021 4:03 pm, Aaron Lauterer wrote:
>> This new method exposes the functionality to request a new, not yet
>> used, volname for a storage.
>>
>> The default implementation will return the result from
>> 'find_free_diskname' prefixed with "<VMID>/" if $scfg->{path} exists.
>> Otherwise it will only return the result from 'find_free_diskname'.
>>
>> If the format suffix is added also depends on the existence of
>> $scfg->{path}.
>>
>> $scfg->{path} will be present for directory based storage types.
>>
>> Should a storage need to return different volname, it needs to override
>> the 'find_free_volname' method.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>
> as discussed off-list (I am sorry I didn't read through the full
> discussion on-list of all 10 versions of this and the previous series
> ;)) - IMHO this is not a backwards compatible addition since it's not
> guarded by any feature flag, so external plugins fall into two
> categories:
> - derived from Plugin.pm, so automatically implement it via
> find_free_diskname -> get_next_vm_diskname, which might or might not
> be correct for that plugin
> - not derived from Plugin.pm, don't implement it, no safeguard to handle
> that
>
> RIGHT NOW, the only callers are guarded by the rename feature so there
> can't be any problems in practice - but there is no guarantee that this
> will be remembered down the line, and it also breaks the
> modularization/layering of the code to rely on this fact ;)
>
> while we could argue about whether we really want to be that strict,
> another point against exposing this method for me is that it's also
> prone to mis-use, since it encourages code patterns like in this
> series with:
>
> get_free_volname
> do_something_that_allocates_that_volname
>
> without holding a storage lock over both calls, since the storage lock
> is only available on the plugin level and not exposed in PVE::Storage
> itself.
for the record, that's not a problem in the sense of consistency or the like,
the alloc is the important thing and that is locked, it can be a nuisance as
one can gets their rename rejected, but that's not uncommon in our API in
general.
IIRC my idea was that the user (has to) get a free name proposed (i.e., the
client can request a free name for $owner and submits that, so that they are
in the loop about the new name already, if the API call returns OK then all
worked out, else, well not -> check error.
That way we avoid that the user has no idea about what the new volid will be
and they won't have to parse some (task) logs or guess around..
>
> an interface like
> $plugin->rename_volume($scfg, $storeid, $source, $new_owner, $new_name)
>
> with at least $new_owner or $new_name set would also cover both cases
> (reassign with just $new_owner, rename with just $new_name) and can be
> feature-guarded while keeping the locking in PVE::Storage for the full
> operation including finding a free slot if needed.
>
> alternatively, just implementing
>
> $plugin->reassign_volume($scfg, $storeid, $source, $new_owner)
>
> for now (guarded by a 'reassign' feature), and postponing the rename
> feature for a future series that handles 'custom suffix/volname'
> across the board of our stack would also be an option.
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [pve-devel] [PATCH v4 storage 1/9] storage: add new find_free_volname
2021-11-08 15:53 ` Thomas Lamprecht
@ 2021-11-09 14:42 ` Aaron Lauterer
0 siblings, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-09 14:42 UTC (permalink / raw)
To: Proxmox VE development discussion, Thomas Lamprecht,
Fabian Grünbichler
On 11/8/21 16:53, Thomas Lamprecht wrote:
> On 08.11.21 16:21, Fabian Grünbichler wrote:
>> On November 2, 2021 4:03 pm, Aaron Lauterer wrote:
>>> This new method exposes the functionality to request a new, not yet
>>> used, volname for a storage.
>>>
>>> The default implementation will return the result from
>>> 'find_free_diskname' prefixed with "<VMID>/" if $scfg->{path} exists.
>>> Otherwise it will only return the result from 'find_free_diskname'.
>>>
>>> If the format suffix is added also depends on the existence of
>>> $scfg->{path}.
>>>
>>> $scfg->{path} will be present for directory based storage types.
>>>
>>> Should a storage need to return different volname, it needs to override
>>> the 'find_free_volname' method.
>>>
>>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>>
>> as discussed off-list (I am sorry I didn't read through the full
>> discussion on-list of all 10 versions of this and the previous series
>> ;)) - IMHO this is not a backwards compatible addition since it's not
>> guarded by any feature flag, so external plugins fall into two
>> categories:
>> - derived from Plugin.pm, so automatically implement it via
>> find_free_diskname -> get_next_vm_diskname, which might or might not
>> be correct for that plugin
>> - not derived from Plugin.pm, don't implement it, no safeguard to handle
>> that
>>
>> RIGHT NOW, the only callers are guarded by the rename feature so there
>> can't be any problems in practice - but there is no guarantee that this
>> will be remembered down the line, and it also breaks the
>> modularization/layering of the code to rely on this fact ;)
>>
>> while we could argue about whether we really want to be that strict,
>> another point against exposing this method for me is that it's also
>> prone to mis-use, since it encourages code patterns like in this
>> series with:
>>
>> get_free_volname
>> do_something_that_allocates_that_volname
>>
>> without holding a storage lock over both calls, since the storage lock
>> is only available on the plugin level and not exposed in PVE::Storage
>> itself.
>
> for the record, that's not a problem in the sense of consistency or the like,
> the alloc is the important thing and that is locked, it can be a nuisance as
> one can gets their rename rejected, but that's not uncommon in our API in
> general.
>
> IIRC my idea was that the user (has to) get a free name proposed (i.e., the
> client can request a free name for $owner and submits that, so that they are
> in the loop about the new name already, if the API call returns OK then all
> worked out, else, well not -> check error.
>
> That way we avoid that the user has no idea about what the new volid will be
> and they won't have to parse some (task) logs or guess around..
IIRC (it's been a while...) we discussed this and I think agreed that for now in the context of moving disks to other guests, it is okay to define the target config key, e.g. scsi4, virtio3, mp2 since this way the users knows which one in the VM config it will be. With the plan to make it possible to actually give the disk images more customizable names, but that means touching other areas as well. From what I discussed with Fabian yesterday, that is (live)migration, restores and a few other places.
>
>>
>> an interface like
>> $plugin->rename_volume($scfg, $storeid, $source, $new_owner, $new_name)
>>
>> with at least $new_owner or $new_name set would also cover both cases
>> (reassign with just $new_owner, rename with just $new_name) and can be
>> feature-guarded while keeping the locking in PVE::Storage for the full
>> operation including finding a free slot if needed.
>>
>> alternatively, just implementing
>>
>> $plugin->reassign_volume($scfg, $storeid, $source, $new_owner)
>>
>> for now (guarded by a 'reassign' feature), and postponing the rename
>> feature for a future series that handles 'custom suffix/volname'
>> across the board of our stack would also be an option.
>>
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [pve-devel] [PATCH v4 storage 2/9] add disk rename feature
2021-11-02 15:03 [pve-devel] [PATCH v4 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 storage 1/9] storage: add new find_free_volname Aaron Lauterer
@ 2021-11-02 15:03 ` Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 qemu-server 3/9] cli: qm: change move_disk to move-disk Aaron Lauterer
` (6 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 15:03 UTC (permalink / raw)
To: pve-devel
Functionality has been added for the following storage types:
* directory ones, based on the default implementation:
* directory
* NFS
* CIFS
* gluster
* ZFS
* (thin) LVM
* Ceph
A new feature `rename` has been introduced to mark which storage
plugin supports the feature.
Version API and AGE have been bumped.
The storage gets locked and each plugin checks if the target volume
already exists prior renaming.
This is done because there could be a race condition from the time the
external caller requests a new free disk name to the time the volume is
actually renamed.
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
v3 -> v4:
* add notes to ApiChangeLog
* fix style nits
v2 -> v3:
* dropped exists() check
* fixed base image handling
* fixed smaller code style issues
v1 -> v2:
* many small fixes and improvements
* rename_volume now accepts $source_volname instead of $source_volid
helps us to avoid to parse the volid a second time
rfc -> v1:
* reduced number of parameters to minimum needed, plugins infer needed
information themselves
* added storage locking and checking if volume already exists
* parse target_volname prior to renaming to check if valid
old dedicated API endpoint -> rfc:
only do rename now but the rename function handles templates and returns
the new volid as this can be differently handled on some storages.
ApiChangeLog | 10 ++++++++++
PVE/Storage.pm | 20 +++++++++++++++++--
PVE/Storage/LVMPlugin.pm | 24 +++++++++++++++++++++++
PVE/Storage/LvmThinPlugin.pm | 1 +
PVE/Storage/Plugin.pm | 37 ++++++++++++++++++++++++++++++++++++
PVE/Storage/RBDPlugin.pm | 26 +++++++++++++++++++++++++
PVE/Storage/ZFSPoolPlugin.pm | 23 ++++++++++++++++++++++
7 files changed, 139 insertions(+), 2 deletions(-)
diff --git a/ApiChangeLog b/ApiChangeLog
index 8c119c5..5b572e3 100644
--- a/ApiChangeLog
+++ b/ApiChangeLog
@@ -6,6 +6,16 @@ without breaking anything unaware of it.)
Future changes should be documented in here.
+## Version 10: (AGE 1):
+
+* a new `find_free_volname` method has been added
+ It exposes the functionality to request a new, not yet used, volname for a storage
+ to outside callers
+
+* a new `rename_volume` method has been added
+ Storage plugins with rename support need to enable the `rename` feature flag; e.g. in the
+ `volume_has_feature` method.
+
## Version 9: (AGE resets to 0):
* volume_import_formats gets a new parameter *inserted*:
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 4fbeaea..40d0d39 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -41,11 +41,11 @@ use PVE::Storage::PBSPlugin;
use PVE::Storage::BTRFSPlugin;
# Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 9;
+use constant APIVER => 10;
# Age is the number of versions we're backward compatible with.
# This is like having 'current=APIVER' and age='APIAGE' in libtool,
# see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
-use constant APIAGE => 0;
+use constant APIAGE => 1;
# load standard plugins
PVE::Storage::DirPlugin->register();
@@ -358,6 +358,7 @@ sub volume_snapshot_needs_fsfreeze {
# snapshot - taking a snapshot is possible
# sparseinit - volume is sparsely initialized
# template - conversion to base image is possible
+# rename - renaming volumes is possible
# $snap - check if the feature is supported for a given snapshot
# $running - if the guest owning the volume is running
# $opts - hash with further options:
@@ -1865,6 +1866,21 @@ sub complete_volume {
return $res;
}
+sub rename_volume {
+ my ($cfg, $source_volid, $target_volname) = @_;
+
+ my ($storeid, $source_volname) = parse_volume_id($source_volid);
+
+ activate_storage($cfg, $storeid);
+
+ my $scfg = storage_config($cfg, $storeid);
+ my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+
+ return $plugin->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
+ return $plugin->rename_volume($scfg, $storeid, $source_volname, $target_volname);
+ });
+}
+
# Various io-heavy operations require io/bandwidth limits which can be
# configured on multiple levels: The global defaults in datacenter.cfg, and
# per-storage overrides. When we want to do a restore from storage A to storage
diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
index 139d391..f28b818 100644
--- a/PVE/Storage/LVMPlugin.pm
+++ b/PVE/Storage/LVMPlugin.pm
@@ -339,6 +339,15 @@ sub lvcreate {
run_command($cmd, errmsg => "lvcreate '$vg/$name' error");
}
+sub lvrename {
+ my ($vg, $oldname, $newname) = @_;
+
+ run_command(
+ ['/sbin/lvrename', $vg, $oldname, $newname],
+ errmsg => "lvrename '${vg}/${oldname}' to '${newname}' error",
+ );
+}
+
sub alloc_image {
my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
@@ -584,6 +593,7 @@ sub volume_has_feature {
my $features = {
copy => { base => 1, current => 1},
+ rename => {current => 1},
};
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
@@ -692,4 +702,18 @@ sub volume_import_write {
input => '<&'.fileno($input_fh));
}
+sub rename_volume {
+ my ($class, $scfg, $storeid, $source_volname, $target_volname) = @_;
+
+ $class->parse_volname($target_volname);
+
+ my $vg = $scfg->{vgname};
+ my $lvs = lvm_list_volumes($vg);
+ die "target volume '${target_volname}' already exists\n"
+ if ($lvs->{$vg}->{$target_volname});
+
+ lvrename($vg, $source_volname, $target_volname);
+ return "${storeid}:${target_volname}";
+}
+
1;
diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
index 4ba6f90..c24af22 100644
--- a/PVE/Storage/LvmThinPlugin.pm
+++ b/PVE/Storage/LvmThinPlugin.pm
@@ -355,6 +355,7 @@ sub volume_has_feature {
template => { current => 1},
copy => { base => 1, current => 1, snap => 1},
sparseinit => { base => 1, current => 1},
+ rename => {current => 1},
};
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index d21c874..4535456 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -1016,6 +1016,7 @@ sub volume_has_feature {
snap => {qcow2 => 1} },
sparseinit => { base => {qcow2 => 1, raw => 1, vmdk => 1},
current => {qcow2 => 1, raw => 1, vmdk => 1} },
+ rename => { current => {qcow2 => 1, raw => 1, vmdk => 1} },
};
# clone_image creates a qcow2 volume
@@ -1023,6 +1024,8 @@ sub volume_has_feature {
defined($opts->{valid_target_formats}) &&
!(grep { $_ eq 'qcow2' } @{$opts->{valid_target_formats}});
+ return 0 if $feature eq 'rename' && $class->can('api') && $class->api() < 10;
+
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
$class->parse_volname($volname);
@@ -1539,4 +1542,38 @@ sub volume_import_formats {
return ();
}
+sub rename_volume {
+ my ($class, $scfg, $storeid, $source_volname, $target_volname) = @_;
+ die "no path found\n" if !$scfg->{path};
+ die "not implemented in storage plugin '$class'\n" if $class->can('api') && $class->api() < 10;
+
+ my (undef, undef, $target_vmid) = $class->parse_volname($target_volname);
+
+ my (
+ undef,
+ $source_image,
+ $source_vmid,
+ $base_name,
+ $base_vmid,
+ undef,
+ $format
+ ) = $class->parse_volname($source_volname);
+
+ my $basedir = $class->get_subdir($scfg, 'images');
+
+ mkpath "${basedir}/${target_vmid}";
+
+ my $old_path = "${basedir}/${source_vmid}/${source_image}";
+ my $new_path = "${basedir}/${target_volname}";
+
+ die "target volume '${target_volname}' already exists\n" if -e $new_path;
+
+ my $base = $base_name ? "${base_vmid}/${base_name}/" : '';
+
+ rename($old_path, $new_path) ||
+ die "rename '$old_path' to '$new_path' failed - $!\n";
+
+ return "${storeid}:${base}${target_volname}";
+}
+
1;
diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index 613d32b..b9e1bc0 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -742,6 +742,7 @@ sub volume_has_feature {
template => { current => 1},
copy => { base => 1, current => 1, snap => 1},
sparseinit => { base => 1, current => 1},
+ rename => {current => 1},
};
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) = $class->parse_volname($volname);
@@ -757,4 +758,29 @@ sub volume_has_feature {
return undef;
}
+sub rename_volume {
+ my ($class, $scfg, $storeid, $source_volname, $target_volname) = @_;
+
+ $class->parse_volname($target_volname);
+
+ my (undef, $source_image, $source_vmid, $base_name) = $class->parse_volname($source_volname);
+
+ eval {
+ my $cmd = $rbd_cmd->($scfg, $storeid, 'info', $target_volname);
+ run_rbd_command($cmd, errmsg => "exist check", quiet => 1);
+ };
+ die "target volume '${target_volname}' already exists\n" if !$@;
+
+ my $cmd = $rbd_cmd->($scfg, $storeid, 'rename', $source_image, $target_volname);
+
+ run_rbd_command(
+ $cmd,
+ errmsg => "could not rename image '${source_image}' to '${target_volname}'",
+ );
+
+ $base_name = $base_name ? "${base_name}/" : '';
+
+ return "${storeid}:${base_name}${target_volname}";
+}
+
1;
diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index 660b3d9..c5d2e67 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -690,6 +690,7 @@ sub volume_has_feature {
copy => { base => 1, current => 1},
sparseinit => { base => 1, current => 1},
replicate => { base => 1, current => 1},
+ rename => {current => 1},
};
my ($vtype, $name, $vmid, $basename, $basevmid, $isBase) =
@@ -792,4 +793,26 @@ sub volume_import_formats {
return $class->volume_export_formats($scfg, $storeid, $volname, $snapshot, $base_snapshot, $with_snapshots);
}
+sub rename_volume {
+ my ($class, $scfg, $storeid, $source_volname, $target_volname) = @_;
+
+ $class->parse_volname($target_volname);
+
+ my (undef, $source_image, $source_vmid, $base_name) = $class->parse_volname($source_volname);
+
+ my $pool = $scfg->{pool};
+ my $source_zfspath = "${pool}/${source_image}";
+ my $target_zfspath = "${pool}/${target_volname}";
+
+ my $exists = 0 == run_command(['zfs', 'get', '-H', 'name', $target_zfspath],
+ noerr => 1, quiet => 1);
+ die "target volume '${target_volname}' already exists\n" if $exists;
+
+ $class->zfs_request($scfg, 5, 'rename', ${source_zfspath}, ${target_zfspath});
+
+ $base_name = $base_name ? "${base_name}/" : '';
+
+ return "${storeid}:${base_name}${target_volname}";
+}
+
1;
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [pve-devel] [PATCH v4 qemu-server 3/9] cli: qm: change move_disk to move-disk
2021-11-02 15:03 [pve-devel] [PATCH v4 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 storage 1/9] storage: add new find_free_volname Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 storage 2/9] add disk rename feature Aaron Lauterer
@ 2021-11-02 15:03 ` Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 qemu-server 4/9] Drive: add valid_drive_names_with_unused Aaron Lauterer
` (5 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 15:03 UTC (permalink / raw)
To: pve-devel
also add alias to keep move_disk working.
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
PVE/CLI/qm.pm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 8307dc1..ef99b6d 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -910,7 +910,8 @@ our $cmddef = {
resize => [ "PVE::API2::Qemu", 'resize_vm', ['vmid', 'disk', 'size'], { node => $nodename } ],
- move_disk => [ "PVE::API2::Qemu", 'move_vm_disk', ['vmid', 'disk', 'storage'], { node => $nodename }, $upid_exit ],
+ 'move-disk' => [ "PVE::API2::Qemu", 'move_vm_disk', ['vmid', 'disk', 'storage'], { node => $nodename }, $upid_exit ],
+ move_disk => { alias => 'move-disk' },
unlink => [ "PVE::API2::Qemu", 'unlink', ['vmid'], { node => $nodename } ],
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [pve-devel] [PATCH v4 qemu-server 4/9] Drive: add valid_drive_names_with_unused
2021-11-02 15:03 [pve-devel] [PATCH v4 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
` (2 preceding siblings ...)
2021-11-02 15:03 ` [pve-devel] [PATCH v4 qemu-server 3/9] cli: qm: change move_disk to move-disk Aaron Lauterer
@ 2021-11-02 15:03 ` Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 qemu-server 5/9] api: move-disk: add move to other VM Aaron Lauterer
` (4 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 15:03 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
v1 -> v2: fixed spacing between - and 1
PVE/QemuServer/Drive.pm | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 97b82f9..0f9ceba 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -467,6 +467,10 @@ sub valid_drive_names {
'tpmstate0');
}
+sub valid_drive_names_with_unused {
+ return (valid_drive_names(), map {"unused$_"} (0 .. ($MAX_UNUSED_DISKS - 1)));
+}
+
sub is_valid_drivename {
my $dev = shift;
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [pve-devel] [PATCH v4 qemu-server 5/9] api: move-disk: add move to other VM
2021-11-02 15:03 [pve-devel] [PATCH v4 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
` (3 preceding siblings ...)
2021-11-02 15:03 ` [pve-devel] [PATCH v4 qemu-server 4/9] Drive: add valid_drive_names_with_unused Aaron Lauterer
@ 2021-11-02 15:03 ` Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 qemu-server 6/9] api: move-disk: cleanup very long lines Aaron Lauterer
` (3 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 15:03 UTC (permalink / raw)
To: pve-devel
The goal of this is to expand the move-disk API endpoint to make it
possible to move a disk to another VM. Previously this was only possible
with manual intervertion either by renaming the VM disk or by manually
adding the disks volid to the config of the other VM.
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
v3 -> v4:
* add check to avoid potential undefined warning in $vmlist->{$vmid}
* removed line bloat
v2 -> v3:
* mention default target disk key in description
* code style things
v1 -> v2:
* make --target-disk optional and use source disk key as fallback
* use parse_volname instead of custom regex
* adapt to find_free_volname
* smaller (style) fixes
rfc -> v1:
* add check if target guest is replicated and fail if the moved volume
does not support it
* check if source volume has a format suffix and pass it to
'find_free_disk'
* fixed some style nits
old dedicated api endpoint -> rfc:
There are some big changes here. The old [0] dedicated API endpoint is
gone and most of its code is now part of move_disk. Error messages have
been changed accordingly and sometimes enahnced by adding disk keys and
VMIDs where appropriate.
Since a move to other guests should be possible for unused disks, we
need to check before doing a move to storage to make sure to not
handle unused disks.
PVE/API2/Qemu.pm | 209 +++++++++++++++++++++++++++++++++++++++++++++--
PVE/CLI/qm.pm | 2 +-
2 files changed, 205 insertions(+), 6 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 1ac81e2..9d7722d 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -36,6 +36,7 @@ use PVE::API2::Qemu::Agent;
use PVE::VZDump::Plugin;
use PVE::DataCenterConfig;
use PVE::SSHInfo;
+use PVE::Replication;
BEGIN {
if (!$ENV{PVE_GENERATING_DOCS}) {
@@ -3280,9 +3281,11 @@ __PACKAGE__->register_method({
method => 'POST',
protected => 1,
proxyto => 'node',
- description => "Move volume to different storage.",
+ description => "Move volume to different storage or to a different VM.",
permissions => {
- description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, and 'Datastore.AllocateSpace' permissions on the storage.",
+ description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, " .
+ "and 'Datastore.AllocateSpace' permissions on the storage. To move ".
+ "a disk to another VM, you need the permissions on the target VM as well.",
check => [ 'and',
['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
['perm', '/storage/{storage}', [ 'Datastore.AllocateSpace' ]],
@@ -3293,14 +3296,19 @@ __PACKAGE__->register_method({
properties => {
node => get_standard_option('pve-node'),
vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
+ 'target-vmid' => get_standard_option('pve-vmid', {
+ completion => \&PVE::QemuServer::complete_vmid,
+ optional => 1,
+ }),
disk => {
type => 'string',
description => "The disk you want to move.",
- enum => [PVE::QemuServer::Drive::valid_drive_names()],
+ enum => [PVE::QemuServer::Drive::valid_drive_names_with_unused()],
},
storage => get_standard_option('pve-storage-id', {
description => "Target storage.",
completion => \&PVE::QemuServer::complete_storage,
+ optional => 1,
}),
'format' => {
type => 'string',
@@ -3327,6 +3335,20 @@ __PACKAGE__->register_method({
minimum => '0',
default => 'move limit from datacenter or storage config',
},
+ 'target-disk' => {
+ type => 'string',
+ description => "The config key the disk will be moved to on the target VM " .
+ "(for example, ide0 or scsi1). Default is the source disk key.",
+ enum => [PVE::QemuServer::Drive::valid_drive_names_with_unused()],
+ optional => 1,
+ },
+ 'target-digest' => {
+ type => 'string',
+ description => 'Prevent changes if current configuration file of the target VM has " .
+ "a different SHA1 digest. This can be used to prevent concurrent modifications.',
+ maxLength => 40,
+ optional => 1,
+ },
},
},
returns => {
@@ -3341,14 +3363,20 @@ __PACKAGE__->register_method({
my $node = extract_param($param, 'node');
my $vmid = extract_param($param, 'vmid');
+ my $target_vmid = extract_param($param, 'target-vmid');
my $digest = extract_param($param, 'digest');
+ my $target_digest = extract_param($param, 'target-digest');
my $disk = extract_param($param, 'disk');
+ my $target_disk = extract_param($param, 'target-disk') // $disk;
my $storeid = extract_param($param, 'storage');
my $format = extract_param($param, 'format');
+ die "either set storage or target-vmid, but not both\n" if $storeid && $target_vmid;
+
my $storecfg = PVE::Storage::config();
+ my $source_volid;
- my $updatefn = sub {
+ my $move_updatefn = sub {
my $conf = PVE::QemuConfig->load_config($vmid);
PVE::QemuConfig->check_lock($conf);
@@ -3458,7 +3486,178 @@ __PACKAGE__->register_method({
return $rpcenv->fork_worker('qmmove', $vmid, $authuser, $realcmd);
};
- return PVE::QemuConfig->lock_config($vmid, $updatefn);
+ my $load_and_check_reassign_configs = sub {
+ my $vmlist = PVE::Cluster::get_vmlist()->{ids};
+
+ die "could not find VM ${vmid}\n" if !exists($vmlist->{$vmid});
+
+ if ($vmlist->{$vmid}->{node} ne $vmlist->{$target_vmid}->{node}) {
+ die "Both VMs need to be on the same node $vmlist->{$vmid}->{node}) ".
+ "but target VM is on $vmlist->{$target_vmid}->{node}.\n";
+ }
+
+ my $source_conf = PVE::QemuConfig->load_config($vmid);
+ PVE::QemuConfig->check_lock($source_conf);
+ my $target_conf = PVE::QemuConfig->load_config($target_vmid);
+ PVE::QemuConfig->check_lock($target_conf);
+
+ die "Can't move disks from or to template VMs\n"
+ if ($source_conf->{template} || $target_conf->{template});
+
+ if ($digest) {
+ eval { PVE::Tools::assert_if_modified($digest, $source_conf->{digest}) };
+ die "VM ${vmid}: $@" if $@;
+ }
+
+ if ($target_digest) {
+ eval { PVE::Tools::assert_if_modified($target_digest, $target_conf->{digest}) };
+ die "VM ${target_vmid}: $@" if $@;
+ }
+
+ die "Disk '${disk}' for VM '$vmid' does not exist\n" if !defined($source_conf->{$disk});
+
+ die "Target disk key '${target_disk}' is already in use for VM '$target_vmid'\n"
+ if exists($target_conf->{$target_disk});
+
+ my $drive = PVE::QemuServer::parse_drive(
+ $disk,
+ $source_conf->{$disk},
+ );
+ $source_volid = $drive->{file};
+
+ die "disk '${disk}' has no associated volume\n" if !$source_volid;
+ die "CD drive contents can't be moved to another VM\n"
+ if PVE::QemuServer::drive_is_cdrom($drive, 1);
+ die "Can't move physical disk to another VM\n" if $source_volid =~ m|^/dev/|;
+ die "Can't move disk used by a snapshot to another VM\n"
+ if PVE::QemuServer::Drive::is_volume_in_use($storecfg, $source_conf, $disk, $source_volid);
+ die "Storage does not support moving of this disk to another VM\n"
+ if (!PVE::Storage::volume_has_feature($storecfg, 'rename', $source_volid));
+ die "Cannot move disk to another VM while the source VM is running\n"
+ if PVE::QemuServer::check_running($vmid) && $disk !~ m/^unused\d+$/;
+
+ if ($target_disk !~ m/^unused\d+$/ && $target_disk =~ m/^([^\d]+)\d+$/) {
+ my $interface = $1;
+ my $desc = PVE::JSONSchema::get_standard_option("pve-qm-${interface}");
+ eval {
+ PVE::JSONSchema::parse_property_string(
+ $desc->{format},
+ $source_conf->{$disk},
+ )
+ };
+ die "Cannot move disk to another VM: $@" if $@;
+ }
+
+ my $repl_conf = PVE::ReplicationConfig->new();
+ my $is_replicated = $repl_conf->check_for_existing_jobs($target_vmid, 1);
+ my ($storeid, undef) = PVE::Storage::parse_volume_id($source_volid);
+ my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
+ if ($is_replicated && !PVE::Storage::storage_can_replicate($storecfg, $storeid, $format)) {
+ die "Cannot move disk to a replicated VM. Storage does not support replication!\n";
+ }
+
+ return ($source_conf, $target_conf);
+ };
+
+ my $logfunc = sub {
+ my ($msg) = @_;
+ print STDERR "$msg\n";
+ };
+
+ my $disk_reassignfn = sub {
+ return PVE::QemuConfig->lock_config($vmid, sub {
+ return PVE::QemuConfig->lock_config($target_vmid, sub {
+ my ($source_conf, $target_conf) = &$load_and_check_reassign_configs();
+
+ my $drive_param = PVE::QemuServer::parse_drive(
+ $target_disk,
+ $source_conf->{$disk},
+ );
+
+ print "moving disk '$disk' from VM '$vmid' to '$target_vmid'\n";
+ my ($storeid, $source_volname) = PVE::Storage::parse_volume_id($source_volid);
+
+ my $fmt = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
+
+ my $target_volname = PVE::Storage::find_free_volname(
+ $storecfg,
+ $storeid,
+ $target_vmid,
+ $fmt
+ );
+
+ my $new_volid = PVE::Storage::rename_volume(
+ $storecfg,
+ $source_volid,
+ $target_volname,
+ );
+
+ $drive_param->{file} = $new_volid;
+
+ delete $source_conf->{$disk};
+ print "removing disk '${disk}' from VM '${vmid}' config\n";
+ PVE::QemuConfig->write_config($vmid, $source_conf);
+
+ my $drive_string = PVE::QemuServer::print_drive($drive_param);
+ &$update_vm_api(
+ {
+ node => $node,
+ vmid => $target_vmid,
+ digest => $target_digest,
+ $target_disk => $drive_string,
+ },
+ 1,
+ );
+
+ # remove possible replication snapshots
+ if (PVE::Storage::volume_has_feature(
+ $storecfg,
+ 'replicate',
+ $source_volid),
+ ) {
+ eval {
+ PVE::Replication::prepare(
+ $storecfg,
+ [$new_volid],
+ undef,
+ 1,
+ undef,
+ $logfunc,
+ )
+ };
+ if (my $err = $@) {
+ print "Failed to remove replication snapshots on moved disk " .
+ "'$target_disk'. Manual cleanup could be necessary.\n";
+ }
+ }
+ });
+ });
+ };
+
+ if ($target_vmid) {
+ $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk'])
+ if $authuser ne 'root@pam';
+
+ die "Moving a disk to the same VM is not possible. Did you mean to ".
+ "move the disk to a different storage?\n"
+ if $vmid eq $target_vmid;
+
+ &$load_and_check_reassign_configs();
+ return $rpcenv->fork_worker(
+ 'qmmove',
+ "${vmid}-${disk}>${target_vmid}-${target_disk}",
+ $authuser,
+ $disk_reassignfn
+ );
+ } elsif ($storeid) {
+ die "cannot move disk '$disk', only configured disks can be moved to another storage\n"
+ if $disk =~ m/^unused\d+$/;
+ return PVE::QemuConfig->lock_config($vmid, $move_updatefn);
+ } else {
+ die "Provide either a 'storage' to move the disk to a different " .
+ "storage or 'target-vmid' and 'target-disk' to move the disk " .
+ "to another VM\n";
+ }
}});
my $check_vm_disks_local = sub {
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index ef99b6d..a92d301 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -910,7 +910,7 @@ our $cmddef = {
resize => [ "PVE::API2::Qemu", 'resize_vm', ['vmid', 'disk', 'size'], { node => $nodename } ],
- 'move-disk' => [ "PVE::API2::Qemu", 'move_vm_disk', ['vmid', 'disk', 'storage'], { node => $nodename }, $upid_exit ],
+ 'move-disk' => [ "PVE::API2::Qemu", 'move_vm_disk', ['vmid', 'disk', 'storage', 'target-vmid', 'target-disk'], { node => $nodename }, $upid_exit ],
move_disk => { alias => 'move-disk' },
unlink => [ "PVE::API2::Qemu", 'unlink', ['vmid'], { node => $nodename } ],
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [pve-devel] [PATCH v4 qemu-server 6/9] api: move-disk: cleanup very long lines
2021-11-02 15:03 [pve-devel] [PATCH v4 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
` (4 preceding siblings ...)
2021-11-02 15:03 ` [pve-devel] [PATCH v4 qemu-server 5/9] api: move-disk: add move to other VM Aaron Lauterer
@ 2021-11-02 15:03 ` Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 container 7/9] cli: pct: change move_volume to move-volume Aaron Lauterer
` (2 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 15:03 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
PVE/API2/Qemu.pm | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9d7722d..c623471 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3318,13 +3318,15 @@ __PACKAGE__->register_method({
},
delete => {
type => 'boolean',
- description => "Delete the original disk after successful copy. By default the original disk is kept as unused disk.",
+ description => "Delete the original disk after successful copy. By default the " .
+ "original disk is kept as unused disk.",
optional => 1,
default => 0,
},
digest => {
type => 'string',
- description => 'Prevent changes if current configuration file has different SHA1 digest. This can be used to prevent concurrent modifications.',
+ description => 'Prevent changes if current configuration file has different SHA1 " .
+ "digest. This can be used to prevent concurrent modifications.',
maxLength => 40,
optional => 1,
},
@@ -3401,11 +3403,20 @@ __PACKAGE__->register_method({
(!$format || !$oldfmt || $oldfmt eq $format);
# this only checks snapshots because $disk is passed!
- my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use($storecfg, $conf, $disk, $old_volid);
+ my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use(
+ $storecfg,
+ $conf,
+ $disk,
+ $old_volid
+ );
die "you can't move a disk with snapshots and delete the source\n"
if $snapshotted && $param->{delete};
- PVE::Cluster::log_msg('info', $authuser, "move disk VM $vmid: move --disk $disk --storage $storeid");
+ PVE::Cluster::log_msg(
+ 'info',
+ $authuser,
+ "move disk VM $vmid: move --disk $disk --storage $storeid"
+ );
my $running = PVE::QemuServer::check_running($vmid);
@@ -3424,7 +3435,11 @@ __PACKAGE__->register_method({
if $snapshotted;
my $bwlimit = extract_param($param, 'bwlimit');
- my $movelimit = PVE::Storage::get_bandwidth_limit('move', [$oldstoreid, $storeid], $bwlimit);
+ my $movelimit = PVE::Storage::get_bandwidth_limit(
+ 'move',
+ [$oldstoreid, $storeid],
+ $bwlimit
+ );
my $newdrive = PVE::QemuServer::clone_disk(
$storecfg,
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [pve-devel] [PATCH v4 container 7/9] cli: pct: change move_volume to move-volume
2021-11-02 15:03 [pve-devel] [PATCH v4 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
` (5 preceding siblings ...)
2021-11-02 15:03 ` [pve-devel] [PATCH v4 qemu-server 6/9] api: move-disk: cleanup very long lines Aaron Lauterer
@ 2021-11-02 15:03 ` Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 container 8/9] api: move-volume: add move to another container Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 container 9/9] api: move-volume: cleanup very long lines Aaron Lauterer
8 siblings, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 15:03 UTC (permalink / raw)
To: pve-devel
also add alias to keep move_volume working
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
v1 -> v2: fix alias to actually point to move-volume
src/PVE/CLI/pct.pm | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index 254b3b3..462917b 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -849,7 +849,8 @@ our $cmddef = {
clone => [ "PVE::API2::LXC", 'clone_vm', ['vmid', 'newid'], { node => $nodename }, $upid_exit ],
migrate => [ "PVE::API2::LXC", 'migrate_vm', ['vmid', 'target'], { node => $nodename }, $upid_exit],
- move_volume => [ "PVE::API2::LXC", 'move_volume', ['vmid', 'volume', 'storage'], { node => $nodename }, $upid_exit ],
+ 'move-volume' => [ "PVE::API2::LXC", 'move_volume', ['vmid', 'volume', 'storage', 'target-vmid', 'target-volume'], { node => $nodename }, $upid_exit ],
+ move_volume => { alias => 'move-volume' },
snapshot => [ "PVE::API2::LXC::Snapshot", 'snapshot', ['vmid', 'snapname'], { node => $nodename } , $upid_exit ],
delsnapshot => [ "PVE::API2::LXC::Snapshot", 'delsnapshot', ['vmid', 'snapname'], { node => $nodename } , $upid_exit ],
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread* [pve-devel] [PATCH v4 container 8/9] api: move-volume: add move to another container
2021-11-02 15:03 [pve-devel] [PATCH v4 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
` (6 preceding siblings ...)
2021-11-02 15:03 ` [pve-devel] [PATCH v4 container 7/9] cli: pct: change move_volume to move-volume Aaron Lauterer
@ 2021-11-02 15:03 ` Aaron Lauterer
2021-11-02 16:44 ` Aaron Lauterer
2021-11-05 14:56 ` [pve-devel] [PATCH v4 container 8/9 (follow up)] " Aaron Lauterer
2021-11-02 15:03 ` [pve-devel] [PATCH v4 container 9/9] api: move-volume: cleanup very long lines Aaron Lauterer
8 siblings, 2 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 15:03 UTC (permalink / raw)
To: pve-devel
The goal of this is to expand the move-volume API endpoint to make it
possible to move a container volume / mountpoint to another container.
Currently it works for regular mountpoints though it would be nice to be
able to do it for unused mounpoints as well.
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
This is mostly the code from qemu-server with some adaptions. Mainly
error messages and some checks.
Previous checks have been moved to '$move_to_storage_checks'.
v3 -> v4:
* add check to avoid potential undefined warning in $vmlist->{$vmid}
* add check if volume is part of a snapshot
* removed line bloat
v2 -> v3:
* mention default volume key in description
* use $rpcenv->warn should replication snapshot removal fail
* also print the original error
* code style things
v1 -> v2:
* change --target-mp to --target-volume
* make --target-volume optional and fallback to source mount point
* use parse_volname instead of custom regex
* adapt to find_free_volname
* print warnings from update_pct_config
* move running check back in both lock_config sections
* fixed a few style issues
rfc -> v1:
* add check if target guest is replicated and fail if the moved volume
does not support it
* check if source volume has a format suffix and pass it to
'find_free_disk' or if the prefix is vm/subvol as those also have
their own meaning, see the comment in the code
* fixed some style nits
src/PVE/API2/LXC.pm | 253 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 232 insertions(+), 21 deletions(-)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 69df366..afc16d7 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1813,10 +1813,12 @@ __PACKAGE__->register_method({
method => 'POST',
protected => 1,
proxyto => 'node',
- description => "Move a rootfs-/mp-volume to a different storage",
+ description => "Move a rootfs-/mp-volume to a different storage or to a different container.",
permissions => {
description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, " .
- "and 'Datastore.AllocateSpace' permissions on the storage.",
+ "and 'Datastore.AllocateSpace' permissions on the storage. To move ".
+ "a volume to another container, you need the permissions on the ".
+ "target container as well.",
check =>
[ 'and',
['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
@@ -1828,14 +1830,20 @@ __PACKAGE__->register_method({
properties => {
node => get_standard_option('pve-node'),
vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
+ 'target-vmid' => get_standard_option('pve-vmid', {
+ completion => \&PVE::LXC::complete_ctid,
+ optional => 1,
+ }),
volume => {
type => 'string',
+ #TODO: check how to handle unused mount points as the mp parameter is not configured
enum => [ PVE::LXC::Config->valid_volume_keys() ],
description => "Volume which will be moved.",
},
storage => get_standard_option('pve-storage-id', {
description => "Target Storage.",
completion => \&PVE::Storage::complete_storage_enabled,
+ optional => 1,
}),
delete => {
type => 'boolean',
@@ -1856,6 +1864,21 @@ __PACKAGE__->register_method({
minimum => '0',
default => 'clone limit from datacenter or storage config',
},
+ 'target-volume' => {
+ type => 'string',
+ description => "The config key the volume will be moved to. Default is the " .
+ "source volume key.",
+ enum => [PVE::LXC::Config->valid_volume_keys()],
+ optional => 1,
+ },
+ 'target-digest' => {
+ type => 'string',
+ description => 'Prevent changes if current configuration file of the target " .
+ "container has a different SHA1 digest. This can be used to prevent " .
+ "concurrent modifications.',
+ maxLength => 40,
+ optional => 1,
+ },
},
},
returns => {
@@ -1870,32 +1893,48 @@ __PACKAGE__->register_method({
my $vmid = extract_param($param, 'vmid');
+ my $target_vmid = extract_param($param, 'target-vmid');
+
my $storage = extract_param($param, 'storage');
my $mpkey = extract_param($param, 'volume');
+ my $target_mpkey = extract_param($param, 'target-volume') // $mpkey;
+
+ my $digest = extract_param($param, 'digest');
+
+ my $target_digest = extract_param($param, 'target-digest');
+
my $lockname = 'disk';
my ($mpdata, $old_volid);
- PVE::LXC::Config->lock_config($vmid, sub {
- my $conf = PVE::LXC::Config->load_config($vmid);
- PVE::LXC::Config->check_lock($conf);
+ die "either set storage or target-vmid, but not both\n"
+ if $storage && $target_vmid;
- die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid);
+ my $storecfg = PVE::Storage::config();
+ my $source_volid;
- $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey});
- $old_volid = $mpdata->{volume};
+ my $move_to_storage_checks = sub {
+ PVE::LXC::Config->lock_config($vmid, sub {
+ my $conf = PVE::LXC::Config->load_config($vmid);
+ PVE::LXC::Config->check_lock($conf);
- die "you can't move a volume with snapshots and delete the source\n"
- if $param->{delete} && PVE::LXC::Config->is_volume_in_use_by_snapshots($conf, $old_volid);
+ die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid);
- PVE::Tools::assert_if_modified($param->{digest}, $conf->{digest});
+ $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey});
+ $old_volid = $mpdata->{volume};
- PVE::LXC::Config->set_lock($vmid, $lockname);
- });
+ die "you can't move a volume with snapshots and delete the source\n"
+ if $param->{delete} && PVE::LXC::Config->is_volume_in_use_by_snapshots($conf, $old_volid);
- my $realcmd = sub {
+ PVE::Tools::assert_if_modified($digest, $conf->{digest});
+
+ PVE::LXC::Config->set_lock($vmid, $lockname);
+ });
+ };
+
+ my $storage_realcmd = sub {
eval {
PVE::Cluster::log_msg('info', $authuser, "move volume CT $vmid: move --volume $mpkey --storage $storage");
@@ -1965,15 +2004,187 @@ __PACKAGE__->register_method({
warn $@ if $@;
die $err if $err;
};
- my $task = eval {
- $rpcenv->fork_worker('move_volume', $vmid, $authuser, $realcmd);
+
+ my $load_and_check_reassign_configs = sub {
+ my $vmlist = PVE::Cluster::get_vmlist()->{ids};
+
+ die "could not find CT ${vmid}\n" if !exists($vmlist->{$vmid});
+
+ die "Both containers need to be on the same node ($vmlist->{$vmid}->{node}) ".
+ "but target continer is on $vmlist->{$target_vmid}->{node}.\n"
+ if $vmlist->{$vmid}->{node} ne $vmlist->{$target_vmid}->{node};
+
+ my $source_conf = PVE::LXC::Config->load_config($vmid);
+ PVE::LXC::Config->check_lock($source_conf);
+ my $target_conf = PVE::LXC::Config->load_config($target_vmid);
+ PVE::LXC::Config->check_lock($target_conf);
+
+ die "Can't move volumes from or to template VMs\n"
+ if ($source_conf->{template} || $target_conf->{template});
+
+ if ($digest) {
+ eval { PVE::Tools::assert_if_modified($digest, $source_conf->{digest}) };
+ die "Container ${vmid}: $@" if $@;
+ }
+
+ if ($target_digest) {
+ eval { PVE::Tools::assert_if_modified($target_digest, $target_conf->{digest}) };
+ die "Container ${target_vmid}: $@" if $@;
+ }
+
+ die "volume '${mpkey}' for container '$vmid' does not exist\n"
+ if !defined($source_conf->{$mpkey});
+
+ die "Target volume key '${target_mpkey}' is already in use for container '$target_vmid'\n"
+ if exists $target_conf->{$target_mpkey};
+
+ my $drive = PVE::LXC::Config->parse_volume(
+ $mpkey,
+ $source_conf->{$mpkey},
+ );
+
+ $source_volid = $drive->{volume};
+
+ die "disk '${mpkey}' has no associated volume\n"
+ if !$source_volid;
+ die "Cannot move volume used by a snapshot to another container\n"
+ if PVE::LXC::Config->is_volume_in_use_by_snapshots($source_conf, $source_volid);
+ die "Storage does not support moving of this disk to another container\n"
+ if !PVE::Storage::volume_has_feature($storecfg, 'rename', $source_volid);
+ die "Cannot move a bindmound or device mount to another container\n"
+ if $drive->{type} ne "volume";
+ die "Cannot move volume to another container while the source container is running\n"
+ if PVE::LXC::check_running($vmid) && $mpkey !~ m/^unused\d+$/;
+
+ my $repl_conf = PVE::ReplicationConfig->new();
+ my $is_replicated = $repl_conf->check_for_existing_jobs($target_vmid, 1);
+ my ($storeid, undef) = PVE::Storage::parse_volume_id($source_volid);
+ my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
+ if (
+ $is_replicated
+ && !PVE::Storage::storage_can_replicate($storecfg, $storeid, $format)
+ ) {
+ die "Cannot move volume to a replicated container. Storage " .
+ "does not support replication!\n";
+ }
+ return ($source_conf, $target_conf);
};
- if (my $err = $@) {
- eval { PVE::LXC::Config->remove_lock($vmid, $lockname) };
- warn $@ if $@;
- die $err;
+
+ my $logfunc = sub {
+ my ($msg) = @_;
+ print STDERR "$msg\n";
+ };
+
+ my $volume_reassignfn = sub {
+ return PVE::LXC::Config->lock_config($vmid, sub {
+ return PVE::LXC::Config->lock_config($target_vmid, sub {
+ my ($source_conf, $target_conf) = &$load_and_check_reassign_configs();
+ die "cannot move volumes of a running container\n"
+ if PVE::LXC::check_running($vmid);
+
+ my $drive_param = PVE::LXC::Config->parse_volume(
+ $target_mpkey,
+ $source_conf->{$mpkey},
+ );
+
+ print "moving volume '$mpkey' from container '$vmid' to '$target_vmid'\n";
+ my ($storage, $source_volname) = PVE::Storage::parse_volume_id($source_volid);
+
+ my $fmt = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
+
+ my $target_volname = PVE::Storage::find_free_volname(
+ $storecfg,
+ $storage,
+ $target_vmid,
+ $fmt
+ );
+
+ my $new_volid = PVE::Storage::rename_volume(
+ $storecfg,
+ $source_volid,
+ $target_volname,
+ );
+
+ $drive_param->{volume} = $new_volid;
+
+ delete $source_conf->{$mpkey};
+ print "removing volume '${mpkey}' from container '${vmid}' config\n";
+ PVE::LXC::Config->write_config($vmid, $source_conf);
+
+ my $drive_string = PVE::LXC::Config->print_volume($target_mpkey, $drive_param);
+ my $running = PVE::LXC::check_running($target_vmid);
+ my $param = { $target_mpkey => $drive_string };
+
+ my $errors = PVE::LXC::Config->update_pct_config(
+ $target_vmid,
+ $target_conf,
+ $running,
+ $param
+ );
+
+ foreach my $key (keys %$errors) {
+ $rpcenv->warn($errors->{$key});
+ }
+
+ PVE::LXC::Config->write_config($target_vmid, $target_conf);
+ $target_conf = PVE::LXC::Config->load_config($target_vmid);
+
+ PVE::LXC::update_lxc_config($target_vmid, $target_conf);
+ print "target container '$target_vmid' updated with '$target_mpkey'\n";
+
+ # remove possible replication snapshots
+ if (PVE::Storage::volume_has_feature($storecfg,'replicate', $source_volid)) {
+ eval {
+ PVE::Replication::prepare(
+ $storecfg,
+ [$new_volid],
+ undef,
+ 1,
+ undef,
+ $logfunc,
+ )
+ };
+ if (my $err = $@) {
+ $rpcenv->warn("Failed to remove replication snapshots on volume ".
+ "'${target_mpkey}'. Manual cleanup could be necessary. " .
+ "Error: ${err}\n");
+ }
+ }
+ });
+ });
+ };
+
+ if ($target_vmid) {
+ $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk'])
+ if $authuser ne 'root@pam';
+
+ die "Moving a volume to the same container is not possible. Did you ".
+ "mean to move the volume to a different storage?\n"
+ if $vmid eq $target_vmid;
+
+ &$load_and_check_reassign_configs();
+ return $rpcenv->fork_worker(
+ 'move_volume',
+ "${vmid}-${mpkey}>${target_vmid}-${target_mpkey}",
+ $authuser,
+ $volume_reassignfn
+ );
+ } elsif ($storage) {
+ &$move_to_storage_checks();
+ my $task = eval {
+ $rpcenv->fork_worker('move_volume', $vmid, $authuser, $storage_realcmd);
+ };
+ if (my $err = $@) {
+ eval { PVE::LXC::Config->remove_lock($vmid, $lockname) };
+ warn $@ if $@;
+ die $err;
+ }
+ return $task;
+ } else {
+ die "Provide either a 'storage' to move the mount point to a ".
+ "different storage or 'target-vmid' and 'target-mp' to move ".
+ "the mount point to another container\n";
}
- return $task;
}});
__PACKAGE__->register_method({
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [pve-devel] [PATCH v4 container 8/9] api: move-volume: add move to another container
2021-11-02 15:03 ` [pve-devel] [PATCH v4 container 8/9] api: move-volume: add move to another container Aaron Lauterer
@ 2021-11-02 16:44 ` Aaron Lauterer
2021-11-05 14:56 ` [pve-devel] [PATCH v4 container 8/9 (follow up)] " Aaron Lauterer
1 sibling, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 16:44 UTC (permalink / raw)
To: pve-devel
Please wait a bit reviewing this patch. I did send it a bit too early as there are a few things not yet fully done. Especially being able to move unused volume or to move a volume from mpX to an unusedX volume on the target container.
Additionally we probably should not allow the rootfs to be moved as that will lead to som unexpected behaviors because AFAICT we never deal with a container missing a rootfs anywhere in our code.
On 11/2/21 16:03, Aaron Lauterer wrote:
> The goal of this is to expand the move-volume API endpoint to make it
> possible to move a container volume / mountpoint to another container.
>
> Currently it works for regular mountpoints though it would be nice to be
> able to do it for unused mounpoints as well.
>
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> This is mostly the code from qemu-server with some adaptions. Mainly
> error messages and some checks.
>
> Previous checks have been moved to '$move_to_storage_checks'.
>
> v3 -> v4:
> * add check to avoid potential undefined warning in $vmlist->{$vmid}
> * add check if volume is part of a snapshot
> * removed line bloat
>
> v2 -> v3:
> * mention default volume key in description
> * use $rpcenv->warn should replication snapshot removal fail
> * also print the original error
> * code style things
>
> v1 -> v2:
> * change --target-mp to --target-volume
> * make --target-volume optional and fallback to source mount point
> * use parse_volname instead of custom regex
> * adapt to find_free_volname
> * print warnings from update_pct_config
> * move running check back in both lock_config sections
> * fixed a few style issues
>
> rfc -> v1:
> * add check if target guest is replicated and fail if the moved volume
> does not support it
> * check if source volume has a format suffix and pass it to
> 'find_free_disk' or if the prefix is vm/subvol as those also have
> their own meaning, see the comment in the code
> * fixed some style nits
> src/PVE/API2/LXC.pm | 253 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 232 insertions(+), 21 deletions(-)
>
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 69df366..afc16d7 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -1813,10 +1813,12 @@ __PACKAGE__->register_method({
> method => 'POST',
> protected => 1,
> proxyto => 'node',
> - description => "Move a rootfs-/mp-volume to a different storage",
> + description => "Move a rootfs-/mp-volume to a different storage or to a different container.",
> permissions => {
> description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, " .
> - "and 'Datastore.AllocateSpace' permissions on the storage.",
> + "and 'Datastore.AllocateSpace' permissions on the storage. To move ".
> + "a volume to another container, you need the permissions on the ".
> + "target container as well.",
> check =>
> [ 'and',
> ['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
> @@ -1828,14 +1830,20 @@ __PACKAGE__->register_method({
> properties => {
> node => get_standard_option('pve-node'),
> vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
> + 'target-vmid' => get_standard_option('pve-vmid', {
> + completion => \&PVE::LXC::complete_ctid,
> + optional => 1,
> + }),
> volume => {
> type => 'string',
> + #TODO: check how to handle unused mount points as the mp parameter is not configured
> enum => [ PVE::LXC::Config->valid_volume_keys() ],
> description => "Volume which will be moved.",
> },
> storage => get_standard_option('pve-storage-id', {
> description => "Target Storage.",
> completion => \&PVE::Storage::complete_storage_enabled,
> + optional => 1,
> }),
> delete => {
> type => 'boolean',
> @@ -1856,6 +1864,21 @@ __PACKAGE__->register_method({
> minimum => '0',
> default => 'clone limit from datacenter or storage config',
> },
> + 'target-volume' => {
> + type => 'string',
> + description => "The config key the volume will be moved to. Default is the " .
> + "source volume key.",
> + enum => [PVE::LXC::Config->valid_volume_keys()],
> + optional => 1,
> + },
> + 'target-digest' => {
> + type => 'string',
> + description => 'Prevent changes if current configuration file of the target " .
> + "container has a different SHA1 digest. This can be used to prevent " .
> + "concurrent modifications.',
> + maxLength => 40,
> + optional => 1,
> + },
> },
> },
> returns => {
> @@ -1870,32 +1893,48 @@ __PACKAGE__->register_method({
>
> my $vmid = extract_param($param, 'vmid');
>
> + my $target_vmid = extract_param($param, 'target-vmid');
> +
> my $storage = extract_param($param, 'storage');
>
> my $mpkey = extract_param($param, 'volume');
>
> + my $target_mpkey = extract_param($param, 'target-volume') // $mpkey;
> +
> + my $digest = extract_param($param, 'digest');
> +
> + my $target_digest = extract_param($param, 'target-digest');
> +
> my $lockname = 'disk';
>
> my ($mpdata, $old_volid);
>
> - PVE::LXC::Config->lock_config($vmid, sub {
> - my $conf = PVE::LXC::Config->load_config($vmid);
> - PVE::LXC::Config->check_lock($conf);
> + die "either set storage or target-vmid, but not both\n"
> + if $storage && $target_vmid;
>
> - die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid);
> + my $storecfg = PVE::Storage::config();
> + my $source_volid;
>
> - $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey});
> - $old_volid = $mpdata->{volume};
> + my $move_to_storage_checks = sub {
> + PVE::LXC::Config->lock_config($vmid, sub {
> + my $conf = PVE::LXC::Config->load_config($vmid);
> + PVE::LXC::Config->check_lock($conf);
>
> - die "you can't move a volume with snapshots and delete the source\n"
> - if $param->{delete} && PVE::LXC::Config->is_volume_in_use_by_snapshots($conf, $old_volid);
> + die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid);
>
> - PVE::Tools::assert_if_modified($param->{digest}, $conf->{digest});
> + $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey});
> + $old_volid = $mpdata->{volume};
>
> - PVE::LXC::Config->set_lock($vmid, $lockname);
> - });
> + die "you can't move a volume with snapshots and delete the source\n"
> + if $param->{delete} && PVE::LXC::Config->is_volume_in_use_by_snapshots($conf, $old_volid);
>
> - my $realcmd = sub {
> + PVE::Tools::assert_if_modified($digest, $conf->{digest});
> +
> + PVE::LXC::Config->set_lock($vmid, $lockname);
> + });
> + };
> +
> + my $storage_realcmd = sub {
> eval {
> PVE::Cluster::log_msg('info', $authuser, "move volume CT $vmid: move --volume $mpkey --storage $storage");
>
> @@ -1965,15 +2004,187 @@ __PACKAGE__->register_method({
> warn $@ if $@;
> die $err if $err;
> };
> - my $task = eval {
> - $rpcenv->fork_worker('move_volume', $vmid, $authuser, $realcmd);
> +
> + my $load_and_check_reassign_configs = sub {
> + my $vmlist = PVE::Cluster::get_vmlist()->{ids};
> +
> + die "could not find CT ${vmid}\n" if !exists($vmlist->{$vmid});
> +
> + die "Both containers need to be on the same node ($vmlist->{$vmid}->{node}) ".
> + "but target continer is on $vmlist->{$target_vmid}->{node}.\n"
> + if $vmlist->{$vmid}->{node} ne $vmlist->{$target_vmid}->{node};
> +
> + my $source_conf = PVE::LXC::Config->load_config($vmid);
> + PVE::LXC::Config->check_lock($source_conf);
> + my $target_conf = PVE::LXC::Config->load_config($target_vmid);
> + PVE::LXC::Config->check_lock($target_conf);
> +
> + die "Can't move volumes from or to template VMs\n"
> + if ($source_conf->{template} || $target_conf->{template});
> +
> + if ($digest) {
> + eval { PVE::Tools::assert_if_modified($digest, $source_conf->{digest}) };
> + die "Container ${vmid}: $@" if $@;
> + }
> +
> + if ($target_digest) {
> + eval { PVE::Tools::assert_if_modified($target_digest, $target_conf->{digest}) };
> + die "Container ${target_vmid}: $@" if $@;
> + }
> +
> + die "volume '${mpkey}' for container '$vmid' does not exist\n"
> + if !defined($source_conf->{$mpkey});
> +
> + die "Target volume key '${target_mpkey}' is already in use for container '$target_vmid'\n"
> + if exists $target_conf->{$target_mpkey};
> +
> + my $drive = PVE::LXC::Config->parse_volume(
> + $mpkey,
> + $source_conf->{$mpkey},
> + );
> +
> + $source_volid = $drive->{volume};
> +
> + die "disk '${mpkey}' has no associated volume\n"
> + if !$source_volid;
> + die "Cannot move volume used by a snapshot to another container\n"
> + if PVE::LXC::Config->is_volume_in_use_by_snapshots($source_conf, $source_volid);
> + die "Storage does not support moving of this disk to another container\n"
> + if !PVE::Storage::volume_has_feature($storecfg, 'rename', $source_volid);
> + die "Cannot move a bindmound or device mount to another container\n"
> + if $drive->{type} ne "volume";
> + die "Cannot move volume to another container while the source container is running\n"
> + if PVE::LXC::check_running($vmid) && $mpkey !~ m/^unused\d+$/;
> +
> + my $repl_conf = PVE::ReplicationConfig->new();
> + my $is_replicated = $repl_conf->check_for_existing_jobs($target_vmid, 1);
> + my ($storeid, undef) = PVE::Storage::parse_volume_id($source_volid);
> + my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
> + if (
> + $is_replicated
> + && !PVE::Storage::storage_can_replicate($storecfg, $storeid, $format)
> + ) {
> + die "Cannot move volume to a replicated container. Storage " .
> + "does not support replication!\n";
> + }
> + return ($source_conf, $target_conf);
> };
> - if (my $err = $@) {
> - eval { PVE::LXC::Config->remove_lock($vmid, $lockname) };
> - warn $@ if $@;
> - die $err;
> +
> + my $logfunc = sub {
> + my ($msg) = @_;
> + print STDERR "$msg\n";
> + };
> +
> + my $volume_reassignfn = sub {
> + return PVE::LXC::Config->lock_config($vmid, sub {
> + return PVE::LXC::Config->lock_config($target_vmid, sub {
> + my ($source_conf, $target_conf) = &$load_and_check_reassign_configs();
> + die "cannot move volumes of a running container\n"
> + if PVE::LXC::check_running($vmid);
> +
> + my $drive_param = PVE::LXC::Config->parse_volume(
> + $target_mpkey,
> + $source_conf->{$mpkey},
> + );
> +
> + print "moving volume '$mpkey' from container '$vmid' to '$target_vmid'\n";
> + my ($storage, $source_volname) = PVE::Storage::parse_volume_id($source_volid);
> +
> + my $fmt = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
> +
> + my $target_volname = PVE::Storage::find_free_volname(
> + $storecfg,
> + $storage,
> + $target_vmid,
> + $fmt
> + );
> +
> + my $new_volid = PVE::Storage::rename_volume(
> + $storecfg,
> + $source_volid,
> + $target_volname,
> + );
> +
> + $drive_param->{volume} = $new_volid;
> +
> + delete $source_conf->{$mpkey};
> + print "removing volume '${mpkey}' from container '${vmid}' config\n";
> + PVE::LXC::Config->write_config($vmid, $source_conf);
> +
> + my $drive_string = PVE::LXC::Config->print_volume($target_mpkey, $drive_param);
> + my $running = PVE::LXC::check_running($target_vmid);
> + my $param = { $target_mpkey => $drive_string };
> +
> + my $errors = PVE::LXC::Config->update_pct_config(
> + $target_vmid,
> + $target_conf,
> + $running,
> + $param
> + );
> +
> + foreach my $key (keys %$errors) {
> + $rpcenv->warn($errors->{$key});
> + }
> +
> + PVE::LXC::Config->write_config($target_vmid, $target_conf);
> + $target_conf = PVE::LXC::Config->load_config($target_vmid);
> +
> + PVE::LXC::update_lxc_config($target_vmid, $target_conf);
> + print "target container '$target_vmid' updated with '$target_mpkey'\n";
> +
> + # remove possible replication snapshots
> + if (PVE::Storage::volume_has_feature($storecfg,'replicate', $source_volid)) {
> + eval {
> + PVE::Replication::prepare(
> + $storecfg,
> + [$new_volid],
> + undef,
> + 1,
> + undef,
> + $logfunc,
> + )
> + };
> + if (my $err = $@) {
> + $rpcenv->warn("Failed to remove replication snapshots on volume ".
> + "'${target_mpkey}'. Manual cleanup could be necessary. " .
> + "Error: ${err}\n");
> + }
> + }
> + });
> + });
> + };
> +
> + if ($target_vmid) {
> + $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk'])
> + if $authuser ne 'root@pam';
> +
> + die "Moving a volume to the same container is not possible. Did you ".
> + "mean to move the volume to a different storage?\n"
> + if $vmid eq $target_vmid;
> +
> + &$load_and_check_reassign_configs();
> + return $rpcenv->fork_worker(
> + 'move_volume',
> + "${vmid}-${mpkey}>${target_vmid}-${target_mpkey}",
> + $authuser,
> + $volume_reassignfn
> + );
> + } elsif ($storage) {
> + &$move_to_storage_checks();
> + my $task = eval {
> + $rpcenv->fork_worker('move_volume', $vmid, $authuser, $storage_realcmd);
> + };
> + if (my $err = $@) {
> + eval { PVE::LXC::Config->remove_lock($vmid, $lockname) };
> + warn $@ if $@;
> + die $err;
> + }
> + return $task;
> + } else {
> + die "Provide either a 'storage' to move the mount point to a ".
> + "different storage or 'target-vmid' and 'target-mp' to move ".
> + "the mount point to another container\n";
> }
> - return $task;
> }});
>
> __PACKAGE__->register_method({
>
^ permalink raw reply [flat|nested] 15+ messages in thread* [pve-devel] [PATCH v4 container 8/9 (follow up)] api: move-volume: add move to another container
2021-11-02 15:03 ` [pve-devel] [PATCH v4 container 8/9] api: move-volume: add move to another container Aaron Lauterer
2021-11-02 16:44 ` Aaron Lauterer
@ 2021-11-05 14:56 ` Aaron Lauterer
1 sibling, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-05 14:56 UTC (permalink / raw)
To: pve-devel
The goal of this is to expand the move-volume API endpoint to make it
possible to move a container volume / mountpoint to another container.
For unused volumes, the API parameters have been changed to allow them
as well. This means, additional checks had to be introduced to avoid
migration of an unusedX volume to another storage. Some follow up work
is needed for that to work properly.
Moving the rootfs from or to another container is prohibited.
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
In this follow up I introduce the capability to allow moving 'unusedX'
volumes from one container to another.
This includes:
* changing the API parameters to allow for 'unused' config keys with the
needed method in the LXC/Config.pm to enumerate them.
* added a check to prevent unused volumes to be migrated to other
storages. Needs some more work to work properly and is on my todo
* the $volume_reassignfn has ssen the most changes as unused volumes
need to be handled differently than 'mpX' ones when adding them to the
target CT (writing directly to the config, like done when doing a
'rescan')
Other changes that happened:
* added checks to prevent the rootfs to/from a container
* removed a double check if the container is running
* rephrased an occurence of 'disk' to 'volume'
src/PVE/API2/LXC.pm | 276 ++++++++++++++++++++++++++++++++++++++----
src/PVE/LXC/Config.pm | 9 ++
2 files changed, 263 insertions(+), 22 deletions(-)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 69df366..9c760bc 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1813,10 +1813,12 @@ __PACKAGE__->register_method({
method => 'POST',
protected => 1,
proxyto => 'node',
- description => "Move a rootfs-/mp-volume to a different storage",
+ description => "Move a rootfs-/mp-volume to a different storage or to a different container.",
permissions => {
description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, " .
- "and 'Datastore.AllocateSpace' permissions on the storage.",
+ "and 'Datastore.AllocateSpace' permissions on the storage. To move ".
+ "a volume to another container, you need the permissions on the ".
+ "target container as well.",
check =>
[ 'and',
['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
@@ -1828,14 +1830,20 @@ __PACKAGE__->register_method({
properties => {
node => get_standard_option('pve-node'),
vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid }),
+ 'target-vmid' => get_standard_option('pve-vmid', {
+ completion => \&PVE::LXC::complete_ctid,
+ optional => 1,
+ }),
volume => {
type => 'string',
- enum => [ PVE::LXC::Config->valid_volume_keys() ],
+ #TODO: check how to handle unused mount points as the mp parameter is not configured
+ enum => [ PVE::LXC::Config->valid_volume_keys_with_unused() ],
description => "Volume which will be moved.",
},
storage => get_standard_option('pve-storage-id', {
description => "Target Storage.",
completion => \&PVE::Storage::complete_storage_enabled,
+ optional => 1,
}),
delete => {
type => 'boolean',
@@ -1856,6 +1864,21 @@ __PACKAGE__->register_method({
minimum => '0',
default => 'clone limit from datacenter or storage config',
},
+ 'target-volume' => {
+ type => 'string',
+ description => "The config key the volume will be moved to. Default is the " .
+ "source volume key.",
+ enum => [PVE::LXC::Config->valid_volume_keys_with_unused()],
+ optional => 1,
+ },
+ 'target-digest' => {
+ type => 'string',
+ description => 'Prevent changes if current configuration file of the target " .
+ "container has a different SHA1 digest. This can be used to prevent " .
+ "concurrent modifications.',
+ maxLength => 40,
+ optional => 1,
+ },
},
},
returns => {
@@ -1870,32 +1893,54 @@ __PACKAGE__->register_method({
my $vmid = extract_param($param, 'vmid');
+ my $target_vmid = extract_param($param, 'target-vmid');
+
my $storage = extract_param($param, 'storage');
my $mpkey = extract_param($param, 'volume');
+ my $target_mpkey = extract_param($param, 'target-volume') // $mpkey;
+
+ my $digest = extract_param($param, 'digest');
+
+ my $target_digest = extract_param($param, 'target-digest');
+
my $lockname = 'disk';
my ($mpdata, $old_volid);
- PVE::LXC::Config->lock_config($vmid, sub {
- my $conf = PVE::LXC::Config->load_config($vmid);
- PVE::LXC::Config->check_lock($conf);
+ die "either set storage or target-vmid, but not both\n"
+ if $storage && $target_vmid;
- die "cannot move volumes of a running container\n" if PVE::LXC::check_running($vmid);
+ my $storecfg = PVE::Storage::config();
+ my $source_volid;
- $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey});
- $old_volid = $mpdata->{volume};
+ my $move_to_storage_checks = sub {
+ PVE::LXC::Config->lock_config($vmid, sub {
+ my $conf = PVE::LXC::Config->load_config($vmid);
+ PVE::LXC::Config->check_lock($conf);
- die "you can't move a volume with snapshots and delete the source\n"
- if $param->{delete} && PVE::LXC::Config->is_volume_in_use_by_snapshots($conf, $old_volid);
+ die "cannot move volumes of a running container\n"
+ if PVE::LXC::check_running($vmid);
- PVE::Tools::assert_if_modified($param->{digest}, $conf->{digest});
+ if ($mpkey =~ m/^unused\d+$/) {
+ die "cannot move volume '$mpkey', only configured volumes can be moved to ".
+ "another storage\n";
+ }
- PVE::LXC::Config->set_lock($vmid, $lockname);
- });
+ $mpdata = PVE::LXC::Config->parse_volume($mpkey, $conf->{$mpkey});
+ $old_volid = $mpdata->{volume};
- my $realcmd = sub {
+ die "you can't move a volume with snapshots and delete the source\n"
+ if $param->{delete} && PVE::LXC::Config->is_volume_in_use_by_snapshots($conf, $old_volid);
+
+ PVE::Tools::assert_if_modified($digest, $conf->{digest});
+
+ PVE::LXC::Config->set_lock($vmid, $lockname);
+ });
+ };
+
+ my $storage_realcmd = sub {
eval {
PVE::Cluster::log_msg('info', $authuser, "move volume CT $vmid: move --volume $mpkey --storage $storage");
@@ -1965,15 +2010,202 @@ __PACKAGE__->register_method({
warn $@ if $@;
die $err if $err;
};
- my $task = eval {
- $rpcenv->fork_worker('move_volume', $vmid, $authuser, $realcmd);
+
+ my $load_and_check_reassign_configs = sub {
+ my $vmlist = PVE::Cluster::get_vmlist()->{ids};
+
+ die "Cannot move to/from 'rootfs'\n" if $mpkey eq "rootfs" || $target_mpkey eq "rootfs";
+
+ if ($mpkey =~ m/^unused\d+$/ && $target_mpkey !~ m/^unused\d+$/) {
+ die "Moving an unused volume to a used one is not possible\n";
+ }
+ die "could not find CT ${vmid}\n" if !exists($vmlist->{$vmid});
+
+ die "Both containers need to be on the same node ($vmlist->{$vmid}->{node}) ".
+ "but target continer is on $vmlist->{$target_vmid}->{node}.\n"
+ if $vmlist->{$vmid}->{node} ne $vmlist->{$target_vmid}->{node};
+
+ my $source_conf = PVE::LXC::Config->load_config($vmid);
+ PVE::LXC::Config->check_lock($source_conf);
+ my $target_conf = PVE::LXC::Config->load_config($target_vmid);
+ PVE::LXC::Config->check_lock($target_conf);
+
+ die "Can't move volumes from or to template VMs\n"
+ if ($source_conf->{template} || $target_conf->{template});
+
+ if ($digest) {
+ eval { PVE::Tools::assert_if_modified($digest, $source_conf->{digest}) };
+ die "Container ${vmid}: $@" if $@;
+ }
+
+ if ($target_digest) {
+ eval { PVE::Tools::assert_if_modified($target_digest, $target_conf->{digest}) };
+ die "Container ${target_vmid}: $@" if $@;
+ }
+
+ die "volume '${mpkey}' for container '$vmid' does not exist\n"
+ if !defined($source_conf->{$mpkey});
+
+ die "Target volume key '${target_mpkey}' is already in use for container '$target_vmid'\n"
+ if exists $target_conf->{$target_mpkey};
+
+ my $drive = PVE::LXC::Config->parse_volume(
+ $mpkey,
+ $source_conf->{$mpkey},
+ );
+
+ $source_volid = $drive->{volume};
+
+ die "Volume '${mpkey}' has no associated image\n"
+ if !$source_volid;
+ die "Cannot move volume used by a snapshot to another container\n"
+ if PVE::LXC::Config->is_volume_in_use_by_snapshots($source_conf, $source_volid);
+ die "Storage does not support moving of this disk to another container\n"
+ if !PVE::Storage::volume_has_feature($storecfg, 'rename', $source_volid);
+ die "Cannot move a bindmound or device mount to another container\n"
+ if $drive->{type} ne "volume";
+ die "Cannot move volume to another container while the source container is running\n"
+ if PVE::LXC::check_running($vmid) && $mpkey !~ m/^unused\d+$/;
+
+ my $repl_conf = PVE::ReplicationConfig->new();
+ my $is_replicated = $repl_conf->check_for_existing_jobs($target_vmid, 1);
+ my ($storeid, undef) = PVE::Storage::parse_volume_id($source_volid);
+ my $format = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
+ if (
+ $is_replicated
+ && !PVE::Storage::storage_can_replicate($storecfg, $storeid, $format)
+ ) {
+ die "Cannot move volume to a replicated container. Storage " .
+ "does not support replication!\n";
+ }
+ return ($source_conf, $target_conf);
+ };
+
+ my $logfunc = sub {
+ my ($msg) = @_;
+ print STDERR "$msg\n";
};
- if (my $err = $@) {
- eval { PVE::LXC::Config->remove_lock($vmid, $lockname) };
- warn $@ if $@;
- die $err;
+
+ my $volume_reassignfn = sub {
+ return PVE::LXC::Config->lock_config($vmid, sub {
+ return PVE::LXC::Config->lock_config($target_vmid, sub {
+ my ($source_conf, $target_conf) = &$load_and_check_reassign_configs();
+
+ my $target_unused = $target_mpkey =~ m/^unused\d+$/;
+
+ my $drive_param = PVE::LXC::Config->parse_volume(
+ $mpkey,
+ $source_conf->{$mpkey},
+ );
+
+ print "moving volume '$mpkey' from container '$vmid' to '$target_vmid'\n";
+ my ($storage, $source_volname) = PVE::Storage::parse_volume_id($source_volid);
+
+ my $fmt = (PVE::Storage::parse_volname($storecfg, $source_volid))[6];
+
+ my $target_volname = PVE::Storage::find_free_volname(
+ $storecfg,
+ $storage,
+ $target_vmid,
+ $fmt
+ );
+
+ my $new_volid = PVE::Storage::rename_volume(
+ $storecfg,
+ $source_volid,
+ $target_volname,
+ );
+
+ $drive_param->{volume} = $new_volid;
+
+ delete $source_conf->{$mpkey};
+ print "removing volume '${mpkey}' from container '${vmid}' config\n";
+ PVE::LXC::Config->write_config($vmid, $source_conf);
+
+ my $drive_string;
+
+ if ($target_unused) {
+ $drive_string = $new_volid;
+ } else {
+ $drive_string = PVE::LXC::Config->print_volume($target_mpkey, $drive_param);
+ }
+
+ if ($target_unused) {
+ $target_conf->{$target_mpkey} = $drive_string;
+ } else {
+ my $running = PVE::LXC::check_running($target_vmid);
+ my $param = { $target_mpkey => $drive_string };
+ my $errors = PVE::LXC::Config->update_pct_config(
+ $target_vmid,
+ $target_conf,
+ $running,
+ $param
+ );
+
+ foreach my $key (keys %$errors) {
+ $rpcenv->warn($errors->{$key});
+ }
+ }
+
+ PVE::LXC::Config->write_config($target_vmid, $target_conf);
+ $target_conf = PVE::LXC::Config->load_config($target_vmid);
+
+ PVE::LXC::update_lxc_config($target_vmid, $target_conf) if !$target_unused;
+ print "target container '$target_vmid' updated with '$target_mpkey'\n";
+
+ # remove possible replication snapshots
+ if (PVE::Storage::volume_has_feature($storecfg,'replicate', $source_volid)) {
+ eval {
+ PVE::Replication::prepare(
+ $storecfg,
+ [$new_volid],
+ undef,
+ 1,
+ undef,
+ $logfunc,
+ )
+ };
+ if (my $err = $@) {
+ $rpcenv->warn("Failed to remove replication snapshots on volume ".
+ "'${target_mpkey}'. Manual cleanup could be necessary. " .
+ "Error: ${err}\n");
+ }
+ }
+ });
+ });
+ };
+
+ if ($target_vmid) {
+ $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk'])
+ if $authuser ne 'root@pam';
+
+ die "Moving a volume to the same container is not possible. Did you ".
+ "mean to move the volume to a different storage?\n"
+ if $vmid eq $target_vmid;
+
+ &$load_and_check_reassign_configs();
+ return $rpcenv->fork_worker(
+ 'move_volume',
+ "${vmid}-${mpkey}>${target_vmid}-${target_mpkey}",
+ $authuser,
+ $volume_reassignfn
+ );
+ } elsif ($storage) {
+ &$move_to_storage_checks();
+ my $task = eval {
+ $rpcenv->fork_worker('move_volume', $vmid, $authuser, $storage_realcmd);
+ };
+ if (my $err = $@) {
+ eval { PVE::LXC::Config->remove_lock($vmid, $lockname) };
+ warn $@ if $@;
+ die $err;
+ }
+ return $task;
+ } else {
+ die "Provide either a 'storage' to move the mount point to a ".
+ "different storage or 'target-vmid' and 'target-mp' to move ".
+ "the mount point to another container\n";
}
- return $task;
}});
__PACKAGE__->register_method({
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 8557e4c..8a25343 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1556,6 +1556,15 @@ sub valid_volume_keys {
return $reverse ? reverse @names : @names;
}
+sub valid_volume_keys_with_unused {
+ my ($class, $reverse) = @_;
+ my @names = $class->valid_volume_keys();
+ for (my $i = 0; $i < $MAX_UNUSED_DISKS; $i++) {
+ push @names, "unused$i";
+ }
+ return $reverse ? reverse @names : @names;
+}
+
sub get_vm_volumes {
my ($class, $conf, $excludes) = @_;
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [pve-devel] [PATCH v4 container 9/9] api: move-volume: cleanup very long lines
2021-11-02 15:03 [pve-devel] [PATCH v4 storage qemu-server container 0/9] move disk or volume to other guests Aaron Lauterer
` (7 preceding siblings ...)
2021-11-02 15:03 ` [pve-devel] [PATCH v4 container 8/9] api: move-volume: add move to another container Aaron Lauterer
@ 2021-11-02 15:03 ` Aaron Lauterer
8 siblings, 0 replies; 15+ messages in thread
From: Aaron Lauterer @ 2021-11-02 15:03 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
src/PVE/API2/LXC.pm | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index afc16d7..d8afbbc 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -1847,13 +1847,15 @@ __PACKAGE__->register_method({
}),
delete => {
type => 'boolean',
- description => "Delete the original volume after successful copy. By default the original is kept as an unused volume entry.",
+ description => "Delete the original volume after successful copy. By default the " .
+ "original is kept as an unused volume entry.",
optional => 1,
default => 0,
},
digest => {
type => 'string',
- description => 'Prevent changes if current configuration file has different SHA1 digest. This can be used to prevent concurrent modifications.',
+ description => 'Prevent changes if current configuration file has different SHA1 " .
+ "digest. This can be used to prevent concurrent modifications.',
maxLength => 40,
optional => 1,
},
@@ -1936,7 +1938,11 @@ __PACKAGE__->register_method({
my $storage_realcmd = sub {
eval {
- PVE::Cluster::log_msg('info', $authuser, "move volume CT $vmid: move --volume $mpkey --storage $storage");
+ PVE::Cluster::log_msg(
+ 'info',
+ $authuser,
+ "move volume CT $vmid: move --volume $mpkey --storage $storage"
+ );
my $conf = PVE::LXC::Config->load_config($vmid);
my $storage_cfg = PVE::Storage::config();
@@ -1947,8 +1953,20 @@ __PACKAGE__->register_method({
PVE::Storage::activate_volumes($storage_cfg, [ $old_volid ]);
my $bwlimit = extract_param($param, 'bwlimit');
my $source_storage = PVE::Storage::parse_volume_id($old_volid);
- my $movelimit = PVE::Storage::get_bandwidth_limit('move', [$source_storage, $storage], $bwlimit);
- $new_volid = PVE::LXC::copy_volume($mpdata, $vmid, $storage, $storage_cfg, $conf, undef, $movelimit);
+ my $movelimit = PVE::Storage::get_bandwidth_limit(
+ 'move',
+ [$source_storage, $storage],
+ $bwlimit
+ );
+ $new_volid = PVE::LXC::copy_volume(
+ $mpdata,
+ $vmid,
+ $storage,
+ $storage_cfg,
+ $conf,
+ undef,
+ $movelimit
+ );
if (PVE::LXC::Config->is_template($conf)) {
PVE::Storage::activate_volumes($storage_cfg, [ $new_volid ]);
my $template_volid = PVE::Storage::vdisk_create_base($storage_cfg, $new_volid);
@@ -1962,7 +1980,10 @@ __PACKAGE__->register_method({
$conf = PVE::LXC::Config->load_config($vmid);
PVE::Tools::assert_if_modified($digest, $conf->{digest});
- $conf->{$mpkey} = PVE::LXC::Config->print_ct_mountpoint($mpdata, $mpkey eq 'rootfs');
+ $conf->{$mpkey} = PVE::LXC::Config->print_ct_mountpoint(
+ $mpdata,
+ $mpkey eq 'rootfs'
+ );
PVE::LXC::Config->add_unused_volume($conf, $old_volid) if !$param->{delete};
--
2.30.2
^ permalink raw reply [flat|nested] 15+ messages in thread