From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id C55A11FF138 for ; Mon, 20 Jul 2026 10:20:33 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D35B32144D; Mon, 20 Jul 2026 10:20:32 +0200 (CEST) Message-ID: <910c72bb-21d1-4d2a-b594-7838bcb51665@proxmox.com> Date: Mon, 20 Jul 2026 10:20:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: David Riley Subject: Re: [PATCH pve-manager v2 02/10] fix #7294: api: pool: add SDN VNets as pool members To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260626131035.112374-1-d.riley@proxmox.com> <20260626131035.112374-3-d.riley@proxmox.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784535600645 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.133 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: NXP2PSRRUUNW4KTLKCC5T2ZIUARKXIHT X-Message-ID-Hash: NXP2PSRRUUNW4KTLKCC5T2ZIUARKXIHT X-MailFrom: d.riley@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Thanks for the feedback. I'll address the nits you mentioned in v3. Some comments inline. On 7/3/26 3:19 PM, Daniel Kral wrote: > On Fri Jun 26, 2026 at 3:10 PM CEST, David Riley wrote: >> Extend the pool API to accept SDN VNets and optional VLAN tags. Group >> VNets under the new 'network' property type in the pool configuration. >> >> Unlike VMs or containers which strictly belong to a single pool, VNets >> are shared similar to storage. A single VNet can be assigned to >> multiple pools simultaneously, allowing cross-team usage without >> management conflicts. >> >> Enforce a cluster-wide version check before allowing network >> assignments. This prevents older nodes from accidentally overwriting >> the newly structured pool configurations. >> >> Link:https://bugzilla.proxmox.com/show_bug.cgi?id=7294 >> Signed-off-by: David Riley >> --- > This patch relies on some new features from other patches, e.g., from > pve-cluster: PVE::Cluster::assert_min_cluster_version() and > pve-access-control: the new $pool_cfg->{network} property. > > We encode such constraints in version dependency, so it's a good measure > to add a comment here for the applying maintainer that these > dependencies' versions should be increased. Alright. I'll add it to the git notes in the updated series. > Additionally, the multi-repo patch series should be ordered as someone > should build & install the repositories/packages. That is, also ordered > by dependency, e.g., the changes to pve-cluster and pve-access-control, > which are used here should come before these patches. I agree that would make the patches easier to review. Will update the ordering in the next version. >> PVE/API2/Pool.pm | 135 ++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 129 insertions(+), 6 deletions(-) >> >> diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm >> index 63aff5bb..6bd99abb 100644 >> --- a/PVE/API2/Pool.pm >> +++ b/PVE/API2/Pool.pm >> @@ -4,10 +4,12 @@ use strict; >> use warnings; >> >> use PVE::AccessControl; >> -use PVE::Cluster qw (cfs_read_file cfs_write_file); >> +use PVE::Cluster qw (cfs_read_file cfs_write_file assert_min_cluster_version); >> use PVE::Exception qw(raise_param_exc); >> use PVE::INotify; >> +use PVE::Network; >> use PVE::Storage; >> +use PVE::Tools; >> >> use PVE::SafeSyslog; >> >> @@ -16,6 +18,26 @@ use PVE::RESTHandler; >> >> use base qw(PVE::RESTHandler); >> >> +my $pool_network_format = { >> + zone => { >> + description => 'SDN Zone', >> + type => 'string', >> + format => 'pve-sdn-zone-id', >> + }, >> + vnet => { >> + description => 'VNet to add or remove from this pool.', >> + type => 'string', >> + format => 'pve-sdn-vnet-id', >> + }, >> + tag => { >> + description => "Specify a VLAN tag", >> + optional => 1, >> + type => 'integer', >> + minimum => 1, >> + maximum => 4094, >> + }, >> +}; >> + >> __PACKAGE__->register_method({ >> name => 'index', >> path => '', >> @@ -36,7 +58,7 @@ __PACKAGE__->register_method({ >> }, >> type => { >> type => 'string', >> - enum => ['qemu', 'lxc', 'storage'], >> + enum => ['qemu', 'lxc', 'storage', 'network'], >> optional => 1, >> requires => 'poolid', >> }, >> @@ -61,7 +83,7 @@ __PACKAGE__->register_method({ >> properties => { >> type => { >> type => 'string', >> - enum => ['qemu', 'lxc', 'openvz', 'storage'], >> + enum => ['qemu', 'lxc', 'openvz', 'storage', 'network'], >> }, >> id => { >> type => 'string', >> @@ -135,6 +157,29 @@ __PACKAGE__->register_method({ >> } >> } >> >> + if (!defined($param->{type}) || $param->{type} eq 'network') { >> + if ($pool_config->{network}) { > nit: the additional check can be dropped with the properly versioned > dependency to pve-access-control, which should always at least contain > an empty hash ref for $pool_config->{network}. I'll drop it. >> + for my $net_key (sort keys $pool_config->{network}->%*) { >> + my ($type, @path) = split('/', $net_key); >> + >> + if ($type eq 'vnet') { >> + my ($zoneid, $vnet, $tag) = @path; >> + >> + my $description = "$vnet ($zoneid)"; >> + $description = "$vnet.$tag ($zoneid)" if defined($tag); >> + >> + push @$members, >> + { >> + type => 'network', >> + 'network-type' => $type, >> + id => $net_key, >> + text => $description, >> + }; >> + } >> + } >> + } >> + } >> + >> my $pool_info = { >> members => $members, >> }; >> @@ -243,6 +288,13 @@ __PACKAGE__->register_method({ >> format => 'pve-storage-id-list', >> optional => 1, >> }, >> + network => { >> + description => 'Network resource to add or remove from this pool.', >> + type => 'string', >> + typetext => 'zone=,vnet=[,tag=]', >> + format => $pool_network_format, >> + optional => 1, >> + }, > As discussed off-list, this would prevent the property from being > extended with lists, at least if we want to go with a comma-separated > list, which is pretty much the default we use. > > Maybe there's some other representation for this, like > 'vnet//[/]' or something similar? Then each entry > (or only one entry for now) would be the $network_key. > > Essentially, we could also allow lists of network resources already with > > # outside so we only read the configs once > my $zones_cfg = PVE::Network::SDN::Zones::config(); > my $vnets_cfg = PVE::Network::SDN::Vnets::config(); > > for my $network_key (PVE::Tools::split_list($params->{network})) { > # ... > } > > then if I didn't miss anything? You don't have to add adding multiple > VNets at once in the web interface though, that can be a separate > feature request. > > What do you think? You're right, the current property string format makes it difficult to support comma-separated lists in the future. I was focusing too much on leveraging the schema for rigorous API parameter validation, but I overlooked the parsing conflict this creates for lists. Your suggested format (/[/]) is definitely the cleanest way to handle multiple network resources while avoiding mapping issues. I'll update the patch to use this approach and implement the necessary custom validation for it. I'll make lists available for the CLI right away, but I'll skip it in the WebUI for now. >> 'allow-move' => { >> description => 'Allow adding a guest even if already in another pool.' >> . ' The guest will be removed from its current pool and added to this one.', >> @@ -295,6 +347,13 @@ __PACKAGE__->register_method({ >> format => 'pve-storage-id-list', >> optional => 1, >> }, >> + network => { >> + description => 'Network resource to add or remove from this pool.', >> + type => 'string', >> + typetext => 'zone=,vnet=[,tag=]', >> + format => $pool_network_format, >> + optional => 1, >> + }, > Same here > >> 'allow-move' => { >> description => 'Allow adding a guest even if already in another pool.' >> . ' The guest will be removed from its current pool and added to this one.', >> @@ -304,7 +363,7 @@ __PACKAGE__->register_method({ >> }, >> delete => { >> description => >> - 'Remove the passed VMIDs and/or storage IDs instead of adding them.', >> + 'Remove the passed VMIDs, storage IDs and/or network resource instead of adding them.', >> type => 'boolean', >> optional => 1, >> default => 0, >> @@ -373,6 +432,62 @@ __PACKAGE__->register_method({ >> } >> } >> >> + if (defined($param->{network})) { >> + my $vnet_entry = PVE::JSONSchema::parse_property_string( >> + $pool_network_format, $param->{network}, >> + ); >> + >> + # gatekeep vnet as pool members >> + PVE::Cluster::assert_min_cluster_version(9, 2, 3); > Sorry, I missed this in the previous review: This patch should > definitely have some note that the maintainer applying this must update > this version number appropriately with the introduction of this feature. > > Either the applying maintainer will change the patch itself or make a > follow-up commit that changes the version number afterwards. I'll make it more visible that this patch requires a version bump. >> + >> + my ($zone, $vnetid, $tag) = $vnet_entry->@{qw(zone vnet tag)}; >> + >> + my $zones_cfg = PVE::Network::SDN::Zones::config(); >> + die "SDN Zone '$zone' does not exist\n" if !$zones_cfg->{ids}->{$zone}; >> + >> + my $vnets_cfg = PVE::Network::SDN::Vnets::config(); >> + >> + my $vnet_data = $vnets_cfg->{ids}->{$vnetid} >> + or die "VNet '$vnetid' does not exist\n"; >> + >> + my $vnet_zone = $vnet_data->{zone}; >> + if ($zone ne $vnet_zone) { >> + die >> + "VNet '$vnetid' does not belong to zone '$zone' (it belongs to '$vnet_zone')\n"; >> + } > nit: use the post-if version here (might allow that the die expr doesn't > need to be on a new line) Sadly it does not fit. I was considering this but perltidy would stretch it over 3 lines with a post-if. >> + >> + my $network_key = "vnet/$vnet_zone/$vnetid"; >> + >> + if (defined($tag)) { >> + if (!$vnet_data->{vlanaware}) { >> + die >> + "VNet '$vnetid' is not VLAN-aware, cannot assign a specific tag\n"; >> + } > nit: use the post-if version here (might allow that the die expr doesn't > need to be on a new line) > >> + >> + $network_key .= "/$tag"; >> + } >> + >> + $rpcenv->check_perm_modify( >> + $authuser, >> + "/sdn/zones/$vnet_zone/$vnetid", >> + ['SDN.Allocate'], >> + ); >> + >> + if ($param->{delete}) { >> + if (!$pool_config->{network}->{$network_key}) { >> + die "Network resource '$network_key' is not a pool member\n"; >> + } > nit: use the post-if version here > >> + >> + delete $pool_config->{network}->{$network_key}; >> + } else { >> + if ($pool_config->{network}->{$network_key}) { >> + die "Network resource '$network_key' is already a pool member\n"; >> + } > nit: use the post-if version here > >> + >> + $pool_config->{network}->{$network_key} = 1; >> + } >> + } >> + >> cfs_write_file("user.cfg", $usercfg); >> }, >> "update pools failed", >> @@ -400,7 +515,7 @@ __PACKAGE__->register_method({ >> }, >> type => { >> type => 'string', >> - enum => ['qemu', 'lxc', 'storage'], >> + enum => ['qemu', 'lxc', 'storage', 'network'], >> optional => 1, >> }, >> }, >> @@ -421,7 +536,7 @@ __PACKAGE__->register_method({ >> properties => { >> type => { >> type => 'string', >> - enum => ['qemu', 'lxc', 'openvz', 'storage'], >> + enum => ['qemu', 'lxc', 'openvz', 'storage', 'network'], >> }, >> id => { >> type => 'string', >> @@ -524,6 +639,14 @@ __PACKAGE__->register_method({ >> die "pool '$pool' is not empty (contains storage '$storeid')\n"; >> } >> >> + for my $netid (sort keys $pool_config->{network}->%*) { >> + my ($type, $id) = split('/', $netid, 2); >> + $type = 'network' if !defined($type); >> + $id = $netid if !defined($id); >> + >> + die "pool '$pool' is not empty (contains $type '$id')\n"; >> + } >> + >> delete($usercfg->{pools}->{$pool}); >> PVE::AccessControl::delete_pool_acl($pool, $usercfg); >>