public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option
@ 2025-02-04 16:13 Markus Frank
  2025-02-04 16:13 ` [pve-devel] [PATCH manager v1 2/3] ui: form: add hideFormatWhenStorageEmpty option to DiskStorageSelector Markus Frank
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Markus Frank @ 2025-02-04 16:13 UTC (permalink / raw)
  To: pve-devel

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>
---
Choosing the file format and storage for each disk would require much
more change and would be more in line with Feature Request #4275.

 PVE/API2/Qemu.pm     |  3 +++
 PVE/CLI/qmrestore.pm |  1 +
 PVE/QemuServer.pm    | 10 +++++++---
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 295260e7..b742e71f 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1013,6 +1013,7 @@ __PACKAGE__->register_method({
 		    default => 0,
 		    description => "Start VM after it was created successfully.",
 		},
+		diskformat => PVE::JSONSchema::get_standard_option('pve-qm-image-format'),
 		'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 +1047,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 $diskformat = extract_param($param, 'diskformat');
 	my $extraction_storage = extract_param($param, 'import-working-storage');
 
 	if (defined(my $ssh_keys = $param->{sshkeys})) {
@@ -1143,6 +1145,7 @@ __PACKAGE__->register_method({
 		    unique => $unique,
 		    bwlimit => $bwlimit,
 		    live => $live_restore,
+		    diskformat => $diskformat,
 		    override_conf => $param,
 		};
 		if (my $volid = $archive->{volid}) {
diff --git a/PVE/CLI/qmrestore.pm b/PVE/CLI/qmrestore.pm
index a47648bd..c698ca77 100755
--- a/PVE/CLI/qmrestore.pm
+++ b/PVE/CLI/qmrestore.pm
@@ -64,6 +64,7 @@ __PACKAGE__->register_method({
 		type => 'boolean',
 		description => "Start the VM immediately from the backup and restore in background. PBS only.",
 	    },
+	    diskformat => PVE::JSONSchema::get_standard_option('pve-qm-image-format'),
 	},
     },
     returns => {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 808c0e1c..f6c58f09 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6660,7 +6660,7 @@ my $parse_backup_hints = sub {
 #
 # Returns: { $virtdev => $volid }
 my $restore_allocate_devices = sub {
-    my ($storecfg, $virtdev_hash, $vmid) = @_;
+    my ($storecfg, $virtdev_hash, $vmid, $diskformat) = @_;
 
     my $map = {};
     foreach my $virtdev (sort keys %$virtdev_hash) {
@@ -6670,6 +6670,10 @@ my $restore_allocate_devices = sub {
 	my $storeid = $d->{storeid};
 	my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
 
+	if ($diskformat) {
+	    $d->{format} = $diskformat;
+	}
+
 	# test if requested format is supported
 	my ($defFormat, $validFormats) = PVE::Storage::storage_default_format($storecfg, $storeid);
 	my $supported = grep { $_ eq $d->{format} } @$validFormats;
@@ -7047,7 +7051,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->{'diskformat'});
 
 	foreach my $virtdev (sort keys %$virtdev_hash) {
 	    my $d = $virtdev_hash->{$virtdev};
@@ -7444,7 +7448,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->{'diskformat'});
 
 	# 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [pve-devel] [PATCH manager v1 2/3] ui: form: add hideFormatWhenStorageEmpty option to DiskStorageSelector
  2025-02-04 16:13 [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option Markus Frank
@ 2025-02-04 16:13 ` Markus Frank
  2025-02-04 16:13 ` [pve-devel] [PATCH manager v1 3/3] ui: window: add diskformat option to the restore window Markus Frank
  2025-02-04 16:21 ` [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option Fiona Ebner
  2 siblings, 0 replies; 7+ messages in thread
From: Markus Frank @ 2025-02-04 16:13 UTC (permalink / raw)
  To: pve-devel

Prerequisite for "ui: window: add diskformat option to restore window by
changing to DiskStorageSelector"

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

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 www/manager6/form/DiskStorageSelector.js | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/www/manager6/form/DiskStorageSelector.js b/www/manager6/form/DiskStorageSelector.js
index 0ef48f51..e2064934 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
@@ -41,12 +42,20 @@ Ext.define('PVE.form.DiskStorageSelector', {
 	var hdfilesel = me.getComponent('hdimage');
 	var hdsizesel = me.getComponent('disksize');
 
+	// 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);
+
 	// initial store load, and reset/deletion of the storage
 	if (!value) {
 	    hdfilesel.setDisabled(true);
 	    hdfilesel.setVisible(false);
 
 	    formatsel.setDisabled(true);
+	    if (me.hideFormatWhenStorageEmpty) {
+		formatsel.setVisible(false);
+	    }
 	    return;
 	}
 
-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [pve-devel] [PATCH manager v1 3/3] ui: window: add diskformat option to the restore window
  2025-02-04 16:13 [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option Markus Frank
  2025-02-04 16:13 ` [pve-devel] [PATCH manager v1 2/3] ui: form: add hideFormatWhenStorageEmpty option to DiskStorageSelector Markus Frank
@ 2025-02-04 16:13 ` Markus Frank
  2025-02-04 16:21 ` [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option Fiona Ebner
  2 siblings, 0 replies; 7+ messages in thread
From: Markus Frank @ 2025-02-04 16:13 UTC (permalink / raw)
  To: pve-devel

This is done by changing the StorageSelector to a DiskStorageSelector
and using the hideFormatWhenStorageEmpty option to hide the format
selector when no storage is selected, as the format selector would show
the default value qcow2 in a disabled state, which could confuse users.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
 www/manager6/window/Restore.js | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
index 6efe1313..393a6b28 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.diskformat = values.diskformat;
 	    }
 
 	    ['bwlimit', 'cores', 'name', 'memory', 'sockets'].forEach(opt => {
@@ -141,9 +144,10 @@ Ext.define('PVE.window.Restore', {
 			    view.lookupReference(`${key}Field`).setEmptyText(value);
 			}
 		    });
-
+		    let diskformat = view.down('pveDiskFormatSelector[name=diskformat]');
+		    diskformat.setVisible(false);
 		    if (!allStoragesAvailable) {
-			let storagesel = view.down('pveStorageSelector[name=storage]');
+			let storagesel = view.down('pveStorageSelector[name=hdstorage]');
 			storagesel.allowBlank = false;
 			storagesel.setEmptyText('');
 		    }
@@ -165,12 +169,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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option
  2025-02-04 16:13 [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option Markus Frank
  2025-02-04 16:13 ` [pve-devel] [PATCH manager v1 2/3] ui: form: add hideFormatWhenStorageEmpty option to DiskStorageSelector Markus Frank
  2025-02-04 16:13 ` [pve-devel] [PATCH manager v1 3/3] ui: window: add diskformat option to the restore window Markus Frank
@ 2025-02-04 16:21 ` Fiona Ebner
  2025-02-04 16:39   ` Fiona Ebner
  2 siblings, 1 reply; 7+ messages in thread
From: Fiona Ebner @ 2025-02-04 16:21 UTC (permalink / raw)
  To: Proxmox VE development discussion, Markus Frank

Am 04.02.25 um 17:13 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>
> ---
> Choosing the file format and storage for each disk would require much
> more change and would be more in line with Feature Request #4275.

It's not that much more change. And if we add this now and then the
implementation for the other feature, we'll have duplicate/conflicting
options, which is also not nice. We can deprecate this option then of
course and later drop, but why not just go for the more complete
solution directly? Or at least a solution that can be built upon for
adding that feature later?


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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option
  2025-02-04 16:21 ` [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option Fiona Ebner
@ 2025-02-04 16:39   ` Fiona Ebner
  2025-02-04 16:44     ` Markus Frank
  0 siblings, 1 reply; 7+ messages in thread
From: Fiona Ebner @ 2025-02-04 16:39 UTC (permalink / raw)
  To: Proxmox VE development discussion, Markus Frank

Am 04.02.25 um 17:21 schrieb Fiona Ebner:
> Am 04.02.25 um 17:13 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>
>> ---
>> Choosing the file format and storage for each disk would require much
>> more change and would be more in line with Feature Request #4275.
> 
> It's not that much more change. And if we add this now and then the
> implementation for the other feature, we'll have duplicate/conflicting
> options, which is also not nice. We can deprecate this option then of
> course and later drop, but why not just go for the more complete
> solution directly? Or at least a solution that can be built upon for
> adding that feature later?

Although I guess it can still serve as a fallback later too, so maybe we
don't even need to drop it. For new multi-word options, please use
kebap-case. Also, the option should not be "pve-qm-image-format", but
"pve-storage-format".


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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option
  2025-02-04 16:39   ` Fiona Ebner
@ 2025-02-04 16:44     ` Markus Frank
  2025-02-04 16:53       ` Fiona Ebner
  0 siblings, 1 reply; 7+ messages in thread
From: Markus Frank @ 2025-02-04 16:44 UTC (permalink / raw)
  To: Proxmox VE development discussion, Fiona Ebner



On  2025-02-04 17:39, Fiona Ebner wrote:
> Am 04.02.25 um 17:21 schrieb Fiona Ebner:
>> Am 04.02.25 um 17:13 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>
>>> ---
>>> Choosing the file format and storage for each disk would require much
>>> more change and would be more in line with Feature Request #4275.
>>
>> It's not that much more change. And if we add this now and then the
>> implementation for the other feature, we'll have duplicate/conflicting
>> options, which is also not nice. We can deprecate this option then of
>> course and later drop, but why not just go for the more complete
>> solution directly? Or at least a solution that can be built upon for
>> adding that feature later?
> 
> Although I guess it can still serve as a fallback later too, so maybe we
> don't even need to drop it. For new multi-word options, please use
> kebap-case. Also, the option should not be "pve-qm-image-format", but
> "pve-storage-format".

I have an idea how we could avoid deprecating this.
We could make this the default storage/format option, and override this selection per vm disk if desired.
In the WebUI for example we could make this per disk selection in an advanced tab.


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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option
  2025-02-04 16:44     ` Markus Frank
@ 2025-02-04 16:53       ` Fiona Ebner
  0 siblings, 0 replies; 7+ messages in thread
From: Fiona Ebner @ 2025-02-04 16:53 UTC (permalink / raw)
  To: Markus Frank, Proxmox VE development discussion

Am 04.02.25 um 17:44 schrieb Markus Frank:
> 
> 
> On  2025-02-04 17:39, Fiona Ebner wrote:
>> Am 04.02.25 um 17:21 schrieb Fiona Ebner:
>>> Am 04.02.25 um 17:13 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>
>>>> ---
>>>> Choosing the file format and storage for each disk would require much
>>>> more change and would be more in line with Feature Request #4275.
>>>
>>> It's not that much more change. And if we add this now and then the
>>> implementation for the other feature, we'll have duplicate/conflicting
>>> options, which is also not nice. We can deprecate this option then of
>>> course and later drop, but why not just go for the more complete
>>> solution directly? Or at least a solution that can be built upon for
>>> adding that feature later?
>>
>> Although I guess it can still serve as a fallback later too, so maybe we
>> don't even need to drop it. For new multi-word options, please use
>> kebap-case. Also, the option should not be "pve-qm-image-format", but
>> "pve-storage-format".

(The reason is pve-qm-image-format contains more than our storage layer
supports). But pve-storage-format is actually is also not fully correct,
because it contains "subvol", which should not be used for VM images. We
could think about adding a new in the storage layer (because that's
where it needs to be supported) that includes only the ones that can be
used for VM images.

> 
> I have an idea how we could avoid deprecating this.
> We could make this the default storage/format option, and override this
> selection per vm disk if desired.
> In the WebUI for example we could make this per disk selection in an
> advanced tab.

Yes, that's what I mean by "fallback" :)


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-02-04 16:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-04 16:13 [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option Markus Frank
2025-02-04 16:13 ` [pve-devel] [PATCH manager v1 2/3] ui: form: add hideFormatWhenStorageEmpty option to DiskStorageSelector Markus Frank
2025-02-04 16:13 ` [pve-devel] [PATCH manager v1 3/3] ui: window: add diskformat option to the restore window Markus Frank
2025-02-04 16:21 ` [pve-devel] [PATCH qemu-server v1 1/3] fix #4888: qmrestore: add diskformat option Fiona Ebner
2025-02-04 16:39   ` Fiona Ebner
2025-02-04 16:44     ` Markus Frank
2025-02-04 16:53       ` Fiona Ebner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal