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

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

On Mon Jun 22, 2026 at 3:37 PM CEST, David Riley wrote:
> On 6/22/26 1:20 PM, Daniel Kral wrote:
>> On Thu Jun 11, 2026 at 4:59 PM CEST, David Riley wrote:

[ snip ]

>>> @@ -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.
>

Good to know, yeah if there's currently no strong demand for it then
it's better left out for now and it can be added later anyway as it can
be easily extended.

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

Right, I only thought about it as 'vms' and 'storage' does specify one
entity and there seems to be 2 (3?) for VNet entries, and it would make
the extra error path for one of them missing obsolete.

Maybe there's some way how these can be specified in one string with the
tag being optional at the end? Though I don't know enough about the SDN
stack here.

>>> +            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.',

[ snip ]

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

Yes, but if $zone eq $vnet_data->{zone} is required to continue on here,
the additional variable is only a duplicate, no?

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

All good :)

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

I see, no strong feelings about this, so choose as you like. I only
thought that it also makes clear that the $network_key changes if the
optional $tag is given by the user.

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

[ snip ]