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

["Daniel Kral" <d.kral@proxmox.com>]

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

> +                        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?

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 => {
> +                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.


nit: might be nice to destructure them through a tuple:

    my ($zone, $vnetid, $tag) = $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 = 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?

> +
> +                    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?

> +                    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

> +
> +                    $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

> +                    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);
>