From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 787C91FF17C for ; Wed, 3 Sep 2025 15:04:55 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4E056D0CE; Wed, 3 Sep 2025 15:05:10 +0200 (CEST) Message-ID: <7d10f2fd-0dc7-42b4-ba0b-90cdbde2535e@proxmox.com> Date: Wed, 3 Sep 2025 15:04:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Shannon Sterz , Proxmox Datacenter Manager development discussion References: <20250902140956.228031-1-s.hanreich@proxmox.com> <20250902140956.228031-10-s.hanreich@proxmox.com> Content-Language: en-US From: Stefan Hanreich In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.706 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_MSPIKE_H2 0.001 Average reputation (+2) RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [vxlanplugin.pm, evpnplugin.pm, vlanplugin.pm, zones.pm, qinqplugin.pm] Subject: Re: [pdm-devel] [PATCH pve-network v3 6/6] api: zones: update schema of endpoints X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" 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 >> --- >> 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