Re: [PATCH pve-manager v2 02/10] fix #7294: api: pool: add SDN VNets as pool members

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

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=7294
> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---

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;
>  
>  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_version);
>  use PVE::Exception qw(raise_param_exc);
>  use PVE::INotify;
> +use PVE::Network;
>  use PVE::Storage;
> +use PVE::Tools;
>  
>  use PVE::SafeSyslog;
>  
> @@ -16,6 +18,26 @@ use PVE::RESTHandler;
>  
>  use base qw(PVE::RESTHandler);
>  
> +my $pool_network_format = {
> +    zone => {
> +        description => 'SDN Zone',
> +        type => 'string',
> +        format => 'pve-sdn-zone-id',
> +    },
> +    vnet => {
> +        description => 'VNet to add or remove from this pool.',
> +        type => 'string',
> +        format => 'pve-sdn-vnet-id',
> +    },
> +    tag => {
> +        description => "Specify a VLAN tag",
> +        optional => 1,
> +        type => 'integer',
> +        minimum => 1,
> +        maximum => 4094,
> +    },
> +};
> +
>  __PACKAGE__->register_method({
>      name => 'index',
>      path => '',
> @@ -36,7 +58,7 @@ __PACKAGE__->register_method({
>              },
>              type => {
>                  type => 'string',
> -                enum => ['qemu', 'lxc', 'storage'],
> +                enum => ['qemu', 'lxc', 'storage', 'network'],
>                  optional => 1,
>                  requires => 'poolid',
>              },
> @@ -61,7 +83,7 @@ __PACKAGE__->register_method({
>                          properties => {
>                              type => {
>                                  type => 'string',
> -                                enum => ['qemu', 'lxc', 'openvz', 'storage'],
> +                                enum => ['qemu', 'lxc', 'openvz', 'storage', 'network'],
>                              },
>                              id => {
>                                  type => 'string',
> @@ -135,6 +157,29 @@ __PACKAGE__->register_method({
>                  }
>              }
>  
> +            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) = split('/', $net_key);
> +
> +                        if ($type eq 'vnet') {
> +                            my ($zoneid, $vnet, $tag) = @path;
> +
> +                            my $description = "$vnet ($zoneid)";
> +                            $description = "$vnet.$tag ($zoneid)" if defined($tag);
> +
> +                            push @$members,
> +                                {
> +                                    type => 'network',
> +                                    'network-type' => $type,
> +                                    id => $net_key,
> +                                    text => $description,
> +                                };
> +                        }
> +                    }
> +                }
> +            }
> +
>              my $pool_info = {
>                  members => $members,
>              };
> @@ -243,6 +288,13 @@ __PACKAGE__->register_method({
>                  format => 'pve-storage-id-list',
>                  optional => 1,
>              },
> +            network => {
> +                description => 'Network resource to add or remove from this pool.',
> +                type => 'string',
> +                typetext => 'zone=<zone>,vnet=<vnet>[,tag=<tag>]',
> +                format => $pool_network_format,
> +                optional => 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/<zoneid>/<vnetid>[/<tagid>]' 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 = PVE::Network::SDN::Zones::config();
    my $vnets_cfg = 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' => {
>                  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 +347,13 @@ __PACKAGE__->register_method({
>                  format => 'pve-storage-id-list',
>                  optional => 1,
>              },
> +            network => {
> +                description => 'Network resource to add or remove from this pool.',
> +                type => 'string',
> +                typetext => 'zone=<zone>,vnet=<vnet>[,tag=<tag>]',
> +                format => $pool_network_format,
> +                optional => 1,
> +            },

Same here

>              '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 +363,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 network resource instead of adding them.',
>                  type => 'boolean',
>                  optional => 1,
>                  default => 0,
> @@ -373,6 +432,62 @@ __PACKAGE__->register_method({
>                      }
>                  }
>  
> +                if (defined($param->{network})) {
> +                    my $vnet_entry = PVE::JSONSchema::parse_property_string(
> +                        $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) = $vnet_entry->@{qw(zone vnet tag)};
> +
> +                    my $zones_cfg = PVE::Network::SDN::Zones::config();
> +                    die "SDN Zone '$zone' does not exist\n" if !$zones_cfg->{ids}->{$zone};
> +
> +                    my $vnets_cfg = PVE::Network::SDN::Vnets::config();
> +
> +                    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: use the post-if version here (might allow that the die expr doesn't
     need to be on a new line)

> +
> +                    my $network_key = "vnet/$vnet_zone/$vnetid";
> +
> +                    if (defined($tag)) {
> +                        if (!$vnet_data->{vlanaware}) {
> +                            die
> +                                "VNet '$vnetid' is not VLAN-aware, cannot 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 .= "/$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 already a pool member\n";
> +                        }

nit: use the post-if version here

> +
> +                        $pool_config->{network}->{$network_key} = 1;
> +                    }
> +                }
> +
>                  cfs_write_file("user.cfg", $usercfg);
>              },
>              "update pools failed",
> @@ -400,7 +515,7 @@ __PACKAGE__->register_method({
>              },
>              type => {
>                  type => 'string',
> -                enum => ['qemu', 'lxc', 'storage'],
> +                enum => ['qemu', 'lxc', 'storage', 'network'],
>                  optional => 1,
>              },
>          },
> @@ -421,7 +536,7 @@ __PACKAGE__->register_method({
>                      properties => {
>                          type => {
>                              type => 'string',
> -                            enum => ['qemu', 'lxc', 'openvz', 'storage'],
> +                            enum => ['qemu', 'lxc', 'openvz', 'storage', 'network'],
>                          },
>                          id => {
>                              type => 'string',
> @@ -524,6 +639,14 @@ __PACKAGE__->register_method({
>                      die "pool '$pool' is not empty (contains storage '$storeid')\n";
>                  }
>  
> +                for my $netid (sort keys $pool_config->{network}->%*) {
> +                    my ($type, $id) = split('/', $netid, 2);
> +                    $type = 'network' if !defined($type);
> +                    $id = $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);
>