[pve-devel] [PATCH guest-common, network, container, qemu-server 0/4] fix #5900: enhance SDN auto-dhcp behaviour

[pve-devel] [PATCH guest-common, network, container, qemu-server 0/4] fix #5900: enhance SDN auto-dhcp behaviour

[Daniel Herzig]

This patch series prevents containers and vms to be created when their bridges are within an SDN zone,
that is set to deploy IPs automatically via dnsmasq, while the dhcp range is exhausted.

Currently adding a bridge under these circumstances is already impossible, however on VM or container
creation, this is overridden, which can lead to confusing behaviour as described in detail on bugzilla.

*** BLURB HERE ***

guest-common: Daniel Herzig (1):
  fix #5900: add helper function

 src/PVE/GuestHelpers.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

network: Daniel Herzig (1):
  fix #5900: add helper functions

 src/PVE/Network/SDN/Dhcp.pm | 83 +++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

container: Daniel Herzig (1):
  fix #5900: do not create container if dhcp range is exhausted

 src/PVE/API2/LXC.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

qemu-server: Daniel Herzig (1):
  fix #5900: do not create vm if dhcp range is exhausted

 PVE/API2/Qemu.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH guest-common 1/4] fix #5900: add helper function

[Daniel Herzig]

This patch adds a small helper function to retrieve the bridge name
from the netN parameter string of a container or VM configuration.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 src/PVE/GuestHelpers.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
index 592b4a8..c6006ba 100644
--- a/src/PVE/GuestHelpers.pm
+++ b/src/PVE/GuestHelpers.pm
@@ -450,4 +450,15 @@ sub abort_guest_tasks {
     return $aborted_tasks;
 }
 
+sub get_bridge {
+    my $net_params = shift;
+    my $param_array = [ split(/,/, $net_params) ];
+    my $bridge;
+    for my $net_param (@$param_array) {
+	$bridge = $net_param if ($net_param =~ /bridge\=/);
+	$bridge =~ s|bridge\=|| if (defined($bridge));
+    }
+    return $bridge;
+}
+
 1;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH network 2/4] fix #5900: add helper functions

[Daniel Herzig]

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 <d.herzig@proxmox.com>
---
 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;
+    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) {
+		    my $start_ip = ${dhcp_range}->{'start-address'};
+		    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;
+    }
+    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;
+    for my $subnet_in_zone (keys %$subnets_in_zone) {
+	my $ips_in_subnet = ${subnets_in_zone}->{$subnet_in_zone}->{'ips'};
+	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;
+    }
+    my $res = $available_ip_count - $used_ip_count;
+    return $res;
+}
+
+sub test_bridge_for_sdn_dnsmasq {
+    my $bridge = shift;
+    my $vnets_cfg = PVE::Network::SDN::Vnets::config();
+    my $vnet_ids = [ PVE::Network::SDN::Vnets::sdn_vnets_ids($vnets_cfg) ];
+    my $zones_cfg = PVE::Network::SDN::Zones::config();
+    my $zone_ids = [ PVE::Network::SDN::Zones::sdn_zones_ids($zones_cfg) ];
+    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 (("@$vnets_in_dhcp_dnsmasq_zones" =~ /$bridge/)) {
+	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;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH container 3/4] fix #5900: do not create container if dhcp range is exhausted

[Daniel Herzig]

This patch prevents containers to be created if their bridge is
associated with a SDN zone with dnsmasq automatic dhcp enabled, and if
the dhcp-range is exhausted or unset.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 src/PVE/API2/LXC.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 213e518..35fb0a6 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -244,6 +244,18 @@ __PACKAGE__->register_method({
 	    PVE::LXC::Config->check_protection($conf, "unable to restore CT $vmid");
 	}
 
+	for my $opt (keys %$param) {
+	    if ($opt =~ /net/) {
+		my $bridge_err;
+		my $bridge = PVE::GuestHelpers::get_bridge(${param}->{$opt});
+		if (defined($bridge)) {
+		    eval { PVE::Network::SDN::Dhcp::test_bridge_for_sdn_dnsmasq($bridge) };
+		    ${bridge_err}->{$opt} = $@ if $@;
+		    raise_param_exc($bridge_err) if $bridge_err;
+		}
+	    }
+	}
+
 	my $password = extract_param($param, 'password');
 	my $ssh_keys = extract_param($param, 'ssh-public-keys');
 	PVE::Tools::validate_ssh_public_keys($ssh_keys) if defined($ssh_keys);
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server 4/4] fix #5900: do not create vm if dhcp range is exhausted

[Daniel Herzig]

This patch prevents VMs to be created if their bridge is
associated with a SDN zone with dnsmasq automatic dhcp enabled, and if
the dhcp-range is exhausted or unset.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 PVE/API2/Qemu.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index ec45d5ff..73580c64 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1065,6 +1065,18 @@ __PACKAGE__->register_method({
 	    raise_perm_exc();
 	}
 
+	for my $opt (keys %$param) {
+	    if ($opt =~ /net/) {
+		my $bridge_err;
+		my $bridge = PVE::GuestHelpers::get_bridge(${param}->{$opt});
+		if (defined($bridge)) {
+		    eval { PVE::Network::SDN::Dhcp::test_bridge_for_sdn_dnsmasq($bridge) };
+		    ${bridge_err}->{$opt} = $@ if $@;
+		    raise_param_exc($bridge_err) if $bridge_err;
+		}
+	    }
+	}
+
 	if ($archive) {
 	    for my $opt (sort keys $param->%*) {
 		if (PVE::QemuServer::Drive::is_valid_drivename($opt)) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH network 2/4] fix #5900: add helper functions

[Stefan Hanreich]

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 <d.herzig@proxmox.com>
> ---
>  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

Re: [pve-devel] [PATCH guest-common 1/4] fix #5900: add helper function

[Stefan Hanreich]

On 12/5/24 17:33, Daniel Herzig wrote:
> This patch adds a small helper function to retrieve the bridge name
> from the netN parameter string of a container or VM configuration.
> 
> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
> ---
>  src/PVE/GuestHelpers.pm | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
> index 592b4a8..c6006ba 100644
> --- a/src/PVE/GuestHelpers.pm
> +++ b/src/PVE/GuestHelpers.pm
> @@ -450,4 +450,15 @@ sub abort_guest_tasks {
>      return $aborted_tasks;
>  }
>  
> +sub get_bridge {
> +    my $net_params = shift;
> +    my $param_array = [ split(/,/, $net_params) ];
> +    my $bridge;
> +    for my $net_param (@$param_array) {
> +	$bridge = $net_param if ($net_param =~ /bridge\=/);
> +	$bridge =~ s|bridge\=|| if (defined($bridge));
> +    }
> +    return $bridge;
> +}
> +
>  1;

net is a property string, if you want to parse it there are helpers for
that in PVE::JSONSchema.

For VMs as well as CTs we already have helpers for parsing the network
property string defined:

PVE::QemuServer::parse_net
PVE::LXC::Config::parse_lxc_network


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH network 2/4] fix #5900: add helper functions

[Daniel Herzig]

Hey Stefan,

thanks for the feedback!

Stefan Hanreich <s.hanreich@proxmox.com> writes:

>
> 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)
>
I like this thought a lot, sounds like a much cleaner and modular
solution.

>
>
> The current implementation only works for the PVE plugin and would
> actually break on zones using Netbox / Phpipam (if my brain compiler is
> correct).
>
>

Thanks for the hint -- I need to do some research on that!  The code
assumes the ~key: value~ of ~dhcp: dnsmasq~ to be exclusive for IPAM
PVE.

```
for my $zone (@$zone_ids) {
   push(@$dhcp_dnsmasq_zones, $zone)
    if (defined(${zones_cfg}->{'ids'}->{$zone}->{'dhcp'}) &&
	(${zones_cfg}->{'ids'}->{$zone}->{'dhcp'} eq 'dnsmasq'))
}
```

If this is not the case, it will affect (and not ignore as intended)
zones with Netbox/Phpipam indeed. That would be bad.

A check for ~ipam: pve~ could however be easily implemented in the same section.

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

Thanks, not having any feelings here.

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

Right, this is too timid indeed :)

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

Very cool, thank you, I did not see that.  

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

Thank you  for your insights and the hints very much. I cannot claim that I'm exactly happy about
the current implementation of this patch from an aesthetical point of
view by now! I will act on it further, if we consider the
'ask-for-permission' approach in general.



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH guest-common 1/4] fix #5900: add helper function

[Daniel Herzig]

Hi Stefan,

thanks for the feedback.

Thanks for hinting at ~PVE::LXC::Config::parse_lxc_network~. I oversaw
that and did not want to pull ~PVE::QemuServer::parse_net~ into LXC's
deps.


Stefan Hanreich <s.hanreich@proxmox.com> writes:

> On 12/5/24 17:33, Daniel Herzig wrote:
>> This patch adds a small helper function to retrieve the bridge name
>> from the netN parameter string of a container or VM configuration.
>> 
>> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
>> ---
>>  src/PVE/GuestHelpers.pm | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
>> index 592b4a8..c6006ba 100644
>> --- a/src/PVE/GuestHelpers.pm
>> +++ b/src/PVE/GuestHelpers.pm
>> @@ -450,4 +450,15 @@ sub abort_guest_tasks {
>>      return $aborted_tasks;
>>  }
>>  
>> +sub get_bridge {
>> +    my $net_params = shift;
>> +    my $param_array = [ split(/,/, $net_params) ];
>> +    my $bridge;
>> +    for my $net_param (@$param_array) {
>> +	$bridge = $net_param if ($net_param =~ /bridge\=/);
>> +	$bridge =~ s|bridge\=|| if (defined($bridge));
>> +    }
>> +    return $bridge;
>> +}
>> +
>>  1;
>
> net is a property string, if you want to parse it there are helpers for
> that in PVE::JSONSchema.
>
> For VMs as well as CTs we already have helpers for parsing the network
> property string defined:
>
> PVE::QemuServer::parse_net
> PVE::LXC::Config::parse_lxc_network


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel