[pve-devel] [PATCH manager v2] ui: resource mappings: fix editing of mapping for non first node
when editing the pci mapping, we set the nodename of the pci/usbselector
to the selected node. At the same time we disable and hide the node
selector, but it still changes it's value to the 'first' node
(alphabetically sorted) and that triggers a change event.
To prevent that we accidentally set the node of the pci/usbselector
too, we need to check here if the field is disabled.
There seems to be a race when loading the nodes for the nodeselector
which leads to inconsistent behaviour, so this was only encountered for
the pciselector, but theoretically it could also happen for the
usbselector so adding that condition to both.
Signed-off-by: Dominik Csapak
---
changes from v1:
* also fixed it for usbselector, but mention in the commit message that
this is just a safety measure and was never encountered there
www/manager6/window/PCIMapEdit.js | 6 --
www/manager6/window/USBMapEdit.js | 8 +---
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/www/manager6/window/PCIMapEdit.js
b/www/manager6/window/PCIMapEdit.js
index d43f04eb..faf58255 100644
--- a/www/manager6/window/PCIMapEdit.js
+++ b/www/manager6/window/PCIMapEdit.js
@@ -126,8 +126,10 @@ Ext.define('PVE.window.PCIMapEditWindow', {
this.lookup('pciselector').setMdev(value);
},
- nodeChange: function(_field, value) {
- this.lookup('pciselector').setNodename(value);
+ nodeChange: function(field, value) {
+ if (!field.isDisabled()) {
+ this.lookup('pciselector').setNodename(value);
+ }
},
pciChange: function(_field, value) {
diff --git a/www/manager6/window/USBMapEdit.js
b/www/manager6/window/USBMapEdit.js
index 358f0778..69a40026 100644
--- a/www/manager6/window/USBMapEdit.js
+++ b/www/manager6/window/USBMapEdit.js
@@ -101,9 +101,11 @@ Ext.define('PVE.window.USBMapEditWindow', {
usbsel.setDisabled(!value);
},
- nodeChange: function(_field, value) {
- this.lookup('id').setNodename(value);
- this.lookup('path').setNodename(value);
+ nodeChange: function(field, value) {
+ if (!field.isDisabled()) {
+ this.lookup('id').setNodename(value);
+ this.lookup('path').setNodename(value);
+ }
},
--
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] [PATCH manager] ui: resource mappings: fix editing of mapping for non first node
sent a v2: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064916.html ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH docs] passthrough: viommu: replace host requirement with reference to pcie passthrough
Enabling IOMMU on the host is not a requirement for vIOMMU. It is only a requirement for passthrough. Add a sentence to clarify the need for a configured PCI(e) passthrough on the host for passthrough to nested VMs. Suggested-by: Dominik Csapak Signed-off-by: Markus Frank --- This patch replaces the "[PATCH docs] passthrough: viommu: remove 'amd_iommu=on' from the docs" patch: https://lists.proxmox.com/pipermail/pve-devel/2024-April/063688.html qm-pci-passthrough.adoc | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/qm-pci-passthrough.adoc b/qm-pci-passthrough.adoc index bbd6b85..fd0872e 100644 --- a/qm-pci-passthrough.adoc +++ b/qm-pci-passthrough.adoc @@ -505,14 +505,12 @@ vIOMMU (emulated IOMMU) vIOMMU is the emulation of a hardware IOMMU within a virtual machine, providing improved memory access control and security for virtualized I/O devices. Using -the vIOMMU option also allows you to pass through PCI devices to level-2 VMs in -level-1 VMs via https://pve.proxmox.com/wiki/Nested_Virtualization[Nested Virtualization]. -There are currently two vIOMMU implementations available: Intel and VirtIO. - -Host requirement: +the vIOMMU option also allows you to pass through PCI(e) devices to level-2 VMs +in level-1 VMs via https://pve.proxmox.com/wiki/Nested_Virtualization[Nested Virtualization]. +To pass through physical PCI(e) devices from the host to nested VMs, follow the +PCI(e) passthrough instructions. -* Add `intel_iommu=on` or `amd_iommu=on` depending on your CPU to your kernel -command line. +There are currently two vIOMMU implementations available: Intel and VirtIO. Intel vIOMMU -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH kernel] add fix for regression causing broken power indicators/LEDs
The issue was reported in the enterprise support. The customer
contacted the ledmon maintainer, who found that it is not an issue
with ledmon, bisected the kernel and came up with this fix.
Signed-off-by: Fiona Ebner
---
...n-Power-Indicator-bits-for-userspace.patch | 56 +++
1 file changed, 56 insertions(+)
create mode 100644
patches/kernel/0021-PCI-pciehp-Retain-Power-Indicator-bits-for-userspace.patch
diff --git
a/patches/kernel/0021-PCI-pciehp-Retain-Power-Indicator-bits-for-userspace.patch
b/patches/kernel/0021-PCI-pciehp-Retain-Power-Indicator-bits-for-userspace.patch
new file mode 100644
index 000..7b94bcf
--- /dev/null
+++
b/patches/kernel/0021-PCI-pciehp-Retain-Power-Indicator-bits-for-userspace.patch
@@ -0,0 +1,56 @@
+From Mon Sep 17 00:00:00 2001
+From: Blazej Kucman
+Date: Mon, 22 Jul 2024 16:14:40 +0200
+Subject: [PATCH] PCI: pciehp: Retain Power Indicator bits for userspace
+ indicators
+
+The sysfs "attention" file normally controls the Slot Control Attention
+Indicator with 0 (off), 1 (on), 2 (blink) settings.
+
+576243b3f9ea ("PCI: pciehp: Allow exclusive userspace control of
+indicators") added pciehp_set_raw_indicator_status() to allow userspace to
+directly control all four bits in both the Attention Indicator and the
+Power Indicator fields via the "attention" file.
+
+This is used on Intel VMD bridges so utilities like "ledmon" can use sysfs
+"attention" to control up to 16 indicators for NVMe device RAID status.
+
+abaaac4845a0 ("PCI: hotplug: Use FIELD_GET/PREP()") broke this by masking
+the sysfs data with PCI_EXP_SLTCTL_AIC, which discards the upper two bits
+intended for the Power Indicator Control field (PCI_EXP_SLTCTL_PIC).
+
+For NVMe devices behind an Intel VMD, ledmon settings that use the
+PCI_EXP_SLTCTL_PIC bits, i.e., ATTENTION_REBUILD (0x5), ATTENTION_LOCATE
+(0x7), ATTENTION_FAILURE (0xD), ATTENTION_OFF (0xF), no longer worked
+correctly.
+
+Mask with PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC to retain both the
+Attention Indicator and the Power Indicator bits.
+
+Fixes: abaaac4845a0 ("PCI: hotplug: Use FIELD_GET/PREP()")
+Link: https://lore.kernel.org/r/20240722141440.7210-1-blazej.kuc...@intel.com
+Signed-off-by: Blazej Kucman
+[bhelgaas: commit log]
+Signed-off-by: Bjorn Helgaas
+Cc: sta...@vger.kernel.org # v6.7+
+(picked from
https://lore.kernel.org/linux-pci/20240722141440.7210-1-blazej.kuc...@intel.com/T/#u)
+Signed-off-by: Fiona Ebner
+---
+ drivers/pci/hotplug/pciehp_hpc.c | 4 +++-
+ 1 file changed, 3 insertions(+), 1 deletion(-)
+
+diff --git a/drivers/pci/hotplug/pciehp_hpc.c
b/drivers/pci/hotplug/pciehp_hpc.c
+index b1d0a1b3917d..9d3c249207c4 100644
+--- a/drivers/pci/hotplug/pciehp_hpc.c
b/drivers/pci/hotplug/pciehp_hpc.c
+@@ -485,7 +485,9 @@ int pciehp_set_raw_indicator_status(struct hotplug_slot
*hotplug_slot,
+ struct pci_dev *pdev = ctrl_dev(ctrl);
+
+ pci_config_pm_runtime_get(pdev);
+- pcie_write_cmd_nowait(ctrl, FIELD_PREP(PCI_EXP_SLTCTL_AIC, status),
++
++ /* Attention and Power Indicator Control bits are supported */
++ pcie_write_cmd_nowait(ctrl, FIELD_PREP(PCI_EXP_SLTCTL_AIC |
PCI_EXP_SLTCTL_PIC, status),
+ PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC);
+ pci_config_pm_runtime_put(pdev);
+ return 0;
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH manager v2] ui: resource mappings: fix editing of mapping for non first node
Am 26.07.24 um 09:40 schrieb Dominik Csapak: > when editing the pci mapping, we set the nodename of the pci/usbselector > to the selected node. At the same time we disable and hide the node > selector, but it still changes it's value to the 'first' node > (alphabetically sorted) and that triggers a change event. > > To prevent that we accidentally set the node of the pci/usbselector > too, we need to check here if the field is disabled. > > There seems to be a race when loading the nodes for the nodeselector > which leads to inconsistent behaviour, so this was only encountered for > the pciselector, but theoretically it could also happen for the > usbselector so adding that condition to both. > > Signed-off-by: Dominik Csapak applied, thanks! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
Am 25.07.24 um 17:32 schrieb Max Carrara:
> On Thu Jul 25, 2024 at 3:11 PM CEST, Fiona Ebner wrote:
>> Am 25.07.24 um 11:48 schrieb Max Carrara:
>>> The same goes for backup provider plugins - IMO namespacing them
>>> like e.g. `PVE::Backup::Provider::Plugin::Foo` where `Foo` is a
>>> (concrete) plugin.
>>>
>>
>> The BackupProvider namespace is already intended for the plugins, adding
>> an extra level with "Plugin" would just bloat the module names,
>> especially if we decide to go the same route as for storage plugins and
>> have a "Custom" sub-namespace.
>
> I understand what you mean, yeah. Would perhaps something like
> `PVE::BackupProvider::Plugin::*` be better?
>
> The reason why I'm suggesting this is because in `PVE::Storage::*`,
> every plugin lives alongside `Plugin.pm`, even though the extra
> directory wouldn't really hurt IMO. E.g. `PVE::Storage::DirPlugin` would
> then be `PVE::Storage::Plugin::Dir`.
>
I think it's fine to live alongside the base plugin (I'd prefer
Plugin::Base if going for a dedicated directory). I agree, if we ever
want to add something other than plugins to the top namespace, it is
much nicer to have the dedicated directory. And it is made more explicit
that things in there are plugins (and not having to name each one
FooPlugin). However, I still feel like
PVE::BackupProvider::Plugin::Custom::Bar is rather lengthy (should we go
with the Custom directory again), but I'm not really opposed to doing it
like this.
>>
>>> The above two methods - `backup_nbd` and `backup_directory` - is there
>>> perhaps a way to merge them? I'm not sure if what I'm having in mind
>>> here is actually feasible, but what I mean is "making the method
>>> agnostic to the type of backup". As in, perhaps pass a hash that
>>> contains a `type` key for the type of backup being made, and instead of
>>> having long method signatures, include the remaining parameters as the
>>> remaining keys. For example:
>>>
>>> {
>>> 'type' => 'lxc-dir', # type names are just examples here
>>> 'directory' => '/foo/bar/baz',
>>> 'bandwidth_limit' => 42,
>>> ...
>>> }
>>>
>>> {
>>> 'type' => 'vm-nbd',
>>> 'device_name' => '...',
>>> 'nbd_path' => '...',
>>> ...
>>> }
>>>
>>> You get the point :P
>>>
>>> IMO it would make it easier to extend later, and also make it more
>>> straightforward to introduce new parameters / deprecate old ones, while
>>> the method signature stays stable otherwise.
>>>
>>> The same goes for the different cleanup methods further down below;
>>> instead of having a separate method for each "type of cleanup being
>>> performed", let the implementor handle it according to the data the
>>> method receives.
>>>
>>> IMHO I think it's best to be completely agnostic over VM / LXC backups
>>> (and their specific types) wherever possible and let the data describe
>>> what's going on instead.
>>>
>>
>> The point about extensibility is a good one. The API wouldn't need to
>> change even if we implement new mechanisms. But thinking about it some
>> more, is there anything really gained? Because we will not force plugins
>> to implement the methods for new mechanisms of course, they can just
>> continue supporting what they support. Each mechanism will have its own
>> specific set of parameters, so throwing everything into a catch-all
>> method and hash might make it too generic.
>
> The main point is indeed extensibility, but it also makes maintaining
> the API a bit easier. Should we (in the future) decide to add or remove
> any parameters, we don't need to touch the signature - and in turn, we
> don't need to tediously `grep` for every call site to ensure that
> they're updated accordingly.
>
> With hashes one could instead always just check if the required
> arguments have been provided.
>
I'm all for going with a similar API age + version mechanism like for
the storage plugins, so removing parameters should not be done except
for major releases and adding will be backwards-compatible.
I don't quite get your point about not needing to update the call sites.
If you change the structure of the passed-in hash you still need to do that.
I do see some benefit in not needing to add new methods for every
mechanism, and there could also be a single backup_hook() method instead
of a dedicated one for each phase while we're at it. But changes to the
passed-in hashes will still fall under the same restrictions like
changing a signature API-wise, so users will be informed via API age +
version.
>>
>> Or think about the documentation for the single backup method: it would
>> become super lengthy and describe all backup mechanisms, while a plugin
>> most likely only cares about a single one and would have an easier time
>> with a method that captures that mechanism's parameters explicitly.
>> Won't the end result be making the implementors life slightly harder,
>> because it first needs to extract the parameters for the specific mechanism?
>
> Yes, I agree - th
Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
On Fri Jul 26, 2024 at 11:52 AM CEST, Fiona Ebner wrote:
> Am 25.07.24 um 17:32 schrieb Max Carrara:
> > On Thu Jul 25, 2024 at 3:11 PM CEST, Fiona Ebner wrote:
> >> Am 25.07.24 um 11:48 schrieb Max Carrara:
> >>> The same goes for backup provider plugins - IMO namespacing them
> >>> like e.g. `PVE::Backup::Provider::Plugin::Foo` where `Foo` is a
> >>> (concrete) plugin.
> >>>
> >>
> >> The BackupProvider namespace is already intended for the plugins, adding
> >> an extra level with "Plugin" would just bloat the module names,
> >> especially if we decide to go the same route as for storage plugins and
> >> have a "Custom" sub-namespace.
> >
> > I understand what you mean, yeah. Would perhaps something like
> > `PVE::BackupProvider::Plugin::*` be better?
> >
> > The reason why I'm suggesting this is because in `PVE::Storage::*`,
> > every plugin lives alongside `Plugin.pm`, even though the extra
> > directory wouldn't really hurt IMO. E.g. `PVE::Storage::DirPlugin` would
> > then be `PVE::Storage::Plugin::Dir`.
> >
>
> I think it's fine to live alongside the base plugin (I'd prefer
> Plugin::Base if going for a dedicated directory). I agree, if we ever
> want to add something other than plugins to the top namespace, it is
> much nicer to have the dedicated directory. And it is made more explicit
> that things in there are plugins (and not having to name each one
> FooPlugin). However, I still feel like
> PVE::BackupProvider::Plugin::Custom::Bar is rather lengthy (should we go
> with the Custom directory again), but I'm not really opposed to doing it
> like this.
>
> >>
> >>> The above two methods - `backup_nbd` and `backup_directory` - is there
> >>> perhaps a way to merge them? I'm not sure if what I'm having in mind
> >>> here is actually feasible, but what I mean is "making the method
> >>> agnostic to the type of backup". As in, perhaps pass a hash that
> >>> contains a `type` key for the type of backup being made, and instead of
> >>> having long method signatures, include the remaining parameters as the
> >>> remaining keys. For example:
> >>>
> >>> {
> >>> 'type' => 'lxc-dir', # type names are just examples here
> >>> 'directory' => '/foo/bar/baz',
> >>> 'bandwidth_limit' => 42,
> >>> ...
> >>> }
> >>>
> >>> {
> >>> 'type' => 'vm-nbd',
> >>> 'device_name' => '...',
> >>> 'nbd_path' => '...',
> >>> ...
> >>> }
> >>>
> >>> You get the point :P
> >>>
> >>> IMO it would make it easier to extend later, and also make it more
> >>> straightforward to introduce new parameters / deprecate old ones, while
> >>> the method signature stays stable otherwise.
> >>>
> >>> The same goes for the different cleanup methods further down below;
> >>> instead of having a separate method for each "type of cleanup being
> >>> performed", let the implementor handle it according to the data the
> >>> method receives.
> >>>
> >>> IMHO I think it's best to be completely agnostic over VM / LXC backups
> >>> (and their specific types) wherever possible and let the data describe
> >>> what's going on instead.
> >>>
> >>
> >> The point about extensibility is a good one. The API wouldn't need to
> >> change even if we implement new mechanisms. But thinking about it some
> >> more, is there anything really gained? Because we will not force plugins
> >> to implement the methods for new mechanisms of course, they can just
> >> continue supporting what they support. Each mechanism will have its own
> >> specific set of parameters, so throwing everything into a catch-all
> >> method and hash might make it too generic.
> >
> > The main point is indeed extensibility, but it also makes maintaining
> > the API a bit easier. Should we (in the future) decide to add or remove
> > any parameters, we don't need to touch the signature - and in turn, we
> > don't need to tediously `grep` for every call site to ensure that
> > they're updated accordingly.
> >
> > With hashes one could instead always just check if the required
> > arguments have been provided.
> >
>
> I'm all for going with a similar API age + version mechanism like for
> the storage plugins, so removing parameters should not be done except
> for major releases and adding will be backwards-compatible.
>
> I don't quite get your point about not needing to update the call sites.
> If you change the structure of the passed-in hash you still need to do that.
Pardon me, I was a bit imprecise there. You're completely right that the
passed hash has to be changed as well, of course.
What I meant in particular was that because the signature (most likely)
won't change, we won't have to take special care to update the
individual arguments that are passed to a method when e.g. a parameter
is deprecated (or removed) or added.
For example, optional arguments are often just left out (because that's
possible in Perl), so if we introduced another parameter ...
* after the optional one, we'd have to pass something like `0` or
`undef` for the opt
Re: [pve-devel] [PATCH common, widget-toolkit, manager v3 0/3] fix #3893: make bridge vids configurable
On Wed Jul 3, 2024 at 10:01 AM CEST, Aaron Lauterer wrote: > this version reworks a few parts since v2. > > * renamed format in JSONSchema to a more generic `pve-vlan-id-or-range` > * explicitly use spaces when writing interfaces file. This is one > possible approach to deal with the fact, that the generic `-list` > format will accept quite a few delimiters and we need spaces. > * code style improvements such as naming the regex results. > * add parameter verification to the web ui > > With the changes to the JSONSchema we can then work on using it too for > the guest trunk option. This hasn't been started yet though. > > common: Aaron Lauterer (3): > tools: add check_list_empty function > fix #3893: network: add vlan id and range parameter definitions > inotify: interfaces: make sure bridge_vids use space as separator > > src/PVE/INotify.pm| 2 +- > src/PVE/JSONSchema.pm | 34 ++ > src/PVE/Tools.pm | 8 > 3 files changed, 43 insertions(+), 1 deletion(-) > > > widget-toolkit: Aaron Lauterer (1): > fix #3892: Network: add bridge vids field for bridge_vids > > src/node/NetworkEdit.js | 62 + > src/node/NetworkView.js | 5 > 2 files changed, 67 insertions(+) > > > manager: Aaron Lauterer (2): > fix #3893: api: network: add bridge_vids parameter > fix #3893: ui: network: enable bridge_vids field > > PVE/API2/Network.pm | 15 ++- > www/manager6/node/Config.js | 1 + > 2 files changed, 15 insertions(+), 1 deletion(-) thanks for this, i did a short review of the code. i have a couple of minor issue, but other than that this series: Reviewed-by: Shannon Sterz ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH widget-toolkit v3] fix #3892: Network: add bridge vids field for bridge_vids
On Wed Jul 3, 2024 at 10:01 AM CEST, Aaron Lauterer wrote:
> The new optional bridge_vids field allows to set that property via the
> GUI. Since the backend needs to support it, the field needs to be
> explicitly enabled.
>
> For now, Proxmox VE (PVE) is the use case.
>
> Signed-off-by: Aaron Lauterer
> ---
> changes since v2:
> * added validation code following how it is implemented in the API
>
> src/node/NetworkEdit.js | 62 +
> src/node/NetworkView.js | 5
> 2 files changed, 67 insertions(+)
>
> diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js
> index 27c1baf..8c1b135 100644
> --- a/src/node/NetworkEdit.js
> +++ b/src/node/NetworkEdit.js
> @@ -2,6 +2,9 @@ Ext.define('Proxmox.node.NetworkEdit', {
> extend: 'Proxmox.window.Edit',
> alias: ['widget.proxmoxNodeNetworkEdit'],
>
> +// Enable to show the VLAN ID field
> +bridge_set_vids: false,
> +
> initComponent: function() {
> let me = this;
>
> @@ -57,11 +60,67 @@ Ext.define('Proxmox.node.NetworkEdit', {
> }
>
> if (me.iftype === 'bridge') {
> + let vids = Ext.create('Ext.form.field.Text', {
> + fieldLabel: gettext('Bridge VIDS'),
> + name: 'bridge_vids',
> + emptyText: '2-4094',
> + disabled: true,
> + autoEl: {
> + tag: 'div',
> + 'data-qtip': gettext('Space-separated list of VLANs and
> ranges, for example: 2 4 100-200'),
> + },
> + validator: function(value) {
> + if (!value) { return true; } // Empty
nit: our js style guide state that single line ifs should be avoided
> + let result = true;
> +
> + let vid_list = value.split(' ');
> +
> + let checkVid = function(tag) {
> + if (tag < 2 || tag > 4094) {
> + return `not a valid VLAN ID '${tag}'`;
> + }
> + return true;
> + };
> +
> + for (const vid of vid_list) {
> + if (!vid) {
> + continue;
> + }
> + let res = vid.match(/^(\d+)([-](\d+))?$/i);
nit: see my comment for patch 2.
> + if (!res) {
> + return `not a valid VLAN configuration '${vid}'`;
> + }
> + let start = Number(res[1]);
> + let end = Number(res[3]);
> +
> + if (start) {
> + result = checkVid(start);
> + if (result !== true) { return result; }
same here...
> + }
> + if (end) {
> + result = checkVid(end);
> + if (result !== true) { return result; }
and here. it might be more elegant to do this:
let invalidVid = function(tag) {
if (!isNaN(tag) && (tag < 2 || tag > 4094)) {
return `not a valid VLAN ID '${tag}'`;
}
return false;
};
// [..]
if (res = invalidVid(start)) {
return res;
}
if (res = invalidVid(end)) {
return res;
}
`Number(res[n])` should return `NaN` if `res[n]` is undefined, this
shouldn't be the case for start ever but is relevant for `end`.
> + }
> +
> + if (start >= end) {
> + return `VID range must go from lower to higher tag:
> '${vid}'`;
> + }
> + }
> + return true;
> + },
> + });
> column2.push({
> xtype: 'proxmoxcheckbox',
> fieldLabel: gettext('VLAN aware'),
> name: 'bridge_vlan_aware',
> deleteEmpty: !me.isCreate,
> + listeners: {
> + change: function(f, newVal) {
> + if (me.bridge_set_vids) {
> + vids.setDisabled(!newVal);
> + }
> + },
> + },
> });
> column2.push({
> xtype: 'textfield',
> @@ -72,6 +131,9 @@ Ext.define('Proxmox.node.NetworkEdit', {
> 'data-qtip': gettext('Space-separated list of interfaces,
> for example: enp0s0 enp1s0'),
> },
> });
> + if (me.bridge_set_vids) {
> + advancedColumn2.push(vids);
> + }
> } else if (me.iftype === 'OVSBridge') {
> column2.push({
> xtype: 'textfield',
> diff --git a/src/node/NetworkView.js b/src/node/NetworkView.js
> index 1d67ac8..c08cbfa 100644
> --- a/src/node/NetworkView.js
> +++ b/src/node/NetworkView.js
> @@ -33,6 +33,9 @@ Ext.define('Proxmox.node.NetworkView', {
>
> showApplyBtn: false,
>
> +// decides if VLAN IDs field for bridges is shown, depends on the
> prod
Re: [pve-devel] [PATCH common v3 2/3] fix #3893: network: add vlan id and range parameter definitions
On Wed Jul 3, 2024 at 10:01 AM CEST, Aaron Lauterer wrote:
> This is one step to make it possible to define the VLAN IDs and ranges
> for bridges.
>
> It is expected to be used in combination with the `-list` magic
> property. Therefore it defines and checks the validity of a single list
> item that could just be a single VLAN tag ID or a range.
>
> Signed-off-by: Aaron Lauterer
> ---
> changes since v2:
> * renamed schema to a more generic one with the use case of the guest
> trunk config in mind
>
> src/PVE/JSONSchema.pm | 34 ++
> 1 file changed, 34 insertions(+)
>
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 115f811..22fe048 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -81,6 +81,12 @@ register_standard_option('pve-iface', {
> minLength => 2, maxLength => 20,
> });
>
> +register_standard_option('pve-vlan-id-or-range', {
> +description => "VLAN ID or range.",
> +type => 'string', format => 'pve-vlan-id-or-range',
> +minLength => 1, maxLength => 9,
> +});
> +
> register_standard_option('pve-storage-id', {
> description => "The storage identifier.",
> type => 'string', format => 'pve-storage-id',
> @@ -595,6 +601,34 @@ sub pve_verify_iface {
> return $id;
> }
>
> +# vlan id (vids), single or range
> +register_format('pve-vlan-id-or-range', \&pve_verify_vlan_id_or_range);
> +sub pve_verify_vlan_id_or_range {
> +my ($vlan, $noerr) = @_;
> +
> +my $check_vid = sub {
> + my $tag = shift;
> + if ( $tag < 2 || $tag > 4094) {
> + return undef if $noerr;
> + die "invalid VLAN tag '$tag'\n";
> + }
> +};
> +
> +if ($vlan !~ m/^(\d+)([-](\d+))?$/i) {
nit: pretty sure you can ommit the [] brackets here and you could use a
non-capturing group to get this somewhat cleaner regex:
^(\d+)(?:-(\d+))?$
also not sure this needs to be case insensitive, but it's probably fine.
by using a non capturing group you don't get the useless "-" in the
second group...
> + return undef if $noerr;
> + die "invalid VLAN configuration '$vlan'\n";
> +}
> +my $start = $1;
> +my $end = $3;
... so this becomes:
my $end = $2;
> +return undef if (!defined $check_vid->($start));
nit: there is an extra space here.
> +if ($end) {
> + return undef if (!defined $check_vid->($end));
> + die "VLAN range must go from lower to higher tag '$vlan'" if $start >=
> $end && !$noerr;
> +}
> +
> +return $vlan;
> +}
> +
> # general addresses by name or IP
> register_format('address', \&pve_verify_address);
> sub pve_verify_address {
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH widget-toolkit v3 7/6 follow-up] Network: add explanation for bridge vids field
On Tue Jul 23, 2024 at 3:33 PM CEST, Aaron Lauterer wrote:
> Make clear that it affects only out-/inbound traffic and can be used if
> the underlying physical NICs support only a limited number of VLANs when
> offloading is possible.
>
> Signed-off-by: Aaron Lauterer
> ---
> After some off-list discussion with @Stefan Hanreich after his review,
> we came to the conclusion that some explanation is needed to avoid
> confusion.
> I am not too happy with how long the explanation is, if someone has a
> better idea to transport that information, I am absolutely not opposed
> to it.
>
> src/node/NetworkEdit.js | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/src/node/NetworkEdit.js b/src/node/NetworkEdit.js
> index 8c1b135..1149dee 100644
> --- a/src/node/NetworkEdit.js
> +++ b/src/node/NetworkEdit.js
> @@ -109,6 +109,12 @@ Ext.define('Proxmox.node.NetworkEdit', {
> return true;
> },
> });
> + let vidsExplanation = Ext.create('Ext.form.field.Display', {
> + disabled: true,
> + userCls: 'pmx-hint',
> + value: 'Use if you want the bridge ports to only forward a
> limited number of ' +
> +'VLANs or the physical NICs support a limited number
> of offloaded VLANs.',
Maybe:
Limits the number of VLANs forwarded by the bridge ports. Useful if the
physical NICs only support a limited number of offloaded VLANs.
> + });
> column2.push({
> xtype: 'proxmoxcheckbox',
> fieldLabel: gettext('VLAN aware'),
> @@ -118,6 +124,7 @@ Ext.define('Proxmox.node.NetworkEdit', {
> change: function(f, newVal) {
> if (me.bridge_set_vids) {
> vids.setDisabled(!newVal);
> + vidsExplanation.setDisabled(!newVal);
> }
> },
> },
> @@ -133,6 +140,7 @@ Ext.define('Proxmox.node.NetworkEdit', {
> });
> if (me.bridge_set_vids) {
> advancedColumn2.push(vids);
> + advancedColumn2.push(vidsExplanation);
> }
> } else if (me.iftype === 'OVSBridge') {
> column2.push({
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager v3 5/6] fix #3893: api: network: add bridge_vids parameter
On Wed Jul 3, 2024 at 10:01 AM CEST, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer
> ---
> changes since v2:
> * added checks to handle empty lists
>
> PVE/API2/Network.pm | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/API2/Network.pm b/PVE/API2/Network.pm
> index f39f04f5..dd3855d1 100644
> --- a/PVE/API2/Network.pm
> +++ b/PVE/API2/Network.pm
> @@ -66,6 +66,11 @@ my $confdesc = {
> type => 'boolean',
> optional => 1,
> },
> +bridge_vids => {
> + description => "Specify the allowed vlans. For example: '2 4 100-200'.
> Only used if the bridge is VLAN aware.",
nit: i think this should be "VLANs" as you seem to capitalize this
abbreviation everywhere else.
> + optional => 1,
> + type => 'string', format => 'pve-vlan-id-or-range-list',
> +},
> bridge_ports => {
> description => "Specify the interfaces you want to add to your bridge.",
> optional => 1,
> @@ -469,6 +474,10 @@ __PACKAGE__->register_method({
> if ! grep { $_ eq $iface } @ports;
> }
>
> + if ($param->{bridge_vids} &&
> !PVE::Tools::check_list_empty($param->{bridge_vids})) {
> + raise_param_exc({ bridge_vids => "VLAN list items are empty" });
> + }
> +
> $ifaces->{$iface} = $param;
>
> PVE::INotify::write_file('interfaces', $config);
> @@ -558,7 +567,11 @@ __PACKAGE__->register_method({
> foreach my $k (keys %$param) {
> $ifaces->{$iface}->{$k} = $param->{$k};
> }
> -
> +
> + if ($param->{bridge_vids} &&
> !PVE::Tools::check_list_empty($param->{bridge_vids})) {
> + raise_param_exc({ bridge_vids => "VLAN list items are empty" });
> + }
> +
> PVE::INotify::write_file('interfaces', $config);
> };
>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method
Am 26.07.24 um 14:02 schrieb Max Carrara:
> On Fri Jul 26, 2024 at 11:52 AM CEST, Fiona Ebner wrote:
>> Am 25.07.24 um 17:32 schrieb Max Carrara:
>>> On Thu Jul 25, 2024 at 3:11 PM CEST, Fiona Ebner wrote:
>>
>> I don't quite get your point about not needing to update the call sites.
>> If you change the structure of the passed-in hash you still need to do that.
>
> Pardon me, I was a bit imprecise there. You're completely right that the
> passed hash has to be changed as well, of course.
>
> What I meant in particular was that because the signature (most likely)
> won't change, we won't have to take special care to update the
> individual arguments that are passed to a method when e.g. a parameter
> is deprecated (or removed) or added.
>
> For example, optional arguments are often just left out (because that's
> possible in Perl), so if we introduced another parameter ...
>
> * after the optional one, we'd have to pass something like `0` or
> `undef` for the optional one first before passing the new one:
>
> # before
> $plugin->foo($first, $second);
>
> # after
> $plugin->foo($first, $second, undef, $new);
>
> * before the optional one, we'd have to make sure the order of arguments
> is correct:
>
> # before
> $plugin->foo($first, $second, 1);
>
> # after
> $plugin->foo($first, $second, $new, 1);
>
> If we were to use a hash representing the parameters instead, the cases
> would look like this respectively:
>
> $plugin->foo({ first => $first, second => $second, new => $new });
>
> $plugin->foo({ first => $first, second => $second, optional => 1, new =
> $new });
>
> More examples of that pattern would be `PVE::Tools::run_command` and
> `PVE::RADOS::mon_command`.
>
> These changes can (and IMO should) still be guarded by an API age +
> version mechanism, it's just that the *surrounding maintenance work*
> becomes easier IMO, even if the initial cost for us and for implementors
> is a bit higher.
>
> Of course, this is all a suggestion and I really don't want to seem like
> I'm telling you what to do here! If you decide not to have a parameter
> hash etc. then that's also completely fine by me, of course. :)
>
Okay, I see what you mean. I agree that having a hash for optional
arguments is cleaner API-wise, but I do think fixed arguments (e.g. vmid
and drive ID for VM backup will always be present) should go directly
into the signature, not into a hash. I'll probably go with something
like what I wrote before about having mechanism-agnostic signatures, one
for VMs and one for containers.
>
> Also, thanks for going through this discussion here with me - even if
> you choose to not incorporate my suggestions, I'm glad we have these
> little exchanges, because they periodically update my perspective(s) on
> Perl code as well :P
>
No worries, your feedback is very welcome :)
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH pve-manager] sdn: vnets: Hide irrelevant fields depending on zone type
--- Begin Message ---
I’ve tested the changes thoroughly. The fields in the VnetEdit dialog
now correctly hide based on the selected zone type. For the EVPN zone
type, the VLAN Aware field is hidden, and for the simple zone type, the
tag field is correctly hidden. Everything is working perfectly.
Tested-by: Theodor Fumics
Reviewed-by: Theodor Fumics
On 12/22/23 11:43, Stefan Hanreich wrote:
Not all fields in the VnetEdit dialog are necessary for every zone
type. This lead to confusion for some users. Hide fields in the
VNetEdit dialog depending on which kind of zone is selected in order
to prevent potential confusion.
Signed-off-by: Stefan Hanreich
---
www/manager6/form/SDNZoneSelector.js | 2 +-
www/manager6/sdn/VnetEdit.js | 40
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/www/manager6/form/SDNZoneSelector.js
b/www/manager6/form/SDNZoneSelector.js
index 28c3457d2..0d0327529 100644
--- a/www/manager6/form/SDNZoneSelector.js
+++ b/www/manager6/form/SDNZoneSelector.js
@@ -40,7 +40,7 @@ Ext.define('PVE.form.SDNZoneSelector', {
}, function() {
Ext.define('pve-sdn-zone', {
extend: 'Ext.data.Model',
- fields: ['zone'],
+ fields: ['zone', 'type'],
proxy: {
type: 'proxmox',
url: "/api2/json/cluster/sdn/zones",
diff --git a/www/manager6/sdn/VnetEdit.js b/www/manager6/sdn/VnetEdit.js
index cdd83ed40..9fb6cd6c7 100644
--- a/www/manager6/sdn/VnetEdit.js
+++ b/www/manager6/sdn/VnetEdit.js
@@ -12,6 +12,13 @@ Ext.define('PVE.sdn.VnetInputPanel', {
return values;
},
+initComponent: function() {
+ let me = this;
+
+ me.callParent();
+ me.setZoneType(undefined);
+},
+
items: [
{
xtype: 'pmxDisplayEditField',
@@ -40,9 +47,21 @@ Ext.define('PVE.sdn.VnetInputPanel', {
name: 'zone',
value: '',
allowBlank: false,
+ listeners: {
+ change: function() {
+ let me = this;
+
+ let record = me.findRecordByValue(me.value);
+ let zoneType = record?.data?.type;
+
+ let panel = me.up('panel');
+ panel.setZoneType(zoneType);
+ },
+ },
},
{
xtype: 'proxmoxintegerfield',
+ itemId: 'sdnVnetTagField',
name: 'tag',
minValue: 1,
maxValue: 16777216,
@@ -54,6 +73,7 @@ Ext.define('PVE.sdn.VnetInputPanel', {
},
{
xtype: 'proxmoxcheckbox',
+ itemId: 'sdnVnetVlanAwareField',
name: 'vlanaware',
uncheckedValue: null,
checked: false,
@@ -63,6 +83,26 @@ Ext.define('PVE.sdn.VnetInputPanel', {
},
},
],
+
+setZoneType: function(zoneType) {
+ let me = this;
+
+ let tagField = me.down('#sdnVnetTagField');
+ if (!zoneType || zoneType === 'simple') {
+ tagField.setVisible(false);
+ tagField.setValue('');
+ } else {
+ tagField.setVisible(true);
+ }
+
+ let vlanField = me.down('#sdnVnetVlanAwareField');
+ if (!zoneType || zoneType === 'evpn') {
+ vlanField.setVisible(false);
+ vlanField.setValue('');
+ } else {
+ vlanField.setVisible(true);
+ }
+},
});
Ext.define('PVE.sdn.VnetEdit', {
--- End Message ---
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager] sdn: subnets: hide irrelevant fields depending on zone type
--- Begin Message ---
The SNAT option is only applicable to Simple and EVPN zone types.
For other zone types, this field is irrelevant and can cause confusion.
This commit hides the SNAT field when it is not applicable to the
selected zone type, improving clarity for users.
Signed-off-by: Theodor Fumics
---
www/manager6/sdn/SubnetEdit.js | 18 ++
www/manager6/sdn/SubnetView.js | 16
www/manager6/sdn/VnetView.js | 9 +
3 files changed, 43 insertions(+)
diff --git a/www/manager6/sdn/SubnetEdit.js b/www/manager6/sdn/SubnetEdit.js
index 8fc3f52b..d80eeca2 100644
--- a/www/manager6/sdn/SubnetEdit.js
+++ b/www/manager6/sdn/SubnetEdit.js
@@ -2,6 +2,8 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
extend: 'Proxmox.panel.InputPanel',
mixins: ['Proxmox.Mixin.CBind'],
+zoneInfo: undefined,
+
onGetValues: function(values) {
let me = this;
@@ -38,6 +40,7 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
},
{
xtype: 'proxmoxcheckbox',
+ itemId: 'snatSubnetCheckbox',
name: 'snat',
uncheckedValue: null,
checked: false,
@@ -57,6 +60,18 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
},
},
],
+
+initComponent: function() {
+ let me = this;
+ me.callParent();
+
+ let zoneType = me.zoneInfo?.type;
+
+ var showSNATCheckbox = ['simple', 'evpn'].includes(zoneType);
+
+ var snatCheckbox = me.down('#snatSubnetCheckbox');
+ snatCheckbox.setHidden(!showSNATCheckbox);
+},
});
Ext.define('PVE.sdn.SubnetDhcpRangePanel', {
@@ -243,6 +258,8 @@ Ext.define('PVE.sdn.SubnetEdit', {
base_url: undefined,
+zoneInfo: undefined,
+
bodyPadding: 0,
initComponent: function() {
@@ -261,6 +278,7 @@ Ext.define('PVE.sdn.SubnetEdit', {
let ipanel = Ext.create('PVE.sdn.SubnetInputPanel', {
isCreate: me.isCreate,
title: gettext('General'),
+ zoneInfo: me.zoneInfo,
});
let dhcpPanel = Ext.create('PVE.sdn.SubnetDhcpRangePanel', {
diff --git a/www/manager6/sdn/SubnetView.js b/www/manager6/sdn/SubnetView.js
index d342f0ba..fd103d78 100644
--- a/www/manager6/sdn/SubnetView.js
+++ b/www/manager6/sdn/SubnetView.js
@@ -7,6 +7,9 @@ Ext.define('PVE.sdn.SubnetView', {
base_url: undefined,
+zoneName: undefined,
+zoneInfo: undefined,
+
remove_btn: undefined,
setBaseUrl: function(url) {
@@ -28,6 +31,18 @@ Ext.define('PVE.sdn.SubnetView', {
}
},
+loadZone: function(name) {
+ let me = this;
+
+ Proxmox.Utils.API2Request({
+ url: `/cluster/sdn/zones/${name}?pending=1`,
+ method: 'GET',
+ success: function(response) {
+ me.zoneInfo = response?.result?.data;
+ },
+ });
+},
+
initComponent: function() {
let me = this;
@@ -59,6 +74,7 @@ Ext.define('PVE.sdn.SubnetView', {
let win = Ext.create('PVE.sdn.SubnetEdit', {
autoShow: true,
base_url: me.base_url,
+ zoneInfo: me.zoneInfo,
type: 'subnet',
});
win.on('destroy', reload);
diff --git a/www/manager6/sdn/VnetView.js b/www/manager6/sdn/VnetView.js
index 3fd3c916..9c9dcc5b 100644
--- a/www/manager6/sdn/VnetView.js
+++ b/www/manager6/sdn/VnetView.js
@@ -141,6 +141,15 @@ Ext.define('PVE.sdn.VnetView', {
select: function(_sm, rec) {
let url = `/cluster/sdn/vnets/${rec.data.vnet}/subnets`;
me.subnetview_panel.setBaseUrl(url);
+
+ let zoneName;
+ if (rec.data.pending) {
+ zoneName = rec.data.pending.zone;
+ } else {
+ zoneName = rec.data.zone;
+ }
+
+ me.subnetview_panel.loadZone(zoneName);
},
deselect: function() {
me.subnetview_panel.setBaseUrl(undefined);
--
2.39.2
--- End Message ---
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] sdn: subnets: hide irrelevant fields depending on zone type
On Fri Jul 26, 2024 at 3:27 PM CEST, Theodor Fumics wrote:
> The SNAT option is only applicable to Simple and EVPN zone types.
> For other zone types, this field is irrelevant and can cause confusion.
> This commit hides the SNAT field when it is not applicable to the
> selected zone type, improving clarity for users.
>
> Signed-off-by: Theodor Fumics
> ---
> www/manager6/sdn/SubnetEdit.js | 18 ++
> www/manager6/sdn/SubnetView.js | 16
> www/manager6/sdn/VnetView.js | 9 +
> 3 files changed, 43 insertions(+)
>
> diff --git a/www/manager6/sdn/SubnetEdit.js b/www/manager6/sdn/SubnetEdit.js
> index 8fc3f52b..d80eeca2 100644
> --- a/www/manager6/sdn/SubnetEdit.js
> +++ b/www/manager6/sdn/SubnetEdit.js
> @@ -2,6 +2,8 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
> extend: 'Proxmox.panel.InputPanel',
> mixins: ['Proxmox.Mixin.CBind'],
>
> +zoneInfo: undefined,
> +
> onGetValues: function(values) {
> let me = this;
>
> @@ -38,6 +40,7 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
> },
> {
> xtype: 'proxmoxcheckbox',
> + itemId: 'snatSubnetCheckbox',
> name: 'snat',
> uncheckedValue: null,
> checked: false,
> @@ -57,6 +60,18 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
> },
> },
> ],
> +
> +initComponent: function() {
> + let me = this;
> + me.callParent();
> +
> + let zoneType = me.zoneInfo?.type;
> +
> + var showSNATCheckbox = ['simple', 'evpn'].includes(zoneType);
> +
> + var snatCheckbox = me.down('#snatSubnetCheckbox');
> + snatCheckbox.setHidden(!showSNATCheckbox);
hey thanks for your contribution, any reason you are using `var` instead
of `let` here? our style guide says all new code should use `let` or
`const` and i can't determine a reason why you'd need a `var` here.
generally the behaviour of a `var` is also less predictable in regards
to hoisting etc.
> +},
> });
>
> Ext.define('PVE.sdn.SubnetDhcpRangePanel', {
> @@ -243,6 +258,8 @@ Ext.define('PVE.sdn.SubnetEdit', {
>
> base_url: undefined,
>
> +zoneInfo: undefined,
> +
> bodyPadding: 0,
>
> initComponent: function() {
> @@ -261,6 +278,7 @@ Ext.define('PVE.sdn.SubnetEdit', {
> let ipanel = Ext.create('PVE.sdn.SubnetInputPanel', {
> isCreate: me.isCreate,
> title: gettext('General'),
> + zoneInfo: me.zoneInfo,
> });
>
> let dhcpPanel = Ext.create('PVE.sdn.SubnetDhcpRangePanel', {
> diff --git a/www/manager6/sdn/SubnetView.js b/www/manager6/sdn/SubnetView.js
> index d342f0ba..fd103d78 100644
> --- a/www/manager6/sdn/SubnetView.js
> +++ b/www/manager6/sdn/SubnetView.js
> @@ -7,6 +7,9 @@ Ext.define('PVE.sdn.SubnetView', {
>
> base_url: undefined,
>
> +zoneName: undefined,
> +zoneInfo: undefined,
> +
> remove_btn: undefined,
>
> setBaseUrl: function(url) {
> @@ -28,6 +31,18 @@ Ext.define('PVE.sdn.SubnetView', {
> }
> },
>
> +loadZone: function(name) {
> + let me = this;
> +
> + Proxmox.Utils.API2Request({
> + url: `/cluster/sdn/zones/${name}?pending=1`,
> + method: 'GET',
> + success: function(response) {
> + me.zoneInfo = response?.result?.data;
> + },
> + });
> +},
> +
> initComponent: function() {
> let me = this;
>
> @@ -59,6 +74,7 @@ Ext.define('PVE.sdn.SubnetView', {
> let win = Ext.create('PVE.sdn.SubnetEdit', {
> autoShow: true,
> base_url: me.base_url,
> + zoneInfo: me.zoneInfo,
> type: 'subnet',
> });
> win.on('destroy', reload);
> diff --git a/www/manager6/sdn/VnetView.js b/www/manager6/sdn/VnetView.js
> index 3fd3c916..9c9dcc5b 100644
> --- a/www/manager6/sdn/VnetView.js
> +++ b/www/manager6/sdn/VnetView.js
> @@ -141,6 +141,15 @@ Ext.define('PVE.sdn.VnetView', {
> select: function(_sm, rec) {
> let url = `/cluster/sdn/vnets/${rec.data.vnet}/subnets`;
> me.subnetview_panel.setBaseUrl(url);
> +
> + let zoneName;
> + if (rec.data.pending) {
> + zoneName = rec.data.pending.zone;
> + } else {
> + zoneName = rec.data.zone;
> + }
> +
> + me.subnetview_panel.loadZone(zoneName);
> },
> deselect: function() {
> me.subnetview_panel.setBaseUrl(undefined);
> --
> 2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager v2] sdn: subnets: hide irrelevant fields depending on zone type
--- Begin Message ---
The SNAT option is only applicable to Simple and EVPN zone types.
For other zone types, this field is irrelevant and can cause confusion.
This commit hides the SNAT field when it is not applicable to the
selected zone type, improving clarity for users.
Signed-off-by: Theodor Fumics
---
Notes:
Changes from v1 -> v2
* switched var to let in SubnetEdit.js (thanks @Shannon)
www/manager6/sdn/SubnetEdit.js | 18 ++
www/manager6/sdn/SubnetView.js | 16
www/manager6/sdn/VnetView.js | 9 +
3 files changed, 43 insertions(+)
diff --git a/www/manager6/sdn/SubnetEdit.js b/www/manager6/sdn/SubnetEdit.js
index 8fc3f52b..7e4ec458 100644
--- a/www/manager6/sdn/SubnetEdit.js
+++ b/www/manager6/sdn/SubnetEdit.js
@@ -2,6 +2,8 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
extend: 'Proxmox.panel.InputPanel',
mixins: ['Proxmox.Mixin.CBind'],
+zoneInfo: undefined,
+
onGetValues: function(values) {
let me = this;
@@ -38,6 +40,7 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
},
{
xtype: 'proxmoxcheckbox',
+ itemId: 'snatSubnetCheckbox',
name: 'snat',
uncheckedValue: null,
checked: false,
@@ -57,6 +60,18 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
},
},
],
+
+initComponent: function() {
+ let me = this;
+ me.callParent();
+
+ let zoneType = me.zoneInfo?.type;
+
+ let showSNATCheckbox = ['simple', 'evpn'].includes(zoneType);
+
+ let snatCheckbox = me.down('#snatSubnetCheckbox');
+ snatCheckbox.setHidden(!showSNATCheckbox);
+},
});
Ext.define('PVE.sdn.SubnetDhcpRangePanel', {
@@ -243,6 +258,8 @@ Ext.define('PVE.sdn.SubnetEdit', {
base_url: undefined,
+zoneInfo: undefined,
+
bodyPadding: 0,
initComponent: function() {
@@ -261,6 +278,7 @@ Ext.define('PVE.sdn.SubnetEdit', {
let ipanel = Ext.create('PVE.sdn.SubnetInputPanel', {
isCreate: me.isCreate,
title: gettext('General'),
+ zoneInfo: me.zoneInfo,
});
let dhcpPanel = Ext.create('PVE.sdn.SubnetDhcpRangePanel', {
diff --git a/www/manager6/sdn/SubnetView.js b/www/manager6/sdn/SubnetView.js
index d342f0ba..fd103d78 100644
--- a/www/manager6/sdn/SubnetView.js
+++ b/www/manager6/sdn/SubnetView.js
@@ -7,6 +7,9 @@ Ext.define('PVE.sdn.SubnetView', {
base_url: undefined,
+zoneName: undefined,
+zoneInfo: undefined,
+
remove_btn: undefined,
setBaseUrl: function(url) {
@@ -28,6 +31,18 @@ Ext.define('PVE.sdn.SubnetView', {
}
},
+loadZone: function(name) {
+ let me = this;
+
+ Proxmox.Utils.API2Request({
+ url: `/cluster/sdn/zones/${name}?pending=1`,
+ method: 'GET',
+ success: function(response) {
+ me.zoneInfo = response?.result?.data;
+ },
+ });
+},
+
initComponent: function() {
let me = this;
@@ -59,6 +74,7 @@ Ext.define('PVE.sdn.SubnetView', {
let win = Ext.create('PVE.sdn.SubnetEdit', {
autoShow: true,
base_url: me.base_url,
+ zoneInfo: me.zoneInfo,
type: 'subnet',
});
win.on('destroy', reload);
diff --git a/www/manager6/sdn/VnetView.js b/www/manager6/sdn/VnetView.js
index 3fd3c916..9c9dcc5b 100644
--- a/www/manager6/sdn/VnetView.js
+++ b/www/manager6/sdn/VnetView.js
@@ -141,6 +141,15 @@ Ext.define('PVE.sdn.VnetView', {
select: function(_sm, rec) {
let url = `/cluster/sdn/vnets/${rec.data.vnet}/subnets`;
me.subnetview_panel.setBaseUrl(url);
+
+ let zoneName;
+ if (rec.data.pending) {
+ zoneName = rec.data.pending.zone;
+ } else {
+ zoneName = rec.data.zone;
+ }
+
+ me.subnetview_panel.loadZone(zoneName);
},
deselect: function() {
me.subnetview_panel.setBaseUrl(undefined);
--
2.39.2
--- End Message ---
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] sdn: subnets: hide irrelevant fields depending on zone type
--- Begin Message ---
Hi Shannon,
Thanks for catching that! I missed the use of var and have now sent a
revised version (v2) using let instead [1].
[1]
https://lore.proxmox.com/pve-devel/mailman.701.1722002701.331.pve-de...@lists.proxmox.com/T/#u
On 7/26/24 15:42, Shannon Sterz wrote:
On Fri Jul 26, 2024 at 3:27 PM CEST, Theodor Fumics wrote:
The SNAT option is only applicable to Simple and EVPN zone types.
For other zone types, this field is irrelevant and can cause confusion.
This commit hides the SNAT field when it is not applicable to the
selected zone type, improving clarity for users.
Signed-off-by: Theodor Fumics
---
www/manager6/sdn/SubnetEdit.js | 18 ++
www/manager6/sdn/SubnetView.js | 16
www/manager6/sdn/VnetView.js | 9 +
3 files changed, 43 insertions(+)
diff --git a/www/manager6/sdn/SubnetEdit.js b/www/manager6/sdn/SubnetEdit.js
index 8fc3f52b..d80eeca2 100644
--- a/www/manager6/sdn/SubnetEdit.js
+++ b/www/manager6/sdn/SubnetEdit.js
@@ -2,6 +2,8 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
extend: 'Proxmox.panel.InputPanel',
mixins: ['Proxmox.Mixin.CBind'],
+zoneInfo: undefined,
+
onGetValues: function(values) {
let me = this;
@@ -38,6 +40,7 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
},
{
xtype: 'proxmoxcheckbox',
+ itemId: 'snatSubnetCheckbox',
name: 'snat',
uncheckedValue: null,
checked: false,
@@ -57,6 +60,18 @@ Ext.define('PVE.sdn.SubnetInputPanel', {
},
},
],
+
+initComponent: function() {
+ let me = this;
+ me.callParent();
+
+ let zoneType = me.zoneInfo?.type;
+
+ var showSNATCheckbox = ['simple', 'evpn'].includes(zoneType);
+
+ var snatCheckbox = me.down('#snatSubnetCheckbox');
+ snatCheckbox.setHidden(!showSNATCheckbox);
hey thanks for your contribution, any reason you are using `var` instead
of `let` here? our style guide says all new code should use `let` or
`const` and i can't determine a reason why you'd need a `var` here.
generally the behaviour of a `var` is also less predictable in regards
to hoisting etc.
+},
});
Ext.define('PVE.sdn.SubnetDhcpRangePanel', {
@@ -243,6 +258,8 @@ Ext.define('PVE.sdn.SubnetEdit', {
base_url: undefined,
+zoneInfo: undefined,
+
bodyPadding: 0,
initComponent: function() {
@@ -261,6 +278,7 @@ Ext.define('PVE.sdn.SubnetEdit', {
let ipanel = Ext.create('PVE.sdn.SubnetInputPanel', {
isCreate: me.isCreate,
title: gettext('General'),
+ zoneInfo: me.zoneInfo,
});
let dhcpPanel = Ext.create('PVE.sdn.SubnetDhcpRangePanel', {
diff --git a/www/manager6/sdn/SubnetView.js b/www/manager6/sdn/SubnetView.js
index d342f0ba..fd103d78 100644
--- a/www/manager6/sdn/SubnetView.js
+++ b/www/manager6/sdn/SubnetView.js
@@ -7,6 +7,9 @@ Ext.define('PVE.sdn.SubnetView', {
base_url: undefined,
+zoneName: undefined,
+zoneInfo: undefined,
+
remove_btn: undefined,
setBaseUrl: function(url) {
@@ -28,6 +31,18 @@ Ext.define('PVE.sdn.SubnetView', {
}
},
+loadZone: function(name) {
+ let me = this;
+
+ Proxmox.Utils.API2Request({
+ url: `/cluster/sdn/zones/${name}?pending=1`,
+ method: 'GET',
+ success: function(response) {
+ me.zoneInfo = response?.result?.data;
+ },
+ });
+},
+
initComponent: function() {
let me = this;
@@ -59,6 +74,7 @@ Ext.define('PVE.sdn.SubnetView', {
let win = Ext.create('PVE.sdn.SubnetEdit', {
autoShow: true,
base_url: me.base_url,
+ zoneInfo: me.zoneInfo,
type: 'subnet',
});
win.on('destroy', reload);
diff --git a/www/manager6/sdn/VnetView.js b/www/manager6/sdn/VnetView.js
index 3fd3c916..9c9dcc5b 100644
--- a/www/manager6/sdn/VnetView.js
+++ b/www/manager6/sdn/VnetView.js
@@ -141,6 +141,15 @@ Ext.define('PVE.sdn.VnetView', {
select: function(_sm, rec) {
let url = `/cluster/sdn/vnets/${rec.data.vnet}/subnets`;
me.subnetview_panel.setBaseUrl(url);
+
+ let zoneName;
+ if (rec.data.pending) {
+ zoneName = rec.data.pending.zone;
+ } else {
+ zoneName = rec.data.zone;
+ }
+
+ me.subnetview_panel.loadZone(zoneName);
},
deselect: function() {
me.subnetview_panel.setBaseUrl(undefined);
--
2.39.2
--- End Message ---
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
