[pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector

[pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector

[Maximiliano Sandoval]

The pool selector is only visible when restoring from the
Datacenter->{node}->{storage}->Backups panel.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---

Differences from v1:
 - Hide the pool selector when restoring a VM in-place.

 www/manager6/window/Restore.js | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
index 22900868..6f9c9f2f 100644
--- a/www/manager6/window/Restore.js
+++ b/www/manager6/window/Restore.js
@@ -47,6 +47,9 @@ Ext.define('PVE.window.Restore', {
 	    if (values.storage) {
 		params.storage = values.storage;
 	    }
+	    if (values.pool) {
+		params.pool = values.pool;
+	    }
 
 	    ['bwlimit', 'cores', 'name', 'memory', 'sockets'].forEach(opt => {
 		if ((values[opt] ?? '') !== '') {
@@ -186,6 +189,14 @@ Ext.define('PVE.window.Restore', {
 		fieldLabel: gettext('Source'),
 	    },
 	    storagesel,
+	    {
+		xtype: 'pvePoolSelector',
+		fieldLabel: gettext('Resource Pool'),
+		name: 'pool',
+		value: '',
+		allowBlank: true,
+		hidden: me.vmid,
+	    },
 	    {
 		xtype: 'pmxDisplayEditField',
 		name: 'vmid',
-- 
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 v2] fix #4166: restore: add resource pool selector

[Thomas Lamprecht]

Am 12.06.25 um 10:23 schrieb Maximiliano Sandoval:
> The pool selector is only visible when restoring from the
> Datacenter->{node}->{storage}->Backups panel.

I mean fine for now, but IMO if the user has enough rights then restoring
to another pool for when overwriting an existing VM might make sense to;
but that can definitively be an independent change.

> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> 
> Differences from v1:
>  - Hide the pool selector when restoring a VM in-place.
> 
>  www/manager6/window/Restore.js | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
> index 22900868..6f9c9f2f 100644
> --- a/www/manager6/window/Restore.js
> +++ b/www/manager6/window/Restore.js
> @@ -47,6 +47,9 @@ Ext.define('PVE.window.Restore', {
>  	    if (values.storage) {
>  		params.storage = values.storage;
>  	    }
> +	    if (values.pool) {
> +		params.pool = values.pool;
> +	    }
>  
>  	    ['bwlimit', 'cores', 'name', 'memory', 'sockets'].forEach(opt => {
>  		if ((values[opt] ?? '') !== '') {
> @@ -186,6 +189,14 @@ Ext.define('PVE.window.Restore', {
>  		fieldLabel: gettext('Source'),
>  	    },
>  	    storagesel,
> +	    {
> +		xtype: 'pvePoolSelector',
> +		fieldLabel: gettext('Resource Pool'),
> +		name: 'pool',
> +		value: '',
> +		allowBlank: true,
> +		hidden: me.vmid,
> +	    },

Should this better get placed in the override section?

>  	    {
>  		xtype: 'pmxDisplayEditField',
>  		name: 'vmid',



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

Re: [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector

[Fiona Ebner]

Am 17.06.25 um 17:45 schrieb Thomas Lamprecht:
> Am 12.06.25 um 10:23 schrieb Maximiliano Sandoval:
>> The pool selector is only visible when restoring from the
>> Datacenter->{node}->{storage}->Backups panel.

This does not explain why it's done like that.

> I mean fine for now, but IMO if the user has enough rights then restoring
> to another pool for when overwriting an existing VM might make sense to;
> but that can definitively be an independent change.

Yes, either it should be supported or at least there should be some
notice to the user like I wrote in the review of v1:

> In case of an in-place restore, the pool option does not have any
> effect, so the selector should be hidden. And pre-existing, but the
> backend should print a message/warning that the parameter is ignored.


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

Re: [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector

[Maximiliano Sandoval]

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

> Am 12.06.25 um 10:23 schrieb Maximiliano Sandoval:
>> The pool selector is only visible when restoring from the
>> Datacenter->{node}->{storage}->Backups panel.
>
> I mean fine for now, but IMO if the user has enough rights then restoring
> to another pool for when overwriting an existing VM might make sense to;
> but that can definitively be an independent change.
>
>> 
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>> 
>> Differences from v1:
>>  - Hide the pool selector when restoring a VM in-place.
>> 
>>  www/manager6/window/Restore.js | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/www/manager6/window/Restore.js b/www/manager6/window/Restore.js
>> index 22900868..6f9c9f2f 100644
>> --- a/www/manager6/window/Restore.js
>> +++ b/www/manager6/window/Restore.js
>> @@ -47,6 +47,9 @@ Ext.define('PVE.window.Restore', {
>>  	    if (values.storage) {
>>  		params.storage = values.storage;
>>  	    }
>> +	    if (values.pool) {
>> +		params.pool = values.pool;
>> +	    }
>>  
>>  	    ['bwlimit', 'cores', 'name', 'memory', 'sockets'].forEach(opt => {
>>  		if ((values[opt] ?? '') !== '') {
>> @@ -186,6 +189,14 @@ Ext.define('PVE.window.Restore', {
>>  		fieldLabel: gettext('Source'),
>>  	    },
>>  	    storagesel,
>> +	    {
>> +		xtype: 'pvePoolSelector',
>> +		fieldLabel: gettext('Resource Pool'),
>> +		name: 'pool',
>> +		value: '',
>> +		allowBlank: true,
>> +		hidden: me.vmid,
>> +	    },
>
> Should this better get placed in the override section?
>

It could be placed there, however I think it makes sense to put it next
to the storage selector (which could also be argued is an override). I
could add a `emptyText` with value 'From backup configuration' similar
as the storage selector though.

>>  	    {
>>  		xtype: 'pmxDisplayEditField',
>>  		name: 'vmid',



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

Re: [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector

[Thomas Lamprecht]

Am 25.06.25 um 15:48 schrieb Maximiliano Sandoval:
> It could be placed there, however I think it makes sense to put it next
> to the storage selector (which could also be argued is an override). I
> could add a `emptyText` with value 'From backup configuration' similar
> as the storage selector though.

My conclusion would then be to move the storage to the rest of the
override settings rather than making the UX pattern even worse here...

But for one that would be certainly not the job of this patch and for
another is the storage a core parameter that is basically always
present (and often explicitly set, as one cannot see what the value
will be, a UX problem but nonetheless the status quo) whereas resource
pools are certainly more of an edge case.

Just like a hypothetical per-volume storage override–or any other
property–should also go in the "Override Setting" section.


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