* [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector
@ 2025-06-12 8:23 Maximiliano Sandoval
2025-06-17 15:45 ` Thomas Lamprecht
0 siblings, 1 reply; 5+ messages in thread
From: Maximiliano Sandoval @ 2025-06-12 8:23 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector
2025-06-12 8:23 [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector Maximiliano Sandoval
@ 2025-06-17 15:45 ` Thomas Lamprecht
2025-06-18 8:32 ` Fiona Ebner
2025-06-25 13:48 ` Maximiliano Sandoval
0 siblings, 2 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2025-06-17 15:45 UTC (permalink / raw)
To: Proxmox VE development discussion, Maximiliano Sandoval
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector
2025-06-17 15:45 ` Thomas Lamprecht
@ 2025-06-18 8:32 ` Fiona Ebner
2025-06-25 13:48 ` Maximiliano Sandoval
1 sibling, 0 replies; 5+ messages in thread
From: Fiona Ebner @ 2025-06-18 8:32 UTC (permalink / raw)
To: Proxmox VE development discussion, Thomas Lamprecht,
Maximiliano Sandoval
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector
2025-06-17 15:45 ` Thomas Lamprecht
2025-06-18 8:32 ` Fiona Ebner
@ 2025-06-25 13:48 ` Maximiliano Sandoval
2025-06-25 14:02 ` Thomas Lamprecht
1 sibling, 1 reply; 5+ messages in thread
From: Maximiliano Sandoval @ 2025-06-25 13:48 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector
2025-06-25 13:48 ` Maximiliano Sandoval
@ 2025-06-25 14:02 ` Thomas Lamprecht
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Lamprecht @ 2025-06-25 14:02 UTC (permalink / raw)
To: Maximiliano Sandoval; +Cc: Proxmox VE development discussion
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-06-25 14:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-06-12 8:23 [pve-devel] [PATCH manager v2] fix #4166: restore: add resource pool selector Maximiliano Sandoval
2025-06-17 15:45 ` Thomas Lamprecht
2025-06-18 8:32 ` Fiona Ebner
2025-06-25 13:48 ` Maximiliano Sandoval
2025-06-25 14:02 ` Thomas Lamprecht
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