From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B53691FF16B for ; Thu, 12 Dec 2024 18:06:56 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7DC23187B; Thu, 12 Dec 2024 18:07:01 +0100 (CET) Message-ID: Date: Thu, 12 Dec 2024 18:06:56 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Stefan Hanreich To: Proxmox VE development discussion , Daniel Herzig References: <20241205163332.130930-1-d.herzig@proxmox.com> <20241205163332.130930-3-d.herzig@proxmox.com> Content-Language: en-US In-Reply-To: <20241205163332.130930-3-d.herzig@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.659 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 Subject: Re: [pve-devel] [PATCH network 2/4] fix #5900: add helper functions X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" I think in this case it is a lot easier to ask for forgiveness rather than asking for permission. One could just try to allocate the IP and check if that fails, avoiding the need to implement a complete check for the IP allocation. Checking beforehand also leads to some TOCTOU issues (admittedly small, since in the window between the check and the actual IP allocation the state of the IPAM could change). I talked with Daniel off-list and the initial implementation actually did only try to allocate and then error out, but the reasoning for implementing it this way was: If you create a VM and then allocating an IP fails the wizard closes, but the task fails. The user would then have to create the VM configuration completely from scratch, which is bad UX. So the tradeoff here is UX vs some technical reasons (TOCTOU, implementation complexity). I think Daniel also talked to you off-list @Aaron, so it'd be great if you could chime in here. Nevertheless I have added some notes on the Perl code, regardless of which option we choose, although I think we could do completely without implementing a manual check which would obsolete this commit. But the general ideas apply for future patch series as well, so I thought they'd help either way. Some general notes on the architecture: If we do it this way (see top-level discussion), I think we should abstract this into the IpamPlugins itself, since this implementation is specific to the PVE Plugin, but that's just one type of IPAM plugin. Something like: Add a abstract method in the base Ipam plugin (Network/SDN/Ipams/Plugin.pm), i.e. PVE::Network::SDN::Ipams::Plugin::vnet_has_free_ip($range, $ipversions) Then implement it for every IPAM Plugin separately. Add a helper method to the VNet that selects the correct plugin based on the zone setting and then iterates over all its subnets to check for free IPs - something like: PVE::Network::SDN::Vnets::has_free_ip($range, $ipversions) The current implementation only works for the PVE plugin and would actually break on zones using Netbox / Phpipam (if my brain compiler is correct). On 12/5/24 17:33, Daniel Herzig wrote: > This patch adds helper functions to evaluate if a vnet (bridge) associated > with a zone under SDN's auto-dhcp control via dnsmasq can retrieve a > dhcp lease. > > Signed-off-by: Daniel Herzig > --- > src/PVE/Network/SDN/Dhcp.pm | 83 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 83 insertions(+) > > diff --git a/src/PVE/Network/SDN/Dhcp.pm b/src/PVE/Network/SDN/Dhcp.pm > index d48de34..4ddd128 100644 > --- a/src/PVE/Network/SDN/Dhcp.pm > +++ b/src/PVE/Network/SDN/Dhcp.pm > @@ -128,4 +128,87 @@ sub regenerate_config { > } > } > > +sub defined_dhcp_ip_count_in_zone { > + my $zone_id = shift; even with 1 argument I think we prefer `my ($arg) = @_;`, but I haven't actually found a definitive answer in our style guide. > + my $vnets_in_zone = PVE::Network::SDN::Zones::get_vnets($zone_id); > + my $range_count_array; > + my $res; > + for my $vnet_id (keys %$vnets_in_zone) { > + my $subnets_in_vnet = PVE::Network::SDN::Vnets::get_subnets($vnet_id); > + for my $subnet (keys %$subnets_in_vnet) { > + my $dhcp_ranges = PVE::Network::SDN::Subnets::get_dhcp_ranges(${subnets_in_vnet}->{$subnet}); > + if (scalar @$dhcp_ranges) { > + for my $dhcp_range (@$dhcp_ranges) { You can just iterate over @$dhcp_ranges, get_dhcp_ranges() always returns an array reference. If it is empty, then there are just 0 iterations of the loop, no need to check for existence. > + my $start_ip = ${dhcp_range}->{'start-address'}; $dhcp_range is fine instead of ${dhcp_range} > + my $end_ip = ${dhcp_range}->{'end-address'}; > + my $subnet_ip_count = new Net::IP("$start_ip - $end_ip")->size(); > + push (@$range_count_array, $subnet_ip_count); > + } > + } > + } > + } > + if ($range_count_array) { > + $res = eval join '+', @$range_count_array; I think it would be easier to just define the variable at the top to be 0: my $res = 0; and then add the size to it during the loop: $count += new Net::IP("$start_ip - $end_ip")->size(); > + } > + return $res; > +} > +> +sub used_dhcp_ips_in_zone { > + my $zone_id = shift; > + my $pve_ipam_db = PVE::Network::SDN::Ipams::PVEPlugin::read_db(); > + my $subnets_in_zone = $pve_ipam_db->{'zones'}->{$zone_id}->{'subnets'}; > + my $res; similarly to the above method we could define res as 0 here > + for my $subnet_in_zone (keys %$subnets_in_zone) { > + my $ips_in_subnet = ${subnets_in_zone}->{$subnet_in_zone}->{'ips'}; After moving this to PVEPlugin, this could maybe also be split into some helper functions like PVEPlugin::get_ips_for_subnet() or something. > + if (scalar (keys %$ips_in_subnet)) { > + for my $leased_ip (keys %$ips_in_subnet) { > + $res++ if (!exists ${ips_in_subnet}->{$leased_ip}->{'gateway'}); > + } > + } > + } > + return $res; > +} > + > +sub available_dhcp_ips_in_zone { > + my $zone_id = shift; > + my $available_ip_count = defined_dhcp_ip_count_in_zone($zone_id); > + my $used_ip_count = used_dhcp_ips_in_zone($zone_id); > + if (!defined($available_ip_count)) { > + $available_ip_count = 0; > + } > + if (!defined($used_ip_count)) { > + $used_ip_count = 0; > + } If you define $res to be 0 as suggested above, then those checks become unnecessary, since 0 becomes the default value. > + my $res = $available_ip_count - $used_ip_count; > + return $res; > +} > + > +sub test_bridge_for_sdn_dnsmasq { this function would basically be the helper inside the VNet - it just needs to consider the IPAM plugin as well. > + my $bridge = shift; > + my $vnets_cfg = PVE::Network::SDN::Vnets::config(); config() takes $running as parameter. With SDN we have a running config and a 'normal' config (that stores all changes you make before applying them). In this case we need to use the running config rather than the normal config. > + my $vnet_ids = [ PVE::Network::SDN::Vnets::sdn_vnets_ids($vnets_cfg) ]; no need for selecting the ids, you can directly iterate over the VNets: my $vnets = PVE::Network::SDN::Vnets::config(1); foreach my $vnet_id (sort keys %{$vnets->{ids}}) { $vnet = PVE::Network::SDN::Vnets::sdn_vnets_config($vnets,$vnet_id); } With Vnets there is no difference, but with Subnets there is a subtle difference, since it generates some additional keys from the section_id (see Subnets.pm). So it is preferable to go via this function ----- An even easier solution - using the fact that VNet names are always equivalent to the bridge names: my $vnet = PVE::Network::SDN::Vnets::get_vnet($bridge) This would give you the respective VNet (or undef if it isn't a VNet) that corresponds to the bridge. Then you just need to get the zone for the VNet: my $zone = PVE::Network::SDN::Zones::get_zone($vnet->{zone}); and then you could just easily check if that zone has DHCP enabled, saving you both loops. > + my $zones_cfg = PVE::Network::SDN::Zones::config(); > + my $zone_ids = [ PVE::Network::SDN::Zones::sdn_zones_ids($zones_cfg) ]; same as above - maybe even just defining a helper would be even better? > + my $dhcp_dnsmasq_zones; > + my $vnets_in_dhcp_dnsmasq_zones; > + for my $zone (@$zone_ids) { > + push(@$dhcp_dnsmasq_zones, $zone) > + if (defined(${zones_cfg}->{'ids'}->{$zone}->{'dhcp'}) && > + (${zones_cfg}->{'ids'}->{$zone}->{'dhcp'} eq 'dnsmasq')) > + } > + for my $vnet (@$vnet_ids) { > + my $vnet_zone = ${vnets_cfg}->{'ids'}->{$vnet}->{'zone'}; > + push(@$vnets_in_dhcp_dnsmasq_zones, $vnet) > + if ("@$dhcp_dnsmasq_zones" =~ /$vnet_zone/) If you want to check for containment in an array it is preferable to use grep: my @array = (1, 2, 3); if (grep {$_ eq 1} @array) > + } > + if (("@$vnets_in_dhcp_dnsmasq_zones" =~ /$bridge/)) { same > + my $zone_id = ${vnets_cfg}->{'ids'}->{$bridge}->{'zone'}; > + if ((PVE::Network::SDN::Dhcp::available_dhcp_ips_in_zone($zone_id)) lt 1) { > + die "No DHCP leases left in zone '$zone_id' for bridge '$bridge', please check your SDN config.\n"; > + } > + } > +} > + > + > + > 1; _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel