Re: [PATCH pve-manager 2/9] fix #7294: api: pool: add SDN VNets as pool members

[David Riley <d.riley@proxmox.com>]

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 <d.riley@proxmox.com>
>> ---
>>   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);
>>   
>>