[pve-devel] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation

[pve-devel] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation

[Michael Köppl]

The current implementation is slightly misleading. When creating a
privileged container, the nesting checkbox is disabled but keeps its
current state. However, nesting is not enabled for privileged containers
even if the checkbox was set to true. Enabling nesting is still possible
through the Options menu.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
As an alternative to this, since we already discourage 
the use of privileged containers [0], removing the checkbox for creating privileged
containers in the web UI might make sense. For the rare cases where they
are required, they can still be created using pct (although the question
remains whether privileged should be the default for pct create).

[0] https://forum.proxmox.com/threads/debian-12-lxc-template-systemd-failures.151630/post-686850

 www/manager6/lxc/CreateWizard.js | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js
index 62cda27a..c7ee56f7 100644
--- a/www/manager6/lxc/CreateWizard.js
+++ b/www/manager6/lxc/CreateWizard.js
@@ -7,6 +7,7 @@ Ext.define('PVE.lxc.CreateWizard', {
 	    nodename: '',
 	    storage: '',
 	    unprivileged: true,
+	    nestingEnabled: true,
 	},
 	formulas: {
 	    cgroupMode: function(get) {
@@ -69,14 +70,22 @@ Ext.define('PVE.lxc.CreateWizard', {
 			value: '{unprivileged}',
 		    },
 		    fieldLabel: gettext('Unprivileged container'),
+		    listeners: {
+			change: function(checkbox, value) {
+			    var viewModel = checkbox.lookupViewModel();
+			    if (!value && viewModel) {
+				viewModel.set('nestingEnabled', false);
+			    }
+			},
+		    },
 		},
 		{
 		    xtype: 'proxmoxcheckbox',
 		    name: 'features',
 		    inputValue: 'nesting=1',
-		    value: true,
 		    bind: {
 			disabled: '{!unprivileged}',
+			value: '{nestingEnabled}',
 		    },
 		    fieldLabel: gettext('Nesting'),
 		},
-- 
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] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation

[Thomas Lamprecht]

Thanks for this patch, some comments inline.

Am 18.03.25 um 17:14 schrieb Michael Köppl:
> The current implementation is slightly misleading. When creating a
> privileged container, the nesting checkbox is disabled but keeps its
> current state. However, nesting is not enabled for privileged containers
> even if the checkbox was set to true. Enabling nesting is still possible
> through the Options menu.
> 
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> As an alternative to this, since we already discourage 
> the use of privileged containers [0], removing the checkbox for creating privileged
> containers in the web UI might make sense. For the rare cases where they
> are required, they can still be created using pct (although the question
> remains whether privileged should be the default for pct create).
> 
> [0] https://forum.proxmox.com/threads/debian-12-lxc-template-systemd-failures.151630/post-686850
> 
>  www/manager6/lxc/CreateWizard.js | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js
> index 62cda27a..c7ee56f7 100644
> --- a/www/manager6/lxc/CreateWizard.js
> +++ b/www/manager6/lxc/CreateWizard.js
> @@ -7,6 +7,7 @@ Ext.define('PVE.lxc.CreateWizard', {
>  	    nodename: '',
>  	    storage: '',
>  	    unprivileged: true,
> +	    nestingEnabled: true,

This works fine as is, but I think there is a better solution available here
(see below) and such use of viewModel data binding can come back and bite one
when extending/refactoring such code, as the code using it needs to be careful
to not interfere with the automatic two-way binding to the field's value.
I do not want to discourage using the viewModel, most of the time it can be
fine and result in even nicer code overall, but it has its oddities.

>  	},
>  	formulas: {
>  	    cgroupMode: function(get) {
> @@ -69,14 +70,22 @@ Ext.define('PVE.lxc.CreateWizard', {
>  			value: '{unprivileged}',
>  		    },
>  		    fieldLabel: gettext('Unprivileged container'),
> +		    listeners: {
> +			change: function(checkbox, value) {
> +			    var viewModel = checkbox.lookupViewModel();

style nit: we mainly use `let` variables for new code as that has less confusing
scope rules, i.e. variables declared through var are also available in the parent
block scope.

> +			    if (!value && viewModel) {
> +				viewModel.set('nestingEnabled', false);

You could also avoid the intermediate viewModel variable that is used just once
and use an optional-chaining operator, like:

    checkbox.lookupViewModel()?.set('nestingEnabled', false);

> +			    }
> +			},
> +		    },
>  		},
>  		{
>  		    xtype: 'proxmoxcheckbox',

FYI that component comes from our proxmox-widget-toolkit and is a small override
of the original ExtJS checkbox. One thing it provides is a `clearOnDisable`
config flag that if set to true should exactly provide the behavior you want
here without needing to hook into the unprivileged change listener.

See src/form/Checkbox.js in the proxmox-widget-toolkit for the rather trivial
implementation of that flag.

>  		    name: 'features',
>  		    inputValue: 'nesting=1',
> -		    value: true,
>  		    bind: {
>  			disabled: '{!unprivileged}',
> +			value: '{nestingEnabled}',
>  		    },
>  		    fieldLabel: gettext('Nesting'),
>  		},



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

Re: [pve-devel] [RFC PATCH] ui: lxc: set nesting to false for privileged container during creation

[Michael Köppl]

Superseded byhttps://lore.proxmox.com/pve-devel/20250321103511.66722-1-m.koeppl@proxmox.com/

Thank you for having a look and your detailed suggestions!

> style nit: we mainly use `let` variables for new code as that has less confusing
> scope rules, i.e. variables declared through var are also available in the parent
> block scope.

Also thanks for this hint. Makes sense, will adhere to it for future patches.

On 3/21/25 08:58, Thomas Lamprecht wrote:
> Thanks for this patch, some comments inline.
>
> Am 18.03.25 um 17:14 schrieb Michael Köppl:
>> The current implementation is slightly misleading. When creating a
>> privileged container, the nesting checkbox is disabled but keeps its
>> current state. However, nesting is not enabled for privileged containers
>> even if the checkbox was set to true. Enabling nesting is still possible
>> through the Options menu.
>>
>> Signed-off-by: Michael Köppl<m.koeppl@proxmox.com>
>> ---
>> As an alternative to this, since we already discourage
>> the use of privileged containers [0], removing the checkbox for creating privileged
>> containers in the web UI might make sense. For the rare cases where they
>> are required, they can still be created using pct (although the question
>> remains whether privileged should be the default for pct create).
>>
>> [0]https://forum.proxmox.com/threads/debian-12-lxc-template-systemd-failures.151630/post-686850
>>
>>   www/manager6/lxc/CreateWizard.js | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/www/manager6/lxc/CreateWizard.js b/www/manager6/lxc/CreateWizard.js
>> index 62cda27a..c7ee56f7 100644
>> --- a/www/manager6/lxc/CreateWizard.js
>> +++ b/www/manager6/lxc/CreateWizard.js
>> @@ -7,6 +7,7 @@ Ext.define('PVE.lxc.CreateWizard', {
>>   	    nodename: '',
>>   	    storage: '',
>>   	    unprivileged: true,
>> +	    nestingEnabled: true,
> This works fine as is, but I think there is a better solution available here
> (see below) and such use of viewModel data binding can come back and bite one
> when extending/refactoring such code, as the code using it needs to be careful
> to not interfere with the automatic two-way binding to the field's value.
> I do not want to discourage using the viewModel, most of the time it can be
> fine and result in even nicer code overall, but it has its oddities.
>
>>   	},
>>   	formulas: {
>>   	    cgroupMode: function(get) {
>> @@ -69,14 +70,22 @@ Ext.define('PVE.lxc.CreateWizard', {
>>   			value: '{unprivileged}',
>>   		    },
>>   		    fieldLabel: gettext('Unprivileged container'),
>> +		    listeners: {
>> +			change: function(checkbox, value) {
>> +			    var viewModel = checkbox.lookupViewModel();
> style nit: we mainly use `let` variables for new code as that has less confusing
> scope rules, i.e. variables declared through var are also available in the parent
> block scope.
>
>> +			    if (!value && viewModel) {
>> +				viewModel.set('nestingEnabled', false);
> You could also avoid the intermediate viewModel variable that is used just once
> and use an optional-chaining operator, like:
>
>      checkbox.lookupViewModel()?.set('nestingEnabled', false);
>
>> +			    }
>> +			},
>> +		    },
>>   		},
>>   		{
>>   		    xtype: 'proxmoxcheckbox',
> FYI that component comes from our proxmox-widget-toolkit and is a small override
> of the original ExtJS checkbox. One thing it provides is a `clearOnDisable`
> config flag that if set to true should exactly provide the behavior you want
> here without needing to hook into the unprivileged change listener.
>
> See src/form/Checkbox.js in the proxmox-widget-toolkit for the rather trivial
> implementation of that flag.
>
>>   		    name: 'features',
>>   		    inputValue: 'nesting=1',
>> -		    value: true,
>>   		    bind: {
>>   			disabled: '{!unprivileged}',
>> +			value: '{nestingEnabled}',
>>   		    },
>>   		    fieldLabel: gettext('Nesting'),
>>   		},
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel