From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 07F421FF13A for ; Wed, 24 Jun 2026 14:07:05 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BED781753E; Wed, 24 Jun 2026 14:07:02 +0200 (CEST) Message-ID: <15c789bb-da51-4f71-a1ec-61badb6fdbb5@proxmox.com> Date: Wed, 24 Jun 2026 14:06:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-manager 2/9] fix #7294: api: pool: add SDN VNets as pool members To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260611145935.147788-1-d.riley@proxmox.com> <20260611145935.147788-3-d.riley@proxmox.com> Content-Language: en-US From: David Riley In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782302814082 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.151 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: POHRUYRMYJT7WJ66ATN745WO3OJV3QLL X-Message-ID-Hash: POHRUYRMYJT7WJ66ATN745WO3OJV3QLL X-MailFrom: d.riley@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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=,vnet=[,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 --vnet --tag we would end up with the following: --network zone=,vnet=,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 ] > > > >