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

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

On 6/24/26 9:33 AM, Daniel Kral wrote:
> 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.

Apart from the complexity it would add to the backend logic, designing a WebUI
might also not be trivial, due to the fact that a VNet always depends on a specific
zone. This is made worse by the optional tag each VNet could get assigned.
So for now I'll skip this.

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

I agree combining them does seem like the way to go.
I will be using a property string in v2.
zone=<zone>,vnet=<vnet>[,tag=<tag>]

This way these options will be grouped under the network parameter, future
proofing it for further additions.

So instead of having single parameters (CLI):
--zone <zone> --vnet <vnet> --tag <tag>

we would end up with the following:
--network zone=<zone>,vnet=<vnet>,tag=<tag>

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