[pve-devel] [PATCH many] fix #1905: Allow moving unused disks

[pve-devel] [PATCH many] fix #1905: Allow moving unused disks

[Filip Schauer]

Allow moving unused/detached disks to another storage.

qemu-server:

Filip Schauer (1):
  fix #1905: Allow moving unused disks

 PVE/API2/Qemu.pm  | 3 ---
 PVE/QemuServer.pm | 5 +++--
 2 files changed, 3 insertions(+), 5 deletions(-)

pve-manager:

Filip Schauer (1):
  Allow moving unused disks to another storage

 www/manager6/qemu/HardwareView.js | 1 -
 1 file changed, 1 deletion(-)

-- 
2.39.2

[pve-devel] [PATCH qemu-server 1/1] fix #1905: Allow moving unused disks

[Filip Schauer]

Allow moving unused/detached disks to another storage.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 PVE/API2/Qemu.pm  | 3 ---
 PVE/QemuServer.pm | 5 +++--
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 69c5896..97216a3 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4283,9 +4283,6 @@ __PACKAGE__->register_method({
 	} elsif ($storeid) {
 	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
 
-	    die "cannot move disk '$disk', only configured disks can be moved to another storage\n"
-		if $disk =~ m/^unused\d+$/;
-
 	    $load_and_check_move->(); # early checks before forking/locking
 
 	    my $realcmd = sub {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b45507a..4527276 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8036,7 +8036,8 @@ sub clone_disk {
     my ($newvmid, $dst_drivename, $efisize) = $dest->@{qw(vmid drivename efisize)};
     my ($storage, $format) = $dest->@{qw(storage format)};
 
-    my $use_drive_mirror = $full && $running && $src_drivename && !$snapname;
+    my $unused = $src_drivename =~ /^unused/;
+    my $use_drive_mirror = $full && $running && $src_drivename && !$snapname && !$unused;
 
     if ($src_drivename && $dst_drivename && $src_drivename ne $dst_drivename) {
 	die "cloning from/to EFI disk requires EFI disk\n"
@@ -8142,7 +8143,7 @@ no_data_clone:
     my $disk = dclone($drive);
     delete $disk->{format};
     $disk->{file} = $newvolid;
-    $disk->{size} = $size if defined($size);
+    $disk->{size} = $size if defined($size) && !$unused;
 
     return $disk;
 }
-- 
2.39.2

Re: [pve-devel] [PATCH qemu-server 1/1] fix #1905: Allow moving unused disks

[Thomas Lamprecht]

Am 19/02/2024 um 12:11 schrieb Filip Schauer:
> Allow moving unused/detached disks to another storage.

this is a repetition of the commit subject, while that is on it's
own OK, I'd rather see a description about why this is OK to do, i.e.,
why was the original check added, what changed since then, what are
the drawbacks, e.g., as the QEMU block-mirror cannot be used I imagine
only a limited set of source storage to target storage can be used
for this?

Re: [pve-devel] [PATCH qemu-server 1/1] fix #1905: Allow moving unused disks

[Filip Schauer]

After a bit of research, here is a more descriptive commit message:


Allow moving unused disks to another storage

In the past, moving unused disks to another storage was prohibited due
to oversights in the handling of unused disks. This commit rectifies
this limitation by allowing the movement of unused disks.

Historical context:
* 16 Sep 2010 r5164 qemu-server/pve2: The disknames sub was removed.
* 17 Sep 2010 r5170 qemu-server/pve2: Unused disks were introduced.
* 28 Jan 2011 r5461 qemu-server/pve2: The same disknames sub that was
   removed in r5164 was brought back. Since unused disks were not around
   yet in r5164 the disknames sub did not consider unused disks.
* 6-8 Aug 2012 c1175c92..f91b2e45 qemu-server.git: Disk resize was
   introduced. In commit c1175c92 in sub qemu_block_resize unused disks
   were not taken into account and in commit 2f48a4f5 (8 Aug 2012) the
   resize API call was changed to only allow disks matching the ones in
   the disknames sub. Since sub disknames did not contain any unused
   disks, those were not allowed at all in the resize API call.
* 27 May 2013 586bfa78 qemu-server.git: Disk move was introduced. The
   API call implementation borrowed heavily from disk resize, including
   the behaviour of not taking unused disks into account. Thus, unused
   disk could not be moved, which persists to this day.

In summary, this behaviour was introduced because the handling of unused
disks was overlooked and it was never changed.

There is no inherent reason why unused disks should be restricted from
being moved to another storage. These disks cannot use the
qemu_drive_mirror, but they can still be moved with qemu_img_convert,
the same way as any other disk of a stopped VM.


On 19/02/2024 13:22, Thomas Lamprecht wrote:
> Am 19/02/2024 um 12:11 schrieb Filip Schauer:
>> Allow moving unused/detached disks to another storage.
> this is a repetition of the commit subject, while that is on it's
> own OK, I'd rather see a description about why this is OK to do, i.e.,
> why was the original check added, what changed since then, what are
> the drawbacks, e.g., as the QEMU block-mirror cannot be used I imagine
> only a limited set of source storage to target storage can be used
> for this?

[pve-devel] [PATCH manager 1/1] Allow moving unused disks to another storage

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 1 -
 1 file changed, 1 deletion(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 5b33b1e2..672a7e1a 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -634,7 +634,6 @@ Ext.define('PVE.qemu.HardwareView', {
 		isCloudInit ||
 		!(isDisk || isEfi || tpmMoveable),
 	    );
-	    move_menuitem.setDisabled(isUnusedDisk);
 	    reassign_menuitem.setDisabled(pending || (isEfi || tpmMoveable));
 	    resize_menuitem.setDisabled(pending || !isUsedDisk);
 
-- 
2.39.2

[pve-devel] applied-series: [PATCH many] fix #1905: Allow moving unused disks

[Fabian Grünbichler]

with the re-worded commit message for qemu-server (although I dropped
the still redundant first line of the commit message ;))

On February 19, 2024 12:11 pm, Filip Schauer wrote:
> Allow moving unused/detached disks to another storage.
> 
> qemu-server:
> 
> Filip Schauer (1):
>   fix #1905: Allow moving unused disks
> 
>  PVE/API2/Qemu.pm  | 3 ---
>  PVE/QemuServer.pm | 5 +++--
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> pve-manager:
> 
> Filip Schauer (1):
>   Allow moving unused disks to another storage
> 
>  www/manager6/qemu/HardwareView.js | 1 -
>  1 file changed, 1 deletion(-)
> 
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>