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 CCB851FF142 for ; Fri, 03 Jul 2026 15:19:11 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 9982721210; Fri, 03 Jul 2026 15:19:11 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 03 Jul 2026 15:19:04 +0200 Message-Id: From: "Daniel Kral" To: "David Riley" , Subject: Re: [PATCH pve-manager v2 02/10] fix #7294: api: pool: add SDN VNets as pool members X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260626131035.112374-1-d.riley@proxmox.com> <20260626131035.112374-3-d.riley@proxmox.com> In-Reply-To: <20260626131035.112374-3-d.riley@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783084737258 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.030 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) 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: O2B3X26DOHIWP55K3IMBHQ3TIAXGJDQP X-Message-ID-Hash: O2B3X26DOHIWP55K3IMBHQ3TIAXGJDQP 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: 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=3D7294 > 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. 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. > 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; > =20 > 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_ver= sion); > use PVE::Exception qw(raise_param_exc); > use PVE::INotify; > +use PVE::Network; > use PVE::Storage; > +use PVE::Tools; > =20 > use PVE::SafeSyslog; > =20 > @@ -16,6 +18,26 @@ use PVE::RESTHandler; > =20 > use base qw(PVE::RESTHandler); > =20 > +my $pool_network_format =3D { > + zone =3D> { > + description =3D> 'SDN Zone', > + type =3D> 'string', > + format =3D> 'pve-sdn-zone-id', > + }, > + vnet =3D> { > + description =3D> 'VNet to add or remove from this pool.', > + type =3D> 'string', > + format =3D> 'pve-sdn-vnet-id', > + }, > + tag =3D> { > + description =3D> "Specify a VLAN tag", > + optional =3D> 1, > + type =3D> 'integer', > + minimum =3D> 1, > + maximum =3D> 4094, > + }, > +}; > + > __PACKAGE__->register_method({ > name =3D> 'index', > path =3D> '', > @@ -36,7 +58,7 @@ __PACKAGE__->register_method({ > }, > type =3D> { > type =3D> 'string', > - enum =3D> ['qemu', 'lxc', 'storage'], > + enum =3D> ['qemu', 'lxc', 'storage', 'network'], > optional =3D> 1, > requires =3D> 'poolid', > }, > @@ -61,7 +83,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', > @@ -135,6 +157,29 @@ __PACKAGE__->register_method({ > } > } > =20 > + 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}. > + for my $net_key (sort keys $pool_config->{network}->= %*) { > + my ($type, @path) =3D split('/', $net_key); > + > + if ($type eq 'vnet') { > + my ($zoneid, $vnet, $tag) =3D @path; > + > + my $description =3D "$vnet ($zoneid)"; > + $description =3D "$vnet.$tag ($zoneid)" if d= efined($tag); > + > + push @$members, > + { > + type =3D> 'network', > + 'network-type' =3D> $type, > + id =3D> $net_key, > + text =3D> $description, > + }; > + } > + } > + } > + } > + > my $pool_info =3D { > members =3D> $members, > }; > @@ -243,6 +288,13 @@ __PACKAGE__->register_method({ > format =3D> 'pve-storage-id-list', > optional =3D> 1, > }, > + network =3D> { > + description =3D> 'Network resource to add or remove from= this pool.', > + type =3D> 'string', > + typetext =3D> 'zone=3D,vnet=3D[,tag=3D]= ', > + format =3D> $pool_network_format, > + optional =3D> 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 =3D PVE::Network::SDN::Zones::config(); my $vnets_cfg =3D 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? > '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 +347,13 @@ __PACKAGE__->register_method({ > format =3D> 'pve-storage-id-list', > optional =3D> 1, > }, > + network =3D> { > + description =3D> 'Network resource to add or remove from= this pool.', > + type =3D> 'string', > + typetext =3D> 'zone=3D,vnet=3D[,tag=3D]= ', > + format =3D> $pool_network_format, > + optional =3D> 1, > + }, Same here > '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 +363,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 network= resource instead of adding them.', > type =3D> 'boolean', > optional =3D> 1, > default =3D> 0, > @@ -373,6 +432,62 @@ __PACKAGE__->register_method({ > } > } > =20 > + if (defined($param->{network})) { > + my $vnet_entry =3D PVE::JSONSchema::parse_property_s= tring( > + $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. > + > + my ($zone, $vnetid, $tag) =3D $vnet_entry->@{qw(zone= vnet tag)}; > + > + my $zones_cfg =3D PVE::Network::SDN::Zones::config()= ; > + die "SDN Zone '$zone' does not exist\n" if !$zones_c= fg->{ids}->{$zone}; > + > + my $vnets_cfg =3D PVE::Network::SDN::Vnets::config()= ; > + > + 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: use the post-if version here (might allow that the die expr doesn't need to be on a new line) > + > + my $network_key =3D "vnet/$vnet_zone/$vnetid"; > + > + if (defined($tag)) { > + if (!$vnet_data->{vlanaware}) { > + die > + "VNet '$vnetid' is not VLAN-aware, canno= t 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 .=3D "/$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 alre= ady a pool member\n"; > + } nit: use the post-if version here > + > + $pool_config->{network}->{$network_key} =3D 1; > + } > + } > + > cfs_write_file("user.cfg", $usercfg); > }, > "update pools failed", > @@ -400,7 +515,7 @@ __PACKAGE__->register_method({ > }, > type =3D> { > type =3D> 'string', > - enum =3D> ['qemu', 'lxc', 'storage'], > + enum =3D> ['qemu', 'lxc', 'storage', 'network'], > optional =3D> 1, > }, > }, > @@ -421,7 +536,7 @@ __PACKAGE__->register_method({ > properties =3D> { > type =3D> { > type =3D> 'string', > - enum =3D> ['qemu', 'lxc', 'openvz', 'storage= '], > + enum =3D> ['qemu', 'lxc', 'openvz', 'storage= ', 'network'], > }, > id =3D> { > type =3D> 'string', > @@ -524,6 +639,14 @@ __PACKAGE__->register_method({ > die "pool '$pool' is not empty (contains storage '$s= toreid')\n"; > } > =20 > + for my $netid (sort keys $pool_config->{network}->%*) { > + my ($type, $id) =3D split('/', $netid, 2); > + $type =3D 'network' if !defined($type); > + $id =3D $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); > =20