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 AFCA31FF153 for ; Mon, 22 Jun 2026 15:37:43 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 93CE0CD1F; Mon, 22 Jun 2026 15:37:41 +0200 (CEST) Message-ID: Date: Mon, 22 Jun 2026 15:37:35 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-manager 2/9] fix #7294: api: pool: add SDN VNets as pool members To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260611145935.147788-1-d.riley@proxmox.com> <20260611145935.147788-3-d.riley@proxmox.com> Content-Language: en-US From: David Riley 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: 1782135446488 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.153 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 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: YNVRBOQV2N64FXLUSP3J2I5TKHMPQM4X X-Message-ID-Hash: YNVRBOQV2N64FXLUSP3J2I5TKHMPQM4X 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: On 6/22/26 1:20 PM, Daniel Kral wrote: > Some comments inline > > On Thu Jun 11, 2026 at 4:59 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 >> --- >> PVE/API2/Pool.pm | 137 ++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 135 insertions(+), 2 deletions(-) >> >> diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm >> index 63aff5bb..f1eb6eb3 100644 >> --- a/PVE/API2/Pool.pm >> +++ b/PVE/API2/Pool.pm >> @@ -5,8 +5,10 @@ use warnings; >> >> use PVE::AccessControl; >> use PVE::Cluster qw (cfs_read_file cfs_write_file); >> +use PVE::Cluster::Helpers qw(assert_min_cluster_version); >> use PVE::Exception qw(raise_param_exc); >> use PVE::INotify; >> +use PVE::Network; >> use PVE::Storage; >> >> use PVE::SafeSyslog; >> @@ -61,7 +63,7 @@ __PACKAGE__->register_method({ >> properties => { >> type => { >> type => 'string', >> - enum => ['qemu', 'lxc', 'openvz', 'storage'], >> + enum => ['qemu', 'lxc', 'openvz', 'storage', 'network'], >> }, >> id => { >> type => 'string', >> @@ -77,6 +79,10 @@ __PACKAGE__->register_method({ >> type => 'string', >> optional => 1, >> }, >> + vnet => { >> + type => 'string', >> + optional => 1, >> + }, >> }, >> }, >> }, >> @@ -135,6 +141,29 @@ __PACKAGE__->register_method({ >> } >> } >> >> + if (!defined($param->{type}) || $param->{type} eq 'network') { >> + if ($pool_config->{network}) { >> + for my $net_key (sort keys %{ $pool_config->{network} }) { > nit: `$pool_config->{network}->%*` might be more readable Fair. > >> + my ($type, @path) = split('/', $net_key); >> + >> + if ($type eq 'vnet') { >> + my ($zoneid, $vnet, $vlan) = @path; >> + >> + my $description = "$vnet ($zoneid)"; >> + $description = "$vnet.$vlan ($zoneid)" if defined($vlan); >> + >> + push @$members, >> + { >> + type => 'network', >> + id => $net_key, >> + text => $description, >> + 'network-type' => $type, >> + }; >> + } >> + } >> + } >> + } >> + >> my $pool_info = { >> members => $members, >> }; >> @@ -243,6 +272,25 @@ __PACKAGE__->register_method({ >> format => 'pve-storage-id-list', >> optional => 1, >> }, >> + zone => { >> + description => 'SDN Zone', >> + type => 'string', >> + format => 'pve-sdn-zone-id', >> + optional => 1, >> + }, >> + vnet => { >> + description => 'VNet to add or remove from this pool.', >> + type => 'string', >> + format => 'pve-sdn-vnet-id', >> + optional => 1, >> + }, > The other parameters like 'vms' and 'storage' allow specifying a list of > them to be added or removed at a single time, would that also make sense > with VNets? I actually initially allowed for a list of VNets, but it increased the complexity significantly, so I opted to keep it simple for now. > and I'm not sure if there is a high demand for adding multiple VNets in one go. > Either way, is there some way to make zone, vnetid and tag a single > property so it's clear that zone and vnet are required to add a VNet and > tag is optional? > I can definitely update this if there is a strong demand for it. I'll think about combining them or at least giving a clear error message if only the VNet is supplied. >> + tag => { >> + description => "Specify a VLAN tag", >> + optional => 1, >> + type => 'integer', >> + minimum => 1, >> + maximum => 4094, >> + }, >> '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 +343,25 @@ __PACKAGE__->register_method({ >> format => 'pve-storage-id-list', >> optional => 1, >> }, >> + zone => { >> + description => 'SDN Zone', >> + type => 'string', >> + format => 'pve-sdn-zone-id', >> + optional => 1, >> + }, >> + vnet => { >> + description => 'VNet to add or remove from this pool.', >> + type => 'string', >> + format => 'pve-sdn-vnet-id', >> + optional => 1, >> + }, >> + tag => { >> + description => "Specify a VLAN tag", >> + optional => 1, >> + type => 'integer', >> + minimum => 1, >> + maximum => 4094, >> + }, >> '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 +371,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 VNets instead of adding them.', >> type => 'boolean', >> optional => 1, >> default => 0, >> @@ -373,6 +440,56 @@ __PACKAGE__->register_method({ >> } >> } >> >> + if (defined($param->{vnet}) && defined($param->{zone})) { > AFAICT there's no error that e.g. zone is missing and it will do nothing > even though a user might have expected the VNet to be added without > specifying the zone. You are right currently it won't give any feedback to the user. Will adapt it in v2 to provide clear feedback to the user. > > > nit: might be nice to destructure them through a tuple: > > my ($zone, $vnetid, $tag) = $param->@{qw(zone vnet tag)}; I agree. >> + # gatekeep vnet as pool members >> + assert_min_cluster_version(9, 2, 3); > nit: might be nicer to prefix it with the full module path here so that > it's clear that it's not a local subroutine (is usually fine for > more well-known helpers), but no hard feelings. Alright, I will adapt it to make it clear that this is not a local subroutine. >> + >> + my $zones_cfg = PVE::Network::SDN::Zones::config(); >> + my $zone = $param->{zone}; >> + >> + if (!$zones_cfg->{ids}->{$zone}) { >> + die "SDN Zone '$zone' does not exist\n"; >> + } >> + >> + my $vnets_cfg = PVE::Network::SDN::Vnets::config(); >> + my $tag = $param->{tag}; >> + >> + my $vnetid = $param->{vnet}; >> + 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: might be nice to make this a single > > die "VNet ...\n" if $zone ne $vnet_data->{zone}; > > to not have two variables with essentially the same content after this? > I'm using the $vnet_zone further down as well that's why I pulled it out. >> + >> + my $has_tag = defined($tag) && $tag ne ''; > When is $tag eq '' true? Shouldn't the API verification already catch > that $tag is a string even though it should be an integer? You are right. Not sure what i was thinking here. >> + if ($has_tag) { >> + my $native_tag = $vnet_data->{tag}; >> + if (!defined($native_tag) || $tag != $native_tag) { >> + die >> + "VNet '$vnetid' is not VLAN-aware, cannot assign a specific tag\n" >> + if !$vnet_data->{vlanaware}; >> + } >> + } >> + >> + my $network_key = "vnet/$vnet_zone/$vnetid"; >> + $network_key .= "/$tag" if $has_tag; > nit: not sure about this, but could also be put in the previous if > block, but I see the point that this should probably be tightly put > together so it's easy to see the possible contents of $network_key Yes, my intention was to make it clear how the final network_key will look like. I will move it into the if in v2, my initial thought was that the if might error out, but it makes more sense to prioritize the happy path. >> + >> + $rpcenv->check_perm_modify( >> + $authuser, >> + "/sdn/zones/$vnet_zone/$vnetid", >> + ['SDN.Allocate'], >> + ); >> + >> + if ($param->{delete}) { >> + delete $pool_config->{network}->{$network_key}; >> + } else { >> + $pool_config->{network}->{$network_key} = 1; >> + } >> + } >> + >> cfs_write_file("user.cfg", $usercfg); >> }, >> "update pools failed", >> @@ -437,6 +554,14 @@ __PACKAGE__->register_method({ >> type => 'string', >> optional => 1, >> }, >> + zone => { >> + type => 'string', >> + optional => 1, >> + }, >> + vnet => { >> + type => 'string', >> + optional => 1, >> + }, >> }, >> }, >> }, >> @@ -524,6 +649,14 @@ __PACKAGE__->register_method({ >> die "pool '$pool' is not empty (contains storage '$storeid')\n"; >> } >> >> + for my $netid (sort keys %{ $pool_config->{network} }) { > nit: `$pool_config->{network}->%*` might be more readable ack. > >> + my ($type, $id) = split('/', $netid, 2); >> + $type //= 'network'; >> + $id //= $netid; >> + >> + die "pool '$pool' is not empty (contains $type '$id')\n"; >> + } >> + >> delete($usercfg->{pools}->{$pool}); >> PVE::AccessControl::delete_pool_acl($pool, $usercfg); >> >>