From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id D8B861FF153 for ; Mon, 22 Jun 2026 13:20:22 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 387A07E75; Mon, 22 Jun 2026 13:20:22 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 22 Jun 2026 13:20:15 +0200 Message-Id: From: "Daniel Kral" To: "David Riley" , Subject: Re: [PATCH pve-manager 2/9] fix #7294: api: pool: add SDN VNets as pool members X-Mailer: aerc 0.21.0-145-gf21bb67f8cab-dirty References: <20260611145935.147788-1-d.riley@proxmox.com> <20260611145935.147788-3-d.riley@proxmox.com> In-Reply-To: <20260611145935.147788-3-d.riley@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782127206626 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.074 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: UTXTQXKWMPQ335JK2Q47DDUEKX2UDDWE X-Message-ID-Hash: UTXTQXKWMPQ335JK2Q47DDUEKX2UDDWE X-MailFrom: d.kral@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: 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=3D7294 > 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; > =20 > 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; > =20 > use PVE::SafeSyslog; > @@ -61,7 +63,7 @@ __PACKAGE__->register_method({ > properties =3D> { > type =3D> { > type =3D> 'string', > - enum =3D> ['qemu', 'lxc', 'openvz', 'sto= rage'], > + enum =3D> ['qemu', 'lxc', 'openvz', 'sto= rage', 'network'], > }, > id =3D> { > type =3D> 'string', > @@ -77,6 +79,10 @@ __PACKAGE__->register_method({ > type =3D> 'string', > optional =3D> 1, > }, > + vnet =3D> { > + type =3D> 'string', > + optional =3D> 1, > + }, > }, > }, > }, > @@ -135,6 +141,29 @@ __PACKAGE__->register_method({ > } > } > =20 > + 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 > + my ($type, @path) =3D split('/', $net_key); > + > + if ($type eq 'vnet') { > + my ($zoneid, $vnet, $vlan) =3D @path; > + > + my $description =3D "$vnet ($zoneid)"; > + $description =3D "$vnet.$vlan ($zoneid)" if = defined($vlan); > + > + push @$members, > + { > + type =3D> 'network', > + id =3D> $net_key, > + text =3D> $description, > + 'network-type' =3D> $type, > + }; > + } > + } > + } > + } > + > my $pool_info =3D { > members =3D> $members, > }; > @@ -243,6 +272,25 @@ __PACKAGE__->register_method({ > format =3D> 'pve-storage-id-list', > optional =3D> 1, > }, > + zone =3D> { > + description =3D> 'SDN Zone', > + type =3D> 'string', > + format =3D> 'pve-sdn-zone-id', > + optional =3D> 1, > + }, > + vnet =3D> { > + description =3D> 'VNet to add or remove from this pool.'= , > + type =3D> 'string', > + format =3D> 'pve-sdn-vnet-id', > + optional =3D> 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? 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? > + tag =3D> { > + description =3D> "Specify a VLAN tag", > + optional =3D> 1, > + type =3D> 'integer', > + minimum =3D> 1, > + maximum =3D> 4094, > + }, > 'allow-move' =3D> { > description =3D> 'Allow adding a guest even if already i= n another pool.' > . ' The guest will be removed from its current pool = and added to this one.', > @@ -295,6 +343,25 @@ __PACKAGE__->register_method({ > format =3D> 'pve-storage-id-list', > optional =3D> 1, > }, > + zone =3D> { > + description =3D> 'SDN Zone', > + type =3D> 'string', > + format =3D> 'pve-sdn-zone-id', > + optional =3D> 1, > + }, > + vnet =3D> { > + description =3D> 'VNet to add or remove from this pool.'= , > + type =3D> 'string', > + format =3D> 'pve-sdn-vnet-id', > + optional =3D> 1, > + }, > + tag =3D> { > + description =3D> "Specify a VLAN tag", > + optional =3D> 1, > + type =3D> 'integer', > + minimum =3D> 1, > + maximum =3D> 4094, > + }, > 'allow-move' =3D> { > description =3D> 'Allow adding a guest even if already i= n another pool.' > . ' The guest will be removed from its current pool = and added to this one.', > @@ -304,7 +371,7 @@ __PACKAGE__->register_method({ > }, > delete =3D> { > description =3D> > - 'Remove the passed VMIDs and/or storage IDs instead = of adding them.', > + 'Remove the passed VMIDs, storage IDs and/or VNets i= nstead of adding them.', > type =3D> 'boolean', > optional =3D> 1, > default =3D> 0, > @@ -373,6 +440,56 @@ __PACKAGE__->register_method({ > } > } > =20 > + 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. nit: might be nice to destructure them through a tuple: my ($zone, $vnetid, $tag) =3D $param->@{qw(zone vnet tag)}; > + # 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. > + > + my $zones_cfg =3D PVE::Network::SDN::Zones::config()= ; > + my $zone =3D $param->{zone}; > + > + if (!$zones_cfg->{ids}->{$zone}) { > + die "SDN Zone '$zone' does not exist\n"; > + } > + > + my $vnets_cfg =3D PVE::Network::SDN::Vnets::config()= ; > + my $tag =3D $param->{tag}; > + > + my $vnetid =3D $param->{vnet}; > + my $vnet_data =3D $vnets_cfg->{ids}->{$vnetid} > + or die "VNet '$vnetid' does not exist\n"; > + > + my $vnet_zone =3D $vnet_data->{zone}; > + if ($zone ne $vnet_zone) { > + die "VNet '$vnetid' does not belong to zone '$zo= ne' (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? > + > + my $has_tag =3D 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? > + if ($has_tag) { > + my $native_tag =3D $vnet_data->{tag}; > + if (!defined($native_tag) || $tag !=3D $native_t= ag) { > + die > + "VNet '$vnetid' is not VLAN-aware, canno= t assign a specific tag\n" > + if !$vnet_data->{vlanaware}; > + } > + } > + > + my $network_key =3D "vnet/$vnet_zone/$vnetid"; > + $network_key .=3D "/$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 > + > + $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} =3D 1; > + } > + } > + > cfs_write_file("user.cfg", $usercfg); > }, > "update pools failed", > @@ -437,6 +554,14 @@ __PACKAGE__->register_method({ > type =3D> 'string', > optional =3D> 1, > }, > + zone =3D> { > + type =3D> 'string', > + optional =3D> 1, > + }, > + vnet =3D> { > + type =3D> 'string', > + optional =3D> 1, > + }, > }, > }, > }, > @@ -524,6 +649,14 @@ __PACKAGE__->register_method({ > die "pool '$pool' is not empty (contains storage '$s= toreid')\n"; > } > =20 > + for my $netid (sort keys %{ $pool_config->{network} }) { nit: `$pool_config->{network}->%*` might be more readable > + my ($type, $id) =3D split('/', $netid, 2); > + $type //=3D 'network'; > + $id //=3D $netid; > + > + die "pool '$pool' is not empty (contains $type '$id'= )\n"; > + } > + > delete($usercfg->{pools}->{$pool}); > PVE::AccessControl::delete_pool_acl($pool, $usercfg); > =20