* [pve-devel] [PATCH v5 manager] ui: vm network: allow to override MTU for virtio devices
@ 2022-05-19 8:48 Oguz Bektas
2022-05-20 7:41 ` Thomas Lamprecht
0 siblings, 1 reply; 4+ messages in thread
From: Oguz Bektas @ 2022-05-19 8:48 UTC (permalink / raw)
To: pve-devel
we already have the 'mtu' option in the API, so we can just expose
that option inside the 'Advanced' menu for virtio network interfaces.
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Tested-By: Dylan Whyte <d.whyte@proxmox.com>
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
v4->v5:
* changed default mtu from 1500 to empty (dominik's review on v4 caught
a small bug where the mtu would be set to 1500 even without showing the
advanced fields)
www/manager6/qemu/NetworkEdit.js | 34 ++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
index b39cffdc..91038de7 100644
--- a/www/manager6/qemu/NetworkEdit.js
+++ b/www/manager6/qemu/NetworkEdit.js
@@ -19,6 +19,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
me.network.macaddr = values.macaddr;
me.network.disconnect = values.disconnect;
me.network.queues = values.queues;
+ me.network.mtu = values.mtu;
if (values.rate) {
me.network.rate = values.rate;
@@ -33,6 +34,17 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
return params;
},
+ viewModel: {
+ data: {
+ network_model: '',
+ mtu: '',
+ },
+ formulas: {
+ isVirtio: (get) => get('network_model') === 'virtio',
+ showMTUHint: (get) => get('mtu') === 1,
+ },
+ },
+
setNetwork: function(confid, data) {
var me = this;
@@ -112,6 +124,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
'macaddr',
'rate',
'queues',
+ 'mtu',
];
fields.forEach(function(fieldname) {
me.down('field[name='+fieldname+']').setDisabled(value);
@@ -130,6 +143,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
xtype: 'pveNetworkCardSelector',
name: 'model',
fieldLabel: gettext('Model'),
+ bind: '{network_model}',
value: PVE.qemu.OSDefaults.generic.networkCard,
allowBlank: false,
},
@@ -161,6 +175,26 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
value: '',
allowBlank: true,
},
+ {
+ xtype: 'proxmoxintegerfield',
+ name: 'mtu',
+ fieldLabel: 'MTU',
+ bind: {
+ disabled: '{!isVirtio}',
+ value: '{mtu}',
+ },
+ minValue: 1,
+ maxValue: 65520,
+ allowBlank: true,
+ },
+ {
+ xtype: 'displayfield',
+ userCls: 'pmx-hint',
+ value: gettext("mtu=1 is a special value, the MTU value will be inherited from the current bridge"),
+ bind: {
+ hidden: '{!showMTUHint}',
+ },
+ },
];
me.callParent();
--
2.30.2
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH v5 manager] ui: vm network: allow to override MTU for virtio devices
2022-05-19 8:48 [pve-devel] [PATCH v5 manager] ui: vm network: allow to override MTU for virtio devices Oguz Bektas
@ 2022-05-20 7:41 ` Thomas Lamprecht
2022-05-20 8:03 ` Oguz Bektas
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Lamprecht @ 2022-05-20 7:41 UTC (permalink / raw)
To: Proxmox VE development discussion, Oguz Bektas
On 19/05/2022 10:48, Oguz Bektas wrote:
> we already have the 'mtu' option in the API, so we can just expose
> that option inside the 'Advanced' menu for virtio network interfaces.
>
> Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
> Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
> Tested-By: Dylan Whyte <d.whyte@proxmox.com>
aren't those from two version previous with some change in between, or
did they re-test / re-review this in private?
As IMO it's not really correct to let one believe that this exact version
was tested by two people.
Functionality wise it looks Ok, a few final layout and code style nits
I'd still like to see inline.
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>
> v4->v5:
> * changed default mtu from 1500 to empty (dominik's review on v4 caught
> a small bug where the mtu would be set to 1500 even without showing the
> advanced fields)
>
>
> www/manager6/qemu/NetworkEdit.js | 34 ++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
> index b39cffdc..91038de7 100644
> --- a/www/manager6/qemu/NetworkEdit.js
> +++ b/www/manager6/qemu/NetworkEdit.js
> @@ -19,6 +19,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> me.network.macaddr = values.macaddr;
> me.network.disconnect = values.disconnect;
> me.network.queues = values.queues;
> + me.network.mtu = values.mtu;
>
> if (values.rate) {
> me.network.rate = values.rate;
> @@ -33,6 +34,17 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> return params;
> },
>
> + viewModel: {
> + data: {
> + network_model: '',
nit: please avoid introducing new snake_case in the gui, we normally use camelCase here.
> + mtu: '',
> + },
> + formulas: {
> + isVirtio: (get) => get('network_model') === 'virtio',
tip, single parameter arrow functions don't need the parenthesis,
isVirtio: get => get('networkModel') === 'virtio',
> + showMTUHint: (get) => get('mtu') === 1,
showMtuHint
> + },
> + },
> +
> setNetwork: function(confid, data) {
> var me = this;
>
> @@ -112,6 +124,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> 'macaddr',
> 'rate',
> 'queues',
> + 'mtu',
> ];
> fields.forEach(function(fieldname) {
> me.down('field[name='+fieldname+']').setDisabled(value);
> @@ -130,6 +143,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> xtype: 'pveNetworkCardSelector',
> name: 'model',
> fieldLabel: gettext('Model'),
> + bind: '{network_model}',
> value: PVE.qemu.OSDefaults.generic.networkCard,
> allowBlank: false,
> },
> @@ -161,6 +175,26 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
> value: '',
> allowBlank: true,
> },
why not add the field to advancedColumn1 and the hint in advancedColumB, so that
the form is more balanced out and the hint isn't wrapping lines increasing the
height of the row its in (even if there wasn't anything _yet_ on the left side of
it to really notice).
> + {
> + xtype: 'proxmoxintegerfield',
> + name: 'mtu',
> + fieldLabel: 'MTU',
> + bind: {
> + disabled: '{!isVirtio}',
> + value: '{mtu}',
> + },
> + minValue: 1,
> + maxValue: 65520,
> + allowBlank: true,
> + },
> + {
> + xtype: 'displayfield',
> + userCls: 'pmx-hint',
> + value: gettext("mtu=1 is a special value, the MTU value will be inherited from the current bridge"),
Maybe:
"Use the special value '1' to inherit the MTU from the underlying bridge"
> + bind: {
> + hidden: '{!showMTUHint}',
> + },
> + },
> ];
>
> me.callParent();
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH v5 manager] ui: vm network: allow to override MTU for virtio devices
2022-05-20 7:41 ` Thomas Lamprecht
@ 2022-05-20 8:03 ` Oguz Bektas
2022-05-20 8:19 ` Thomas Lamprecht
0 siblings, 1 reply; 4+ messages in thread
From: Oguz Bektas @ 2022-05-20 8:03 UTC (permalink / raw)
To: Thomas Lamprecht; +Cc: Proxmox VE development discussion
On Fri, May 20, 2022 at 09:41:10AM +0200, Thomas Lamprecht wrote:
> On 19/05/2022 10:48, Oguz Bektas wrote:
> > we already have the 'mtu' option in the API, so we can just expose
> > that option inside the 'Advanced' menu for virtio network interfaces.
> >
> > Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
> > Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
> > Tested-By: Dylan Whyte <d.whyte@proxmox.com>
>
> aren't those from two version previous with some change in between, or
> did they re-test / re-review this in private?
> As IMO it's not really correct to let one believe that this exact version
> was tested by two people.
sorry for the confusion, the tags were leftover from the previous
commits
>
> Functionality wise it looks Ok, a few final layout and code style nits
> I'd still like to see inline.
i've addressed the nits in the v6.
though i don't think the
> "Use the special value '1' to inherit the MTU from the underlying bridge"
is the best, since the user won't see this message until they've
selected '1' as a value.
regardless, i've made the changes you asked for
- oguz
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH v5 manager] ui: vm network: allow to override MTU for virtio devices
2022-05-20 8:03 ` Oguz Bektas
@ 2022-05-20 8:19 ` Thomas Lamprecht
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2022-05-20 8:19 UTC (permalink / raw)
To: Oguz Bektas, Proxmox VE development discussion
On 20/05/2022 10:03, Oguz Bektas wrote:
> though i don't think the
>> "Use the special value '1' to inherit the MTU from the underlying bridge"
> is the best, since the user won't see this message until they've
> selected '1' as a value.
that is a good point but I actually had the wrong assumption that it's always
shown if the MTU field is shown, which could be OK to do for an advanced field
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-20 8:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-19 8:48 [pve-devel] [PATCH v5 manager] ui: vm network: allow to override MTU for virtio devices Oguz Bektas
2022-05-20 7:41 ` Thomas Lamprecht
2022-05-20 8:03 ` Oguz Bektas
2022-05-20 8:19 ` Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox