[pve-devel] [PATCH storage/qemu-server/manager v3 0/4] restore with a specified file format

[pve-devel] [PATCH storage/qemu-server/manager v3 0/4] restore with a specified file format

[Markus Frank]

This patch series allows to restore all VM disks with a specified format
if supported by the target storage. The existing storage and the new
disk-format options can act as a default/fallback for per disk
storage/format customisation in the future (#4275).

v3:
* see individual patches

v2:
* renamed diskformat to disk-format
* added storage patch to a new standard option that only allows raw,
 qcow2 and vmdk



storage:

Markus Frank (1):
  add standard option for VM disk formats in file-based storages

 src/PVE/Storage/Plugin.pm | 6 ++++++
 1 file changed, 6 insertions(+)



qemu-server:

Markus Frank (1):
  fix 4888: qmrestore: add disk-format option

 PVE/API2/Qemu.pm     |  7 +++++++
 PVE/CLI/qmrestore.pm |  5 +++++
 PVE/QemuServer.pm    | 14 +++++++++-----
 3 files changed, 21 insertions(+), 5 deletions(-)



manager:

Markus Frank (2):
  ui: add hideFormatWhenStorageEmpty option to DiskStorageSelector
  ui: restore window: add disk-format option

 www/manager6/form/DiskStorageSelector.js | 12 +++++++++++-
 www/manager6/window/Restore.js           | 14 +++++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH storage v3 1/4] add standard option for VM disk formats in file-based storages

[Markus Frank]

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
v3:
* removed 'optional => 1'
* changed description

 src/PVE/Storage/Plugin.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 65cf43f..c15dc44 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -354,6 +354,12 @@ sub verify_format {
     return $fmt;
 }
 
+PVE::JSONSchema::register_standard_option('pve-vm-image-format', {
+    type => 'string',
+    enum => ['raw', 'qcow2', 'vmdk'],
+    description => "VM image formats.",
+});
+
 PVE::JSONSchema::register_format('pve-storage-options', \&verify_options);
 sub verify_options {
     my ($value, $noerr) = @_;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH storage v3 1/4] add standard option for VM disk formats in file-based storages

[Fiona Ebner]

Am 07.03.25 um 10:25 schrieb Markus Frank:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> v3:
> * removed 'optional => 1'
> * changed description
> 
>  src/PVE/Storage/Plugin.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 65cf43f..c15dc44 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm

We now have a pve-storage-image-format in Storage/Common.pm. This new
option should be placed alongside that one and POD documentation added.

> @@ -354,6 +354,12 @@ sub verify_format {
>      return $fmt;
>  }
>  
> +PVE::JSONSchema::register_standard_option('pve-vm-image-format', {
> +    type => 'string',
> +    enum => ['raw', 'qcow2', 'vmdk'],
> +    description => "VM image formats.",

This is the user-facing description. Such an option is for selecting a
single format, so:
s/formats/format/

> +});
> +
>  PVE::JSONSchema::register_format('pve-storage-options', \&verify_options);
>  sub verify_options {
>      my ($value, $noerr) = @_;



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server v3 2/4] fix 4888: qmrestore: add disk-format option

[Markus Frank]

Add an option to choose a file format (qcow2, raw, vmdk) when restoring
a vm backup to file based storage. This options allows all disks to be
recreated with the specified file format if supported by the target
storage.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
v3:
* added requires => 'archive' to disk-format
* changed descriptions
* added missing imports/uses
* check if disk_format is in validFormats

 PVE/API2/Qemu.pm     |  7 +++++++
 PVE/CLI/qmrestore.pm |  5 +++++
 PVE/QemuServer.pm    | 14 +++++++++-----
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 5ac61aa5..3f4d19cc 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1013,6 +1013,11 @@ __PACKAGE__->register_method({
 		    default => 0,
 		    description => "Start VM after it was created successfully.",
 		},
+		'disk-format' => get_standard_option('pve-vm-image-format', {
+		    optional => 1,
+		    requires => 'archive',
+		    description => "Preferred image format used for all VM disks when restoring.",
+		}),
 		'import-working-storage' => get_standard_option('pve-storage-id', {
 		    description => "A file-based storage with 'images' content-type enabled, which"
 			." is used as an intermediary extraction storage during import. Defaults to"
@@ -1046,6 +1051,7 @@ __PACKAGE__->register_method({
 	my $storage = extract_param($param, 'storage');
 	my $unique = extract_param($param, 'unique');
 	my $live_restore = extract_param($param, 'live-restore');
+	my $disk_format = extract_param($param, 'disk-format');
 	my $extraction_storage = extract_param($param, 'import-working-storage');
 
 	if (defined(my $ssh_keys = $param->{sshkeys})) {
@@ -1143,6 +1149,7 @@ __PACKAGE__->register_method({
 		    unique => $unique,
 		    bwlimit => $bwlimit,
 		    live => $live_restore,
+		    disk_format => $disk_format,
 		    override_conf => $param,
 		};
 		if (my $volid = $archive->{volid}) {
diff --git a/PVE/CLI/qmrestore.pm b/PVE/CLI/qmrestore.pm
index a47648bd..9910f30d 100755
--- a/PVE/CLI/qmrestore.pm
+++ b/PVE/CLI/qmrestore.pm
@@ -3,6 +3,7 @@ package PVE::CLI::qmrestore;
 use strict;
 use warnings;
 use PVE::SafeSyslog;
+use PVE::Storage;
 use PVE::Tools qw(extract_param);
 use PVE::INotify;
 use PVE::RPCEnvironment;
@@ -64,6 +65,10 @@ __PACKAGE__->register_method({
 		type => 'boolean',
 		description => "Start the VM immediately from the backup and restore in background. PBS only.",
 	    },
+	    'disk-format' => get_standard_option('pve-vm-image-format', {
+		optional => 1,
+		description => "Preferred image format used for all VM disks when restoring.",
+	    }),
 	},
     },
     returns => {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b6fc1f17..dbc7e6be 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6677,7 +6677,7 @@ my $parse_backup_hints = sub {
 #
 # Returns: { $virtdev => $volid }
 my $restore_allocate_devices = sub {
-    my ($storecfg, $virtdev_hash, $vmid) = @_;
+    my ($storecfg, $virtdev_hash, $vmid, $disk_format) = @_;
 
     my $map = {};
     foreach my $virtdev (sort keys %$virtdev_hash) {
@@ -6689,8 +6689,12 @@ my $restore_allocate_devices = sub {
 
 	# test if requested format is supported
 	my ($defFormat, $validFormats) = PVE::Storage::storage_default_format($storecfg, $storeid);
-	my $supported = grep { $_ eq $d->{format} } @$validFormats;
-	$d->{format} = $defFormat if !$supported;
+
+	if ($disk_format && grep { $_ eq $disk_format } @$validFormats) {
+	    $d->{format} = $disk_format;
+	} elsif (!(grep { $_ eq $d->{format} } @$validFormats)) {
+	    $d->{format} = $defFormat;
+	}
 
 	my $name;
 	if ($d->{is_cloudinit}) {
@@ -7064,7 +7068,7 @@ sub restore_proxmox_backup_archive {
 	$restore_cleanup_oldconf->($storecfg, $vmid, $oldconf, $virtdev_hash) if $oldconf;
 
 	# allocate volumes
-	my $map = $restore_allocate_devices->($storecfg, $virtdev_hash, $vmid);
+	my $map = $restore_allocate_devices->($storecfg, $virtdev_hash, $vmid, $options->{disk_format});
 
 	foreach my $virtdev (sort keys %$virtdev_hash) {
 	    my $d = $virtdev_hash->{$virtdev};
@@ -7461,7 +7465,7 @@ sub restore_vma_archive {
 	}
 
 	# allocate volumes
-	my $map = $restore_allocate_devices->($cfg, $virtdev_hash, $vmid);
+	my $map = $restore_allocate_devices->($cfg, $virtdev_hash, $vmid, $opts->{disk_format});
 
 	# print restore information to $fifofh
 	foreach my $virtdev (sort keys %$virtdev_hash) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server v3 2/4] fix 4888: qmrestore: add disk-format option

[Fiona Ebner]

Am 07.03.25 um 10:25 schrieb Markus Frank:
> Add an option to choose a file format (qcow2, raw, vmdk) when restoring
> a vm backup to file based storage. This options allows all disks to be
> recreated with the specified file format if supported by the target
> storage.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>

Looks good to me, but some slight changes would be nice, see below:

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> ---
> v3:
> * added requires => 'archive' to disk-format
> * changed descriptions
> * added missing imports/uses
> * check if disk_format is in validFormats
> 
>  PVE/API2/Qemu.pm     |  7 +++++++
>  PVE/CLI/qmrestore.pm |  5 +++++
>  PVE/QemuServer.pm    | 14 +++++++++-----
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 5ac61aa5..3f4d19cc 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm

Should add an include for PVE::Storage::Common if you make the requested
change for 1/4. I guess it would be inherited implicitly, but it's nicer
to explicitly include it.

> diff --git a/PVE/CLI/qmrestore.pm b/PVE/CLI/qmrestore.pm
> index a47648bd..9910f30d 100755
> --- a/PVE/CLI/qmrestore.pm
> +++ b/PVE/CLI/qmrestore.pm
> @@ -3,6 +3,7 @@ package PVE::CLI::qmrestore;
>  use strict;
>  use warnings;
>  use PVE::SafeSyslog;
> +use PVE::Storage;

Should be PVE::Storage::Common if you make the requested change for 1/4.

> @@ -7064,7 +7068,7 @@ sub restore_proxmox_backup_archive {
>  	$restore_cleanup_oldconf->($storecfg, $vmid, $oldconf, $virtdev_hash) if $oldconf;
>  
>  	# allocate volumes
> -	my $map = $restore_allocate_devices->($storecfg, $virtdev_hash, $vmid);
> +	my $map = $restore_allocate_devices->($storecfg, $virtdev_hash, $vmid, $options->{disk_format});
>  
>  	foreach my $virtdev (sort keys %$virtdev_hash) {
>  	    my $d = $virtdev_hash->{$virtdev};
> @@ -7461,7 +7465,7 @@ sub restore_vma_archive {
>  	}
>  
>  	# allocate volumes
> -	my $map = $restore_allocate_devices->($cfg, $virtdev_hash, $vmid);
> +	my $map = $restore_allocate_devices->($cfg, $virtdev_hash, $vmid, $opts->{disk_format});
>  
>  	# print restore information to $fifofh
>  	foreach my $virtdev (sort keys %$virtdev_hash) {

There's a restore_external_archive() function that also calls
$restore_allocate_devices->() now which should be adapted too.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager v3 3/4] ui: add hideFormatWhenStorageEmpty option to DiskStorageSelector

[Markus Frank]

Prerequisite for "ui: restore window: add diskformat option"

The hide condition is copied from the format selector item in the same
file.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
v3:
* added (me.hideFormatWhenStorageEmpty && !me.autoSelect) to the hide
condition in initComponent instead of manually hiding it in the next
patch (4/4).

 www/manager6/form/DiskStorageSelector.js | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/www/manager6/form/DiskStorageSelector.js b/www/manager6/form/DiskStorageSelector.js
index 0ef48f51..37cb8b4b 100644
--- a/www/manager6/form/DiskStorageSelector.js
+++ b/www/manager6/form/DiskStorageSelector.js
@@ -30,6 +30,7 @@ Ext.define('PVE.form.DiskStorageSelector', {
 
     // hides the format field (e.g. for TPM state)
     hideFormat: false,
+    hideFormatWhenStorageEmpty: false,
 
     // sets the initial size value
     // string because else we get a type confusion
@@ -47,6 +48,9 @@ Ext.define('PVE.form.DiskStorageSelector', {
 	    hdfilesel.setVisible(false);
 
 	    formatsel.setDisabled(true);
+	    if (me.hideFormatWhenStorageEmpty) {
+		formatsel.setVisible(false);
+	    }
 	    return;
 	}
 
@@ -73,6 +77,11 @@ Ext.define('PVE.form.DiskStorageSelector', {
 
 	var select = !!rec.data.select_existing && !me.hideSelection;
 
+	// This is needed to make the format selector visible
+	// after it has been hidden because of hideFormatWhenStorageEmpty.
+	let hideFormatCondition = me.hideFormat || me.storageContent === 'rootdir';
+	formatsel.setVisible(!hideFormatCondition);
+
 	formatsel.setDisabled(me.hideFormat || Ext.Object.getSize(validFormats) <= 1);
 	formatsel.setValue(selectFormat);
 
@@ -161,7 +170,8 @@ Ext.define('PVE.form.DiskStorageSelector', {
 		fieldLabel: gettext('Format'),
 		nodename: me.nodename,
 		disabled: true,
-		hidden: me.hideFormat || me.storageContent === 'rootdir',
+		hidden: me.hideFormat || me.storageContent === 'rootdir'
+		    || (me.hideFormatWhenStorageEmpty && !me.autoSelect),
 		value: 'qcow2',
 		allowBlank: false,
 	    },
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager v3 3/4] ui: add hideFormatWhenStorageEmpty option to DiskStorageSelector

[Fiona Ebner]

Am 07.03.25 um 10:25 schrieb Markus Frank:
> Prerequisite for "ui: restore window: add diskformat option"
> 
> The hide condition is copied from the format selector item in the same
> file.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>

With the eslint issue below fixed:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> ---
> v3:
> * added (me.hideFormatWhenStorageEmpty && !me.autoSelect) to the hide
> condition in initComponent instead of manually hiding it in the next
> patch (4/4).
> 
>  www/manager6/form/DiskStorageSelector.js | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/form/DiskStorageSelector.js b/www/manager6/form/DiskStorageSelector.js
> index 0ef48f51..37cb8b4b 100644
> --- a/www/manager6/form/DiskStorageSelector.js
> +++ b/www/manager6/form/DiskStorageSelector.js
> @@ -30,6 +30,7 @@ Ext.define('PVE.form.DiskStorageSelector', {
>  
>      // hides the format field (e.g. for TPM state)
>      hideFormat: false,
> +    hideFormatWhenStorageEmpty: false,
>  
>      // sets the initial size value
>      // string because else we get a type confusion
> @@ -47,6 +48,9 @@ Ext.define('PVE.form.DiskStorageSelector', {
>  	    hdfilesel.setVisible(false);
>  
>  	    formatsel.setDisabled(true);
> +	    if (me.hideFormatWhenStorageEmpty) {
> +		formatsel.setVisible(false);
> +	    }
>  	    return;
>  	}
>  
> @@ -73,6 +77,11 @@ Ext.define('PVE.form.DiskStorageSelector', {
>  
>  	var select = !!rec.data.select_existing && !me.hideSelection;
>  
> +	// This is needed to make the format selector visible
> +	// after it has been hidden because of hideFormatWhenStorageEmpty.
> +	let hideFormatCondition = me.hideFormat || me.storageContent === 'rootdir';
> +	formatsel.setVisible(!hideFormatCondition);
> +
>  	formatsel.setDisabled(me.hideFormat || Ext.Object.getSize(validFormats) <= 1);
>  	formatsel.setValue(selectFormat);
>  
> @@ -161,7 +170,8 @@ Ext.define('PVE.form.DiskStorageSelector', {
>  		fieldLabel: gettext('Format'),
>  		nodename: me.nodename,
>  		disabled: true,
> -		hidden: me.hideFormat || me.storageContent === 'rootdir',
> +		hidden: me.hideFormat || me.storageContent === 'rootdir'
> +		    || (me.hideFormatWhenStorageEmpty && !me.autoSelect),

eslint complains:
[./form/DiskStorageSelector.js]:
WARN: line 174 col 7: operator-linebreak - '||' should be placed at the
end of the line. (*)

>  		value: 'qcow2',
>  		allowBlank: false,
>  	    },



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager v3 4/4] ui: restore window: add disk-format option

[Markus Frank]

This is done by changing the StorageSelector to a DiskStorageSelector.

Using the hideFormatWhenStorageEmpty option of the DiskStorageSelector
to hide the DiskFormatSelector when no storage is selected, as the
DiskFormatSelector would show the default value qcow2 in a disabled
state, which could confuse users.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
v3:
* added (me.hideFormatWhenStorageEmpty && !me.autoSelect) to the hide
condition in the previous patch (3/4) instead of manually hiding it
here.

 www/manager6/window/Restore.js | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
index 6efe1313..e30e3fab 100644
--- a/www/manager6/window/Restore.js
+++ b/www/manager6/window/Restore.js
@@ -44,8 +44,11 @@ Ext.define('PVE.window.Restore', {
 	    if (values['live-restore']) {
 		params['live-restore'] = 1;
 	    }
-	    if (values.storage) {
-		params.storage = values.storage;
+	    if (values.hdstorage) {
+		params.storage = values.hdstorage;
+	    }
+	    if (values.diskformat) {
+		params['disk-format'] = values.diskformat;
 	    }
 
 	    ['bwlimit', 'cores', 'name', 'memory', 'sockets'].forEach(opt => {
@@ -141,9 +144,8 @@ Ext.define('PVE.window.Restore', {
 			    view.lookupReference(`${key}Field`).setEmptyText(value);
 			}
 		    });
-
 		    if (!allStoragesAvailable) {
-			let storagesel = view.down('pveStorageSelector[name=storage]');
+			let storagesel = view.down('pveStorageSelector[name=hdstorage]');
 			storagesel.allowBlank = false;
 			storagesel.setEmptyText('');
 		    }
@@ -165,12 +167,14 @@ Ext.define('PVE.window.Restore', {
 	    throw "no vmtype specified";
 	}
 
-	let storagesel = Ext.create('PVE.form.StorageSelector', {
+	let storagesel = Ext.create('PVE.form.DiskStorageSelector', {
 	    nodename: me.nodename,
 	    name: 'storage',
 	    value: '',
 	    fieldLabel: gettext('Storage'),
 	    storageContent: me.vmtype === 'lxc' ? 'rootdir' : 'images',
+	    hideSize: true,
+	    hideFormatWhenStorageEmpty: true,
 	    // when restoring a container without specifying a storage, the backend defaults
 	    // to 'local', which is unintuitive and 'rootdir' might not even be allowed on it
 	    allowBlank: me.vmtype !== 'lxc',
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager v3 4/4] ui: restore window: add disk-format option

[Fiona Ebner]

Am 07.03.25 um 10:25 schrieb Markus Frank:
> This is done by changing the StorageSelector to a DiskStorageSelector.
> 
> Using the hideFormatWhenStorageEmpty option of the DiskStorageSelector
> to hide the DiskFormatSelector when no storage is selected, as the
> DiskFormatSelector would show the default value qcow2 in a disabled
> state, which could confuse users.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel