[pve-devel] [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[pve-devel] [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Philipp Hufnagl]

When a member of 2 pools wants to transfer a
 vm/container to an other pool they can not do that. The vm/container would
 have first to be removed form the current pool resulting in a loss of
 privileges of the pool member for this vm/contianer. This feature introduces
 a way to transfer a vm between pools, guarded by a checkbox from accidental
 transfers

changes to v4:
* check for allocate permissions of the originating pool

changes to v3:
* fix subject typo
* at version log

changes to v2:
* split patch in front and backend

Philipp Hufnagl (2):
  fix #474: api: allow transfer from container/vms
  fix #474: ui: allow transfer from container/vms

 PVE/API2/Pool.pm                 | 16 ++++++++++++++--
 www/manager6/grid/PoolMembers.js | 17 ++++++++++++++---
 2 files changed, 28 insertions(+), 5 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH manager v4 1/2] fix #474: api: allow transfer from container/vms

[Philipp Hufnagl]

When the newly introduced optional parameter "transfer" is set, the user
add a vm/container to a pool even if it is already in one. If so it will
be removed from the old pool

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 PVE/API2/Pool.pm | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm
index 007fc815..3c0ae2a0 100644
--- a/PVE/API2/Pool.pm
+++ b/PVE/API2/Pool.pm
@@ -131,6 +131,11 @@ __PACKAGE__->register_method ({
 		type => 'string',  format => 'pve-storage-id-list',
 		optional => 1,
 	    },
+	    transfer => {
+		description => "Allow transferring VMs to another pool.",
+		type => 'boolean',
+		optional => 1,
+	    },
 	    delete => {
 		description => "Remove vms/storage (instead of adding it).",
 		type => 'boolean',
@@ -165,8 +170,15 @@ __PACKAGE__->register_method ({
 		    } else {
 			die "VM $vmid is already a pool member\n" if $pool_config->{vms}->{$vmid};
 			my $existing_pool = $usercfg->{vms}->{$vmid};
-			die "VM $vmid belongs already to pool '$existing_pool'\n" if defined($existing_pool);
-
+			if (defined($existing_pool)) {
+			    if ($param->{transfer}) {
+				$rpcenv->check($authuser, "/pool/$existing_pool", ['Pool.Allocate']);
+				my $existing_pool_config = $usercfg->{pools}->{$existing_pool};
+				delete $existing_pool_config->{vms}->{$vmid};
+			    } else {
+				die "VM $vmid belongs already to pool '$existing_pool' and transfer is not set\n";
+			    }
+			}
 			$pool_config->{vms}->{$vmid} = 1;
 			$usercfg->{vms}->{$vmid} = $pool;
 		    }
-- 
2.39.2

[pve-devel] [PATCH manager v4 2/2] fix #474: ui: allow transfer from container/vms

[Philipp Hufnagl]

A user can no see all vms/containers, even the ones that are already a
member of a pool. They can be transfered now after checking the newly
introduced "allow transfer" checkbox.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 www/manager6/grid/PoolMembers.js | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index 6acb622d..224daca3 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -1,7 +1,7 @@
 Ext.define('PVE.pool.AddVM', {
     extend: 'Proxmox.window.Edit',
     width: 600,
-    height: 400,
+    height: 420,
     isAdd: true,
     isCreate: true,
     initComponent: function() {
@@ -30,7 +30,7 @@ Ext.define('PVE.pool.AddVM', {
 	    ],
 	    filters: [
 		function(item) {
-		    return (item.data.type === 'lxc' || item.data.type === 'qemu') && item.data.pool === '';
+		    return (item.data.type === 'lxc' || item.data.type === 'qemu') &&item.data.pool !== me.pool;
 		},
 	    ],
 	});
@@ -63,6 +63,10 @@ Ext.define('PVE.pool.AddVM', {
 		    header: gettext('Node'),
 		    dataIndex: 'node',
 		},
+		{
+		    header: gettext('Pool'),
+		    dataIndex: 'pool',
+		},
 		{
 		    header: gettext('Status'),
 		    dataIndex: 'uptime',
@@ -85,9 +89,16 @@ Ext.define('PVE.pool.AddVM', {
 		},
 	    ],
 	});
+
+	let transfer = Ext.create('Ext.form.field.Checkbox', {
+	    name: 'transfer',
+	    boxLabel: gettext('Allow Transfer'),
+	    inputValue: 1,
+	    value: 0,
+	});
 	Ext.apply(me, {
 	    subject: gettext('Virtual Machine'),
-	    items: [vmsField, vmGrid],
+	    items: [vmsField, vmGrid, transfer],
 	});
 
 	me.callParent();
-- 
2.39.2

[pve-devel] applied: [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Wolfgang Bumiller]

applied, thanks

@Dominik: does extjs have an 'enableFn' for rows in a grid?
IMO we should either disable the ones with pools when the transfer
checkbox is not checked, or hide them (but when hiding them after
already checking them... it's weird)
Or disable the 'Add' button if a VM with a pool is checked?

On Thu, Aug 10, 2023 at 12:09:00PM +0200, Philipp Hufnagl wrote:
> When a member of 2 pools wants to transfer a
>  vm/container to an other pool they can not do that. The vm/container would
>  have first to be removed form the current pool resulting in a loss of
>  privileges of the pool member for this vm/contianer. This feature introduces
>  a way to transfer a vm between pools, guarded by a checkbox from accidental
>  transfers
> 
> changes to v4:
> * check for allocate permissions of the originating pool
> 
> changes to v3:
> * fix subject typo
> * at version log
> 
> changes to v2:
> * split patch in front and backend
> 
> Philipp Hufnagl (2):
>   fix #474: api: allow transfer from container/vms
>   fix #474: ui: allow transfer from container/vms
> 
>  PVE/API2/Pool.pm                 | 16 ++++++++++++++--
>  www/manager6/grid/PoolMembers.js | 17 ++++++++++++++---
>  2 files changed, 28 insertions(+), 5 deletions(-)
> 
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] applied: [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Dominik Csapak]

On 8/14/23 12:36, Wolfgang Bumiller wrote:
> applied, thanks
> 
> @Dominik: does extjs have an 'enableFn' for rows in a grid?
> IMO we should either disable the ones with pools when the transfer
> checkbox is not checked, or hide them (but when hiding them after
> already checking them... it's weird)
> Or disable the 'Add' button if a VM with a pool is checked?
> 

'enableFn' is our invention ;) and no that only works for some of our components


looking just now at the gui patch, i would have approached it a bit differently:

always enable the 'transfer' property but show a 'warning' box when one is selected
with an old pool

since 'Allow Transfer' is rather non-descriptive (and no documentation is included)
and it adds needless friction on change
(i select a vm, click, get an error, have to select the vm again, click transfer, click button..)

also there is some whitespace error (missing space between && and 'item.data.poll')
don't know why eslint did not pick that up...

Re: [pve-devel] applied: [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Thomas Lamprecht]

Am 14/08/2023 um 12:42 schrieb Dominik Csapak:
> On 8/14/23 12:36, Wolfgang Bumiller wrote:
>> applied, thanks
>>
>> @Dominik: does extjs have an 'enableFn' for rows in a grid?
>> IMO we should either disable the ones with pools when the transfer
>> checkbox is not checked, or hide them (but when hiding them after
>> already checking them... it's weird)
>> Or disable the 'Add' button if a VM with a pool is checked?
>>
> 
> 'enableFn' is our invention ;) and no that only works for some of our components
> 
> 
> looking just now at the gui patch, i would have approached it a bit differently:
> 
> always enable the 'transfer' property but show a 'warning' box when one is selected
> with an old pool
> 
> since 'Allow Transfer' is rather non-descriptive (and no documentation is included)

+1, FWIW I had no idea what this series is about from just reading the subject,
as "pool" is not mentioned there.

> and it adds needless friction on change
> (i select a vm, click, get an error, have to select the vm again, click transfer, click button..)



We normally use "move" or "migrate", not "transfer", or "reassign" (like for
moving a guest disk to another guest) and it has some merits to not expand the
commonly used (parameter) naming scheme to much, but oh well it's already released
and a naming nit that doesn't matters _that_ much.

But the default isn't declared in the schema, please send a follow up for that.

And I agree with Dominik, UX isn't ideal, a warning that one or more VMID will
be moved out of there old Pool, if any, would be sufficient. Not sure if it'd be
better if that's a per-row hint, shown if the row is ticked (e.g., instead of the
Pool column) or a edit-window wide warning hint that gets made visible if any of
the selected VMIDs is in a Pool already.

FWIW, and not directly related (i.e., can be it's own series), you could also fix
the s/Virtual Machine/Virtual Guest/ wording to avoid the confusion that one also
adds Container over this interface.

Re: [pve-devel] applied: [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Philipp Hufnagl]

On 8/14/23 12:42, Dominik Csapak wrote:

> On 8/14/23 12:36, Wolfgang Bumiller wrote:
>> applied, thanks
>>
>> @Dominik: does extjs have an 'enableFn' for rows in a grid?
>> IMO we should either disable the ones with pools when the transfer
>> checkbox is not checked, or hide them (but when hiding them after
>> already checking them... it's weird)
>> Or disable the 'Add' button if a VM with a pool is checked?
>>
>
> 'enableFn' is our invention ;) and no that only works for some of our 
> components
>
>
> looking just now at the gui patch, i would have approached it a bit 
> differently:
>
> always enable the 'transfer' property but show a 'warning' box when 
> one is selected
> with an old pool
>
> since 'Allow Transfer' is rather non-descriptive (and no documentation 
> is included)
> and it adds needless friction on change
> (i select a vm, click, get an error, have to select the vm again, 
> click transfer, click button..)
>
> also there is some whitespace error (missing space between && and 
> 'item.data.poll')
> don't know why eslint did not pick that up...


I considered the option of a warning box. I decided against it because I 
thought it too verbose throwing a warning box every time the user wants 
to transfer/migrate.

As for the UI I agree that it could be improved. Would it solve it if 
the vms currently part of a pool are only shown AFTER the check box is 
checked? This way the User could never experience this error.

Re: [pve-devel] applied: [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Philipp Hufnagl]

On 8/24/23 16:46, Thomas Lamprecht wrote:
> Am 14/08/2023 um 12:42 schrieb Dominik Csapak:
>> On 8/14/23 12:36, Wolfgang Bumiller wrote:
>>> applied, thanks
>>>
>>> @Dominik: does extjs have an 'enableFn' for rows in a grid?
>>> IMO we should either disable the ones with pools when the transfer
>>> checkbox is not checked, or hide them (but when hiding them after
>>> already checking them... it's weird)
>>> Or disable the 'Add' button if a VM with a pool is checked?
>>>
>> 'enableFn' is our invention ;) and no that only works for some of our components
>>
>>
>> looking just now at the gui patch, i would have approached it a bit differently:
>>
>> always enable the 'transfer' property but show a 'warning' box when one is selected
>> with an old pool
>>
>> since 'Allow Transfer' is rather non-descriptive (and no documentation is included)
> +1, FWIW I had no idea what this series is about from just reading the subject,
> as "pool" is not mentioned there.
>
>> and it adds needless friction on change
>> (i select a vm, click, get an error, have to select the vm again, click transfer, click button..)
>
>
> We normally use "move" or "migrate", not "transfer", or "reassign" (like for
> moving a guest disk to another guest) and it has some merits to not expand the
> commonly used (parameter) naming scheme to much, but oh well it's already released
> and a naming nit that doesn't matters _that_ much.
>
> But the default isn't declared in the schema, please send a follow up for that.
>
> And I agree with Dominik, UX isn't ideal, a warning that one or more VMID will
> be moved out of there old Pool, if any, would be sufficient. Not sure if it'd be
> better if that's a per-row hint, shown if the row is ticked (e.g., instead of the
> Pool column) or a edit-window wide warning hint that gets made visible if any of
> the selected VMIDs is in a Pool already.
>
> FWIW, and not directly related (i.e., can be it's own series), you could also fix
> the s/Virtual Machine/Virtual Guest/ wording to avoid the confusion that one also
> adds Container over this interface.

Sorry for the issue. It has been my first Patch on this scale.

I will make a new patch Improving the user experience by renaming to 
"migrate" or "migrate from other pool" and fix the schema

As for the feature design itself: The UI could be improved by only 
showing vms assinged to a pool when the transfer/migrade check box is 
checked. This way it should be clear if it is a migration without the 
use of a popup.

Would that work? Feedback is most welcome :)

Re: [pve-devel] applied: [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Dominik Csapak]

On 8/30/23 14:43, Philipp Hufnagl wrote:
> On 8/14/23 12:42, Dominik Csapak wrote:
> 
>> On 8/14/23 12:36, Wolfgang Bumiller wrote:
>>> applied, thanks
>>>
>>> @Dominik: does extjs have an 'enableFn' for rows in a grid?
>>> IMO we should either disable the ones with pools when the transfer
>>> checkbox is not checked, or hide them (but when hiding them after
>>> already checking them... it's weird)
>>> Or disable the 'Add' button if a VM with a pool is checked?
>>>
>>
>> 'enableFn' is our invention ;) and no that only works for some of our components
>>
>>
>> looking just now at the gui patch, i would have approached it a bit differently:
>>
>> always enable the 'transfer' property but show a 'warning' box when one is selected
>> with an old pool
>>
>> since 'Allow Transfer' is rather non-descriptive (and no documentation is included)
>> and it adds needless friction on change
>> (i select a vm, click, get an error, have to select the vm again, click transfer, click button..)
>>
>> also there is some whitespace error (missing space between && and 'item.data.poll')
>> don't know why eslint did not pick that up...
> 
> 
> I considered the option of a warning box. I decided against it because I thought it too verbose 
> throwing a warning box every time the user wants to transfer/migrate.
> 

IMHO that contradicts itself, since now the user has to press a checkbox more than just read a 
warning. So with a warning, the user has *less* things to do than with the checkbox.
(just read, no clicking; users that know the warning will not read it anyway)

a warning per line would be even nicer, so that the user sees at once which vms are affected.

> As for the UI I agree that it could be improved. Would it solve it if the vms currently part of a 
> pool are only shown AFTER the check box is checked? This way the User could never experience this 
> error.
> 
> 

I don't think that's good because now the user has to either know beforehand it's in another pool,
or search the whole list, wondering where the vm is, just to click the checkbox and search again?

In general we try to make the UI as easy to use as possible, without having to impose too many
hurdles (except the action has severe consequences like deleting data!) but show appropriate
warnings when things might be unexpected (like here with the transferring of one pool to another,
since a user might expect that the vm is in both pools afterwards)

Re: [pve-devel] applied: [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Philipp Hufnagl]

On 8/30/23 16:29, Dominik Csapak wrote:
> On 8/30/23 14:43, Philipp Hufnagl wrote:
>> On 8/14/23 12:42, Dominik Csapak wrote:
>>
>>> On 8/14/23 12:36, Wolfgang Bumiller wrote:
>>>> applied, thanks
>>>>
>>>> @Dominik: does extjs have an 'enableFn' for rows in a grid?
>>>> IMO we should either disable the ones with pools when the transfer
>>>> checkbox is not checked, or hide them (but when hiding them after
>>>> already checking them... it's weird)
>>>> Or disable the 'Add' button if a VM with a pool is checked?
>>>>
>>>
>>> 'enableFn' is our invention ;) and no that only works for some of 
>>> our components
>>>
>>>
>>> looking just now at the gui patch, i would have approached it a bit 
>>> differently:
>>>
>>> always enable the 'transfer' property but show a 'warning' box when 
>>> one is selected
>>> with an old pool
>>>
>>> since 'Allow Transfer' is rather non-descriptive (and no 
>>> documentation is included)
>>> and it adds needless friction on change
>>> (i select a vm, click, get an error, have to select the vm again, 
>>> click transfer, click button..)
>>>
>>> also there is some whitespace error (missing space between && and 
>>> 'item.data.poll')
>>> don't know why eslint did not pick that up...
>>
>>
>> I considered the option of a warning box. I decided against it 
>> because I thought it too verbose throwing a warning box every time 
>> the user wants to transfer/migrate.
>>
>
> IMHO that contradicts itself, since now the user has to press a 
> checkbox more than just read a warning. So with a warning, the user 
> has *less* things to do than with the checkbox.
> (just read, no clicking; users that know the warning will not read it 
> anyway)
>
> a warning per line would be even nicer, so that the user sees at once 
> which vms are affected.
>
>> As for the UI I agree that it could be improved. Would it solve it if 
>> the vms currently part of a pool are only shown AFTER the check box 
>> is checked? This way the User could never experience this error.
>>
>>
>
> I don't think that's good because now the user has to either know 
> beforehand it's in another pool,
> or search the whole list, wondering where the vm is, just to click the 
> checkbox and search again?
>
> In general we try to make the UI as easy to use as possible, without 
> having to impose too many
> hurdles (except the action has severe consequences like deleting 
> data!) but show appropriate
> warnings when things might be unexpected (like here with the 
> transferring of one pool to another,
> since a user might expect that the vm is in both pools afterwards)
>
>
I misunderstood you before. Having a simple notification appear the way 
you descriped sounds like a good solution too. I will make a patch 
featuring that

Re: [pve-devel] applied: [PATCH manager v4 0/2] fix #474: allow transfer from container/vms

[Thomas Lamprecht]

Am 30/08/2023 um 14:53 schrieb Philipp Hufnagl:
> On 8/24/23 16:46, Thomas Lamprecht wrote:
>> And I agree with Dominik, UX isn't ideal, a warning that one or more VMID will
>> be moved out of there old Pool, if any, would be sufficient. Not sure if it'd be
>> better if that's a per-row hint, shown if the row is ticked (e.g., instead of the
>> Pool column) or a edit-window wide warning hint that gets made visible if any of
>> the selected VMIDs is in a Pool already.
>>
>> FWIW, and not directly related (i.e., can be it's own series), you could also fix
>> the s/Virtual Machine/Virtual Guest/ wording to avoid the confusion that one also
>> adds Container over this interface.
> 
> Sorry for the issue. It has been my first Patch on this scale.

Yeah, here to "blame" (exaggeration) is also Wolfgang applying the UI
side IMO a bit prematurely, without consulting Dominik or me for UX.

But it's still all the more important to be reactive to feedback
especially when starting out, otherwise reviewers might stop giving it
if they feel it's not heard anyway.

> As for the feature design itself: The UI could be improved by only showing vms assinged to a pool when the transfer/migrade check box is checked. This way it should be clear if it is a migration without the use of a popup.
> 
> Would that work? Feedback is most welcome :)


IMO that's a bit convoluted and still needs an extra step, as Dominik
and I both, rather avoid the checkbox completely.