Re: [pdm-devel] [PATCH pve-network v3 6/6] api: zones: update schema of endpoints

[Stefan Hanreich <s.hanreich@proxmox.com>]

thanks I'll make the changes to the descriptions, some comments
regarding vxlan-port


On 9/3/25 11:36 AM, Shannon Sterz wrote:
> small comments in-line
> 
> On Tue Sep 2, 2025 at 4:09 PM CEST, Stefan Hanreich wrote:
>> The possible properties returned by the zone endpoints were only
>> partly documented. Add all missing properties and improve descriptions
>> for existing properties.
>>
>> Extract all duplicate properties into a separate variable, so we
>> don't have to rewrite the whole API definition for every endpoint.
>>
>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>> ---
>>  src/PVE/API2/Network/SDN/Zones.pm        | 203 +++++++++++++++++++++--
>>  src/PVE/Network/SDN/Zones/EvpnPlugin.pm  |  22 ++-
>>  src/PVE/Network/SDN/Zones/QinQPlugin.pm  |   6 +-
>>  src/PVE/Network/SDN/Zones/VlanPlugin.pm  |   1 +
>>  src/PVE/Network/SDN/Zones/VxlanPlugin.pm |  15 +-
>>  5 files changed, 218 insertions(+), 29 deletions(-)
>>
> 
> -->8 snip 8<--
> 
>> +    'vxlan-port' => {
>> +        description =>
>> +            "UDP port that should be used for the VXLAN tunnel (default 4789). VXLAN zone only.",
> 
> wouldn't it make sense to define a `default` property here then? so:
> 
>     default => 4789,

bit of a weird one, since the default is 4789 for VXLAN zones only,
otherwise it is null. Also, the default for the section config is
technically null as well, since we rely on the fact that not specifying
a port defaults to 4789 in ifupdown2 (fun fact: in the kernel its 8472).

anyway, from a practical standpoint this makes sense imo and its
informational only, so no harm in adding it here.

> 
> -->8 snip 8<--
> 
>>          'rt-import' => {
>>              type => 'string',
>> -            description => "Route-Target import",
>> +            description =>
>> +                'List of Route Targets that should be imported into the VRF of the zone',
> 
> nit: you added a period to all other descriptions here, so end this
> sentence with one too?
> 
>>              optional => 1,
>>              format => 'pve-sdn-bgp-rt-list',
>>          },
>> diff --git a/src/PVE/Network/SDN/Zones/QinQPlugin.pm b/src/PVE/Network/SDN/Zones/QinQPlugin.pm
>> index 5806e69..3c72d35 100644
>> --- a/src/PVE/Network/SDN/Zones/QinQPlugin.pm
>> +++ b/src/PVE/Network/SDN/Zones/QinQPlugin.pm
>> @@ -18,11 +18,11 @@ sub properties {
>>          tag => {
>>              type => 'integer',
>>              minimum => 0,
>> -            description => "Service-VLAN Tag",
>> +            description => "Service-VLAN Tag (outer VLAN)",
>>          },
> 
> since this is so short, i think it's probably fine to leave that without
> a period here.
> 
>>          mtu => {
>>              type => 'integer',
>> -            description => "MTU",
>> +            description => "MTU of the zone, will be used for the created VNet bridges.",
>>              optional => 1,
>>          },
>>          'vlan-protocol' => {
>> @@ -30,6 +30,8 @@ sub properties {
>>              enum => ['802.1q', '802.1ad'],
>>              default => '802.1q',
>>              optional => 1,
>> +            description =>
>> +                "Which VLAN protocol should be used for the creation of the QinQ zone",
> 
> nit: add a period here too?
> 
>>          },
>>      };
>>  }
>> diff --git a/src/PVE/Network/SDN/Zones/VlanPlugin.pm b/src/PVE/Network/SDN/Zones/VlanPlugin.pm
>> index 90f16bf..9d6932f 100644
>> --- a/src/PVE/Network/SDN/Zones/VlanPlugin.pm
>> +++ b/src/PVE/Network/SDN/Zones/VlanPlugin.pm
>> @@ -27,6 +27,7 @@ sub properties {
>>      return {
>>          'bridge' => {
>>              type => 'string',
>> +            description => 'the bridge for which VLANs should be managed',
> 
> nit: capitalize beginning of sentence and end it in a period?
> 
>>          },
>>          'bridge-disable-mac-learning' => {
>>              type => 'boolean',
>> diff --git a/src/PVE/Network/SDN/Zones/VxlanPlugin.pm b/src/PVE/Network/SDN/Zones/VxlanPlugin.pm
>> index 8f6fba0..7ab89da 100644
>> --- a/src/PVE/Network/SDN/Zones/VxlanPlugin.pm
>> +++ b/src/PVE/Network/SDN/Zones/VxlanPlugin.pm
>> @@ -27,21 +27,22 @@ sub type {
>>  sub properties {
>>      return {
>>          'peers' => {
>> -            description => "peers address list.",
>> +            description =>
>> +                "Comma-separated list of peers, that are part of the VXLAN zone. Usually the IPs of the nodes.",
>>              type => 'string',
>>              format => 'ip-list',
>>          },
>> -        fabric => {
>> -            description => "SDN fabric to use as underlay for this VXLAN zone.",
>> -            type => 'string',
>> -            format => 'pve-sdn-fabric-id',
>> -        },
>>          'vxlan-port' => {
>> -            description => "Vxlan tunnel udp port (default 4789).",
>> +            description => "UDP port that should be used for the VXLAN tunnel (default 4789).",
>>              minimum => 1,
>>              maximum => 65536,
> 
> could also define the default here too?

here it definitely would make sense, since it's non-optional.

> 
>>              type => 'integer',
>>          },
>> +        fabric => {
>> +            description => "SDN fabric to use as underlay for this VXLAN zone.",
>> +            type => 'string',
>> +            format => 'pve-sdn-fabric-id',
>> +        },
>>      };
>>  }
>>
> 



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