[pve-devel] [PATCH storage/manager v6] allow down/upload & import of images in the web UI

[pve-devel] [PATCH storage/manager v6] allow down/upload & import of images in the web UI

[Dominik Csapak]

most of the building blocks are already there:
* we can have image files in an import storage
* we can import image files via the api from such a storage

this series fills in the missing bits & pieces:
* allow uploading/downloading image files into an import storage via the webgui
* adding the possibility to select such a file when creating a vm/disk

One minor "ugliness": when switching between import on/off, the target
storage selector resets. This is normally intended by the component,
since it's most often only disabled when it's still visible, except here
in this case.

If this is a blocker, I can of course add an option to the selector
to not do this here, but IMHO this is a rather odd use case anyway,
so I opted for not handling that explicitely.

changes from v5:
* new patch that renames isOva to is_ova
* rename imageFormat to image_format
* don't use (unnecessary) second regex
* add hint to ui about storage location
* fix bug with format doubly appended

changes from v4:
* add an 'extension alias' mechanism for the upload window,
  to have separate extensions to validate for the file selector
  and the file name field (thanks for noticing @Filip)

changes from v3:
* allow raw (.raw, .img) and vmdk files too
* automatically append '.raw' for '.img' files

changes from v2:
* fix correctly unset 'import-from' in wizard when going to summary,
  back to disk, unselecting import, then going forward to the summary
  again
* fixed an issue with the file selector being mistakenly disabled

changes from v1:
* fixed an issue where the file selector would be hidden but invalid

pve-storage:

Dominik Csapak (2):
  api: rename 'isOva' to 'is_ova' to adhere to style guide
  import: allow upload of guest images files into import storage

 src/PVE/API2/Storage/Status.pm | 35 +++++++++++++++++++++++++---------
 src/PVE/Storage.pm             |  2 +-
 2 files changed, 27 insertions(+), 10 deletions(-)

pve-manager:

Dominik Csapak (4):
  ui: storage content: allow upload of guest images for import type
  ui: form: file selector: allow optional filter
  ui: qemu hd edit: allow importing a disk from the import storage
  ui: upload window: show hint about upload storage location

 www/manager6/form/FileSelector.js           | 10 +++
 www/manager6/qemu/HDEdit.js                 | 70 ++++++++++++++++++++-
 www/manager6/window/DownloadUrlToStorage.js |  4 ++
 www/manager6/window/UploadToStorage.js      | 29 +++++++--
 4 files changed, 108 insertions(+), 5 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 v6 1/2] api: rename 'isOva' to 'is_ova' to adhere to style guide

[Dominik Csapak]

see https://pve.proxmox.com/wiki/Perl_Style_Guide#Casing

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v6, could be applied even if the rest of the series is not

 src/PVE/API2/Storage/Status.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index 28362ca..3332675 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -455,7 +455,7 @@ __PACKAGE__->register_method ({
 	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
 
 	my $path;
-	my $isOva = 0;
+	my $is_ova = 0;
 
 	if ($content eq 'iso') {
 	    if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
@@ -472,7 +472,7 @@ __PACKAGE__->register_method ({
 		raise_param_exc({ filename => "invalid filename or wrong extension" });
 	    }
 
-	    $isOva = 1;
+	    $is_ova = 1;
 	    $path = PVE::Storage::get_import_dir($cfg, $storage);
 	} else {
 	    raise_param_exc({ content => "upload content type '$content' not allowed" });
@@ -541,7 +541,7 @@ __PACKAGE__->register_method ({
 		    PVE::Storage::assert_iso_content($tmpfilename);
 		}
 
-		if ($isOva) {
+		if ($is_ova) {
 		    assert_ova_contents($tmpfilename);
 		}
 	    };
@@ -666,7 +666,7 @@ __PACKAGE__->register_method({
 	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
 
 	my $path;
-	my $isOva = 0;
+	my $is_ova = 0;
 
 	if ($content eq 'iso') {
 	    if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
@@ -684,7 +684,7 @@ __PACKAGE__->register_method({
 	    }
 
 	    if ($filename =~ m/\.ova$/) {
-		$isOva = 1;
+		$is_ova = 1;
 	    }
 
 	    $path = PVE::Storage::get_import_dir($cfg, $storage);
@@ -716,7 +716,7 @@ __PACKAGE__->register_method({
 		PVE::Storage::assert_iso_content($tmp_path);
 	    }
 
-	    if ($isOva) {
+	    if ($is_ova) {
 		assert_ova_contents($tmp_path);
 	    }
 	};
-- 
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 v6 2/2] import: allow upload of guest images files into import storage

[Dominik Csapak]

so users can upload qcow2/raw/vmdk files directly in the UI
Check the uploaded file with 'file_size_info' and the untrusted flag.
This checks the file format, existence of backing files, etc.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v5:
* rebase
* use existing regex to test for either ova, or the other image formats
* add images to the description

 src/PVE/API2/Storage/Status.pm | 25 +++++++++++++++++++++----
 src/PVE/Storage.pm             |  2 +-
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index 3332675..14915ae 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -387,7 +387,7 @@ __PACKAGE__->register_method ({
     name => 'upload',
     path => '{storage}/upload',
     method => 'POST',
-    description => "Upload templates, ISO images and OVAs.",
+    description => "Upload templates, ISO images, OVAs and VM images.",
     permissions => {
 	check => ['perm', '/storage/{storage}', ['Datastore.AllocateTemplate']],
     },
@@ -456,6 +456,7 @@ __PACKAGE__->register_method ({
 
 	my $path;
 	my $is_ova = 0;
+	my $image_format;
 
 	if ($content eq 'iso') {
 	    if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
@@ -471,8 +472,14 @@ __PACKAGE__->register_method ({
 	    if ($filename !~ m!${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::UPLOAD_IMPORT_EXT_RE_1$!) {
 		raise_param_exc({ filename => "invalid filename or wrong extension" });
 	    }
+	    my $format = $1;
+
+	    if ($format eq 'ova') {
+		$is_ova = 1;
+	    } else {
+		$image_format = $format;
+	    }
 
-	    $is_ova = 1;
 	    $path = PVE::Storage::get_import_dir($cfg, $storage);
 	} else {
 	    raise_param_exc({ content => "upload content type '$content' not allowed" });
@@ -543,6 +550,9 @@ __PACKAGE__->register_method ({
 
 		if ($is_ova) {
 		    assert_ova_contents($tmpfilename);
+		} elsif (defined($image_format)) {
+		    # checks untrusted image
+		    PVE::Storage::file_size_info($tmpfilename, 10, $image_format, 1);
 		}
 	    };
 	    if (my $err = $@) {
@@ -578,7 +588,7 @@ __PACKAGE__->register_method({
     name => 'download_url',
     path => '{storage}/download-url',
     method => 'POST',
-    description => "Download templates, ISO images and OVAs by using an URL.",
+    description => "Download templates, ISO images, OVAs and VM images by using an URL.",
     proxyto => 'node',
     permissions => {
 	description => 'Requires allocation access on the storage and as this allows one to probe'
@@ -667,6 +677,7 @@ __PACKAGE__->register_method({
 
 	my $path;
 	my $is_ova = 0;
+	my $image_format;
 
 	if ($content eq 'iso') {
 	    if ($filename !~ m![^/]+$PVE::Storage::ISO_EXT_RE_0$!) {
@@ -682,9 +693,12 @@ __PACKAGE__->register_method({
 	    if ($filename !~ m!${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::UPLOAD_IMPORT_EXT_RE_1$!) {
 		raise_param_exc({ filename => "invalid filename or wrong extension" });
 	    }
+	    my $format = $1;
 
-	    if ($filename =~ m/\.ova$/) {
+	    if ($format eq 'ova') {
 		$is_ova = 1;
+	    } else {
+		$image_format = $format;
 	    }
 
 	    $path = PVE::Storage::get_import_dir($cfg, $storage);
@@ -718,6 +732,9 @@ __PACKAGE__->register_method({
 
 	    if ($is_ova) {
 		assert_ova_contents($tmp_path);
+	    } elsif (defined($image_format)) {
+		# checks untrusted image
+		PVE::Storage::file_size_info($tmp_path, 10, $image_format, 1);
 	    }
 	};
 
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 7174f0f..d0a696a 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -116,7 +116,7 @@ our $BACKUP_EXT_RE_2 = qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPR
 
 our $IMPORT_EXT_RE_1 = qr/\.(ova|ovf|qcow2|raw|vmdk)/;
 
-our $UPLOAD_IMPORT_EXT_RE_1 = qr/\.(ova)/;
+our $UPLOAD_IMPORT_EXT_RE_1 = qr/\.(ova|qcow2|raw|vmdk)/;
 
 our $SAFE_CHAR_CLASS_RE = qr/[a-zA-Z0-9\-\.\+\=\_]/;
 our $SAFE_CHAR_WITH_WHITESPACE_CLASS_RE = qr/[ a-zA-Z0-9\-\.\+\=\_]/;
-- 
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 manager v6 1/4] ui: storage content: allow upload of guest images for import type

[Dominik Csapak]

by allowing 'qcow2', 'vmdk', 'raw' and 'img' files to upload.

Implement 'extension aliases' for the upload window, that append the
'real' extension to the filename after selecting a file with such an
alias. This is necessary so that we have seperate lists for the file
selector and the filename validator. Use that to mark '.img' an alias of
'.raw' for import.

For the download, the api checks the filename for us first, so we
just have to rename '.img' to '.img.raw'.

partially fixes #2424

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
 www/manager6/window/DownloadUrlToStorage.js |  4 ++++
 www/manager6/window/UploadToStorage.js      | 24 +++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/www/manager6/window/DownloadUrlToStorage.js b/www/manager6/window/DownloadUrlToStorage.js
index 3ac80ac9..aabc9d3c 100644
--- a/www/manager6/window/DownloadUrlToStorage.js
+++ b/www/manager6/window/DownloadUrlToStorage.js
@@ -89,6 +89,10 @@ Ext.define('PVE.window.DownloadUrlToStorage', {
 			    filename = matches[1];
 			    compression = matches[2].toLowerCase();
 			}
+		    } else if (view.content === 'import') {
+			if (filename.endsWith('.img')) {
+			    filename += '.raw';
+			}
 		    }
 
 		    view.setValues({
diff --git a/www/manager6/window/UploadToStorage.js b/www/manager6/window/UploadToStorage.js
index cdf548a8..f29d80f4 100644
--- a/www/manager6/window/UploadToStorage.js
+++ b/www/manager6/window/UploadToStorage.js
@@ -9,19 +9,28 @@ Ext.define('PVE.window.UploadToStorage', {
     title: gettext('Upload'),
 
     acceptedExtensions: {
-	'import': ['.ova'],
+	'import': ['.ova', '.qcow2', '.raw', '.vmdk'],
 	iso: ['.img', '.iso'],
 	vztmpl: ['.tar.gz', '.tar.xz', '.tar.zst'],
     },
 
+    // accepted for file selection, will be renamed to real extension
+    extensionAliases: {
+	'import': {
+	    '.img': '.raw',
+	},
+    },
+
     cbindData: function(initialConfig) {
 	const me = this;
 	const ext = me.acceptedExtensions[me.content] || [];
 
 	me.url = `/nodes/${me.nodename}/storage/${me.storage}/upload`;
 
+	let fileSelectorExt = ext.concat(Object.keys(me.extensionAliases[me.content] ?? {}));
+
 	return {
-	    extensions: ext.join(', '),
+	    extensions: fileSelectorExt.join(', '),
 	    filenameRegex: RegExp('^.*(?:' + ext.join('|').replaceAll('.', '\\.') + ')$', 'i'),
 	};
     },
@@ -134,8 +143,15 @@ Ext.define('PVE.window.UploadToStorage', {
 	},
 
 	fileChange: function(input) {
-	    const vm = this.getViewModel();
-	    const name = input.value.replace(/^.*(\/|\\)/, '');
+	    const me = this;
+	    const vm = me.getViewModel();
+	    const view = me.getView();
+	    let name = input.value.replace(/^.*(\/|\\)/, '');
+	    for (const [alias, real] of Object.entries(view.extensionAliases[view.content] ?? {})) {
+		if (name.endsWith(alias)) {
+		    name += real;
+		}
+	    }
 	    const fileInput = input.fileInputEl.dom;
 	    vm.set('filename', name);
 	    vm.set('size', (fileInput.files[0] && Proxmox.Utils.format_size(fileInput.files[0].size)) || '-');
-- 
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 manager v6 2/4] ui: form: file selector: allow optional filter

[Dominik Csapak]

this sometimes comes in handy when we only want to show specific files.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
 www/manager6/form/FileSelector.js | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/www/manager6/form/FileSelector.js b/www/manager6/form/FileSelector.js
index ef2bedf9..9db20711 100644
--- a/www/manager6/form/FileSelector.js
+++ b/www/manager6/form/FileSelector.js
@@ -43,6 +43,13 @@ Ext.define('PVE.form.FileSelector', {
 	    url: url,
 	});
 
+	if (Ext.isFunction(me.filter)) {
+	    me.store.clearFilter();
+	    me.store.addFilter([me.filter]);
+	} else {
+	    me.store.clearFilter();
+	}
+
 	me.store.removeAll();
 	me.store.load();
     },
@@ -60,6 +67,9 @@ Ext.define('PVE.form.FileSelector', {
     valueField: 'volid',
     displayField: 'text',
 
+    // An optional filter function
+    filter: undefined,
+
     listConfig: {
 	width: 600,
 	columns: [
-- 
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 manager v6 3/4] ui: qemu hd edit: allow importing a disk from the import storage

[Dominik Csapak]

adds a checkbox 'import image' above the storage selector which:
* hides the original storage selector
* shows a 'source storage' selector
* shows a 'import file' selector
* shows a 'target storage' selector

Since the wizard and the hd edit share this panel, this also works in
the wizard.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v5:
* don't autoSelect target storage for import, to not accidentally
  enable the format dropdown

 www/manager6/qemu/HDEdit.js | 70 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index b78647ec..a5ec0407 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -78,11 +78,14 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	    if (values.hdimage) {
 		me.drive.file = values.hdimage;
 	    } else {
-		me.drive.file = values.hdstorage + ":" + values.disksize;
+		let disksize = values['import-from'] ? 0 : values.disksize;
+		me.drive.file = `${values.hdstorage}:${disksize}`;
+		PVE.Utils.propertyStringSet(me.drive, values['import-from'], 'import-from');
 	    }
 	    me.drive.format = values.diskformat;
 	}
 
+
 	PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
 	PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
 	PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
@@ -168,6 +171,11 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	var me = this;
 	me.down('#hdstorage').setNodename(nodename);
 	me.down('#hdimage').setStorage(undefined, nodename);
+
+	me.lookup('new-disk').setNodename(nodename);
+	me.lookup('import-source').setNodename(nodename);
+	me.lookup('import-source-file').setNodename(nodename);
+	me.lookup('import-target').setNodename(nodename);
     },
 
     hasAdvanced: true,
@@ -221,12 +229,72 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	    column1.push(me.unusedDisks);
 	} else if (me.isCreate) {
 	    column1.push({
+		xtype: 'proxmoxcheckbox',
+		isFormField: false,
+		fieldLabel: gettext("Import Image"),
+		listeners: {
+		    change: function(_cb, value) {
+			me.lookup('new-disk').setVisible(!value);
+			me.lookup('new-disk').setDisabled(!!value);
+
+			let importSource = me.lookup('import-source');
+			importSource.setVisible(!!value);
+			importSource.setDisabled(!value);
+			me.lookup('import-source-file').setVisible(!!value);
+			me.lookup('import-source-file').setDisabled(!value || !importSource.getValue());
+
+			me.lookup('import-target').setVisible(!!value);
+			me.lookup('import-target').setDisabled(!value);
+		    },
+		},
+	    });
+	    column1.push({
+		reference: 'new-disk',
 		xtype: 'pveDiskStorageSelector',
 		storageContent: 'images',
 		name: 'disk',
 		nodename: me.nodename,
 		autoSelect: me.insideWizard,
 	    });
+	    column1.push({
+		xtype: 'pveStorageSelector',
+		reference: 'import-source',
+		fieldLabel: gettext('Import Storage'),
+		name: 'import-source-storage',
+		hidden: true,
+		disabled: true,
+		storageContent: 'import',
+		nodename: me.nodename,
+		autoSelect: me.insideWizard,
+		listeners: {
+		    change: function(selector, storage) {
+			me.lookup('import-source-file').setStorage(storage);
+			me.lookup('import-source-file').setDisabled(!storage || selector.isDisabled());
+		    },
+		},
+	    });
+	    column1.push({
+		xtype: 'pveFileSelector',
+		reference: 'import-source-file',
+		fieldLabel: gettext("Select Image"),
+		hidden: true,
+		disabled: true,
+		storageContent: 'import',
+		name: 'import-from',
+		filter: (rec) => ['qcow2', 'vmdk', 'raw'].indexOf(rec?.data?.format) !== -1,
+		nodename: me.nodename,
+	    });
+	    column1.push({
+		xtype: 'pveDiskStorageSelector',
+		reference: 'import-target',
+		storageLabel: gettext('Target Storage'),
+		hidden: true,
+		disabled: true,
+		hideSize: true,
+		storageContent: 'images',
+		name: 'target',
+		nodename: me.nodename,
+	    });
 	} else {
 	    column1.push({
 		xtype: 'textfield',
-- 
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 manager v6 4/4] ui: upload window: show hint about upload storage location

[Dominik Csapak]

so users are not surprised where this gets uploaded to.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v6

 www/manager6/window/UploadToStorage.js | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/www/manager6/window/UploadToStorage.js b/www/manager6/window/UploadToStorage.js
index f29d80f4..0a1024ae 100644
--- a/www/manager6/window/UploadToStorage.js
+++ b/www/manager6/window/UploadToStorage.js
@@ -245,6 +245,11 @@ Ext.define('PVE.window.UploadToStorage', {
 		    emptyText: gettext('none'),
 		    reference: 'downloadUrlChecksum',
 		},
+		{
+		    xtype: 'displayfield',
+		    userCls: 'pmx-hint',
+		    value: gettext('Uploads are stored temporarily in /var/tmp/, make sure there is enough free space.'),
+		},
 		{
 		    xtype: 'progressbar',
 		    text: 'Ready',
-- 
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 v6 3/4] ui: qemu hd edit: allow importing a disk from the import storage

[Thomas Lamprecht]

Am 07.04.25 um 12:13 schrieb Dominik Csapak:
> adds a checkbox 'import image' above the storage selector which:
> * hides the original storage selector
> * shows a 'source storage' selector
> * shows a 'import file' selector
> * shows a 'target storage' selector
> 
> Since the wizard and the hd edit share this panel, this also works in
> the wizard.

Can we disable this for HD Add? IMO that dialogue is already really
complex as is.

Might be even beneficial if this is always predefined from the call
or integration side...

And seems I cannot start an import through the import storage content
view? Feels odd to me, all other imports are triggered there after all.


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

[pve-devel] partially-applied: [PATCH storage/manager v6] allow down/upload & import of images in the web UI

[Thomas Lamprecht]

On Mon, 07 Apr 2025 12:13:04 +0200, Dominik Csapak wrote:
> most of the building blocks are already there:
> * we can have image files in an import storage
> * we can import image files via the api from such a storage
> 
> this series fills in the missing bits & pieces:
> * allow uploading/downloading image files into an import storage via the webgui
> * adding the possibility to select such a file when creating a vm/disk
> 
> [...]

Applied, thanks!

[1/2] api: rename 'isOva' to 'is_ova' to adhere to style guide
      commit 551bad9d470117f2214f5fc2640a0bd6cfdeb890
[2/2] import: allow upload of guest images files into import storage
      commit 76f695f2e6887bff6576668d6a8aeafca7ff966a
[1/4] ui: storage content: allow upload of guest images for import type
      commit: f37294e00faca2f335a61fb03318b4c8866ebe47
[2/4] ui: form: file selector: allow optional filter
      commit: b72a01a8737423d67fc624293a031d0e639854f7
[3/4] ui: qemu hd edit: allow importing a disk from the import storage
      SKIPPED for now.
[4/4] ui: upload window: show hint about upload storage location
      commit: cbc27c8a81989c5e5b267c073cf675102d51b086


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

Re: [pve-devel] [PATCH manager v6 3/4] ui: qemu hd edit: allow importing a disk from the import storage

[Dominik Csapak]

On 4/8/25 00:13, Thomas Lamprecht wrote:
> Am 07.04.25 um 12:13 schrieb Dominik Csapak:
>> adds a checkbox 'import image' above the storage selector which:
>> * hides the original storage selector
>> * shows a 'source storage' selector
>> * shows a 'import file' selector
>> * shows a 'target storage' selector
>>
>> Since the wizard and the hd edit share this panel, this also works in
>> the wizard.
> 
> Can we disable this for HD Add? IMO that dialogue is already really
> complex as is.
> 
> Might be even beneficial if this is always predefined from the call
> or integration side...
> 
> And seems I cannot start an import through the import storage content
> view? Feels odd to me, all other imports are triggered there after all.

ok so if I understand you correctly, you want this in the wizard but not
in the 'add hd' panel ? And you want to have a (probably simplified) hd
add panel from the import storage content panel directly ?

sounds fine to me, an alternative I could offer would be to
have a separate input panel only for importing, and show in the wizard
another button (e.g. split button on add) that adds that import panel
and for the guests a new entry in the 'add menu' that says 'import'

(this panel could then also be added to the import storage content,
with an additional vm selector)


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

Re: [pve-devel] [PATCH manager v6 3/4] ui: qemu hd edit: allow importing a disk from the import storage

[Thomas Lamprecht]

Am 08.04.25 um 09:01 schrieb Dominik Csapak:
> On 4/8/25 00:13, Thomas Lamprecht wrote:
>> Am 07.04.25 um 12:13 schrieb Dominik Csapak:
>>> adds a checkbox 'import image' above the storage selector which:
>>> * hides the original storage selector
>>> * shows a 'source storage' selector
>>> * shows a 'import file' selector
>>> * shows a 'target storage' selector
>>>
>>> Since the wizard and the hd edit share this panel, this also works in
>>> the wizard.
>>
>> Can we disable this for HD Add? IMO that dialogue is already really
>> complex as is.
>>
>> Might be even beneficial if this is always predefined from the call
>> or integration side...
>>
>> And seems I cannot start an import through the import storage content
>> view? Feels odd to me, all other imports are triggered there after all.
> 
> ok so if I understand you correctly, you want this in the wizard but not
> in the 'add hd' panel ? And you want to have a (probably simplified) hd
> add panel from the import storage content panel directly ?

Yeah for starters I'd probably just have gone for allowing importing
such a disk to an existing VM. That said checking a few reports and
rethinking this users might actually want to have it available in the
creation process, that feels a bit more natural...

Cc'in Aaron for an opinion as he often gets UX feedback a bit more
directly in trainings and has a more decisive opinion on these
things.

> 
> sounds fine to me, an alternative I could offer would be to
> have a separate input panel only for importing, and show in the wizard
> another button (e.g. split button on add) that adds that import panel
> and for the guests a new entry in the 'add menu' that says 'import'
> 
> (this panel could then also be added to the import storage content,
> with an additional vm selector)

btw: should this have a fix #2424 ?


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

Re: [pve-devel] [PATCH manager v6 3/4] ui: qemu hd edit: allow importing a disk from the import storage

[Dominik Csapak]

On 4/8/25 10:40, Thomas Lamprecht wrote:
> Am 08.04.25 um 09:01 schrieb Dominik Csapak:
>> On 4/8/25 00:13, Thomas Lamprecht wrote:
>>> Am 07.04.25 um 12:13 schrieb Dominik Csapak:
>>>> adds a checkbox 'import image' above the storage selector which:
>>>> * hides the original storage selector
>>>> * shows a 'source storage' selector
>>>> * shows a 'import file' selector
>>>> * shows a 'target storage' selector
>>>>
>>>> Since the wizard and the hd edit share this panel, this also works in
>>>> the wizard.
>>>
>>> Can we disable this for HD Add? IMO that dialogue is already really
>>> complex as is.
>>>
>>> Might be even beneficial if this is always predefined from the call
>>> or integration side...
>>>
>>> And seems I cannot start an import through the import storage content
>>> view? Feels odd to me, all other imports are triggered there after all.
>>
>> ok so if I understand you correctly, you want this in the wizard but not
>> in the 'add hd' panel ? And you want to have a (probably simplified) hd
>> add panel from the import storage content panel directly ?
> 
> Yeah for starters I'd probably just have gone for allowing importing
> such a disk to an existing VM. That said checking a few reports and
> rethinking this users might actually want to have it available in the
> creation process, that feels a bit more natural...
> 
> Cc'in Aaron for an opinion as he often gets UX feedback a bit more
> directly in trainings and has a more decisive opinion on these
> things.

what i currently have (half) working here is basically what i wrote
below, the same input panel, but either for creating a new disk
or importing, and the caller decides. with another button in
the wizard that allows adding such an 'import panel'

(should be relatively easy to add that to the hardware panel and
import storage content too)

> 
>>
>> sounds fine to me, an alternative I could offer would be to
>> have a separate input panel only for importing, and show in the wizard
>> another button (e.g. split button on add) that adds that import panel
>> and for the guests a new entry in the 'add menu' that says 'import'
>>
>> (this panel could then also be added to the import storage content,
>> with an additional vm selector)
> 
> btw: should this have a fix #2424 ?

you're right, at least a 'partially fixes #2424' since the users also wanted to have
definitions uploaded (libvirt xmls?) but not sure if that's even a thing that we
want to have..


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