From: Stefan Hanreich <s.hanreich@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: Re: [PATCH pve-network v3 06/17] sdn: ipam: key the MAC cache by lower-case MAC, stop caching misses
Date: Tue, 20 Oct 2026 15:10:00 +0200 [thread overview]
Message-ID: <05f6ea75-8cd0-4472-b663-3e0130d546c2@proxmox.com> (raw)
In-Reply-To: <20261020080005.470757-7-h.laimer@proxmox.com>
On 10/20/26 10:01 AM, Hannes Laimer wrote:
> Generated guest MACs are upper case, but the API accepts either
> spelling. The cache was keyed by the MAC as its writer spelled it. A
> deletion in the other spelling missed the entry, so the cache kept
> answering with the deleted address. Entries cached before this
> change keep their spelling. A lookup and a deletion still find them,
> and the next write for their MAC moves them to the lower-case key.
>
> A lookup that missed the cache stored the IPAM's empty answer, and
> any entry counted as a hit. So a record created later directly in the
> IPAM stayed hidden, and a guest start allocated another address. An
> empty answer is no longer stored, and an entry without an address
> counts as a miss.
>
> The lookup also wrote the whole cache back without the cluster lock
> the other writers take. A change made between its read and its write
> was lost. The lookup now takes the lock and reads the cache again
> before it stores its answer.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/PVE/Network/SDN/Ipams.pm | 61 +++++++++++---
> src/test/run_test_vnets_blackbox.pl | 118 ++++++++++++++++++++++++++++
> 2 files changed, 168 insertions(+), 11 deletions(-)
>
> diff --git a/src/PVE/Network/SDN/Ipams.pm b/src/PVE/Network/SDN/Ipams.pm
> index 179bdf76..d8051d8d 100644
> --- a/src/PVE/Network/SDN/Ipams.pm
> +++ b/src/PVE/Network/SDN/Ipams.pm
> @@ -47,14 +47,27 @@ sub write_macdb {
> cfs_write_file($macdb_filename, $data);
> }
>
> +# an entry under another spelling moves to the lower-case key
> +my sub fold_spellings {
> + my ($db, $mac) = @_;
> +
> + for my $key (grep { $_ ne $mac && lc($_) eq $mac } reverse sort keys $db->{macs}->%*) {
> + my $old = delete $db->{macs}->{$key};
> + $db->{macs}->{$mac}->{$_} //= $old->{$_} for grep { defined($old->{$_}) } qw(ip4 ip6);
> + }
> +}
> +
> +# the cache is keyed by the MAC in lower case, whatever spelling its writers use
> sub add_cache_mac_ip {
> my ($mac, $ip) = @_;
> + $mac = lc($mac);
>
> cfs_lock_file(
> $macdb_filename,
> undef,
> sub {
> my $db = read_macdb();
> + fold_spellings($db, $mac);
> if (Net::IP::ip_is_ipv4($ip)) {
> $db->{macs}->{$mac}->{ip4} = $ip;
> } else {
> @@ -74,13 +87,17 @@ sub del_cache_mac_ip {
> undef,
> sub {
> my $db = read_macdb();
> - if (Net::IP::ip_is_ipv4($ip)) {
> - delete $db->{macs}->{$mac}->{ip4};
> - } else {
> - delete $db->{macs}->{$mac}->{ip6};
> + # entries from before the cache was keyed in lower case carry their writer's spelling
> + for my $key (grep { lc($_) eq lc($mac) } keys $db->{macs}->%*) {
> + if (Net::IP::ip_is_ipv4($ip)) {
> + delete $db->{macs}->{$key}->{ip4};
> + } else {
> + delete $db->{macs}->{$key}->{ip6};
> + }
> + delete $db->{macs}->{$key}
> + if !defined($db->{macs}->{$key}->{ip4})
> + && !defined($db->{macs}->{$key}->{ip6});
> }
> - delete $db->{macs}->{$mac}
> - if !defined($db->{macs}->{$mac}->{ip4}) && !defined($db->{macs}->{$mac}->{ip6});
> write_macdb($db);
> },
> );
> @@ -136,16 +153,38 @@ sub get_ips_from_mac {
> my ($mac, $zoneid, $zone) = @_;
>
> my $macdb = read_macdb();
> - return ($macdb->{macs}->{$mac}->{ip4}, $macdb->{macs}->{$mac}->{ip6}) if $macdb->{macs}->{$mac};
> + # entries from before the cache was keyed in lower case carry their writer's
> + # spelling, and the lower-case one wins where two hold the same family
> + my %cached;
> + for my $key (grep { lc($_) eq lc($mac) } sort keys $macdb->{macs}->%*) {
> + my $entry = $macdb->{macs}->{$key};
> + $cached{$_} = $entry->{$_} for grep { defined($entry->{$_}) } qw(ip4 ip6);
> + }
> +
> + # an entry holding no address says nothing about the MAC, so ask the IPAM
> + return ($cached{ip4}, $cached{ip6}) if defined($cached{ip4}) || defined($cached{ip6});
>
> my $plugin_config = get_plugin_config($zone);
> my $plugin = PVE::Network::SDN::Ipams::Plugin->lookup($plugin_config->{type});
> - ($macdb->{macs}->{$mac}->{ip4}, $macdb->{macs}->{$mac}->{ip6}) =
> - $plugin->get_ips_from_mac($plugin_config, $mac, $zoneid);
> + my ($ip4, $ip6) = $plugin->get_ips_from_mac($plugin_config, $mac, $zoneid);
> +
> + # an empty answer is not cached, since the record may simply not exist yet
> + return if !defined($ip4) && !defined($ip6);
>
> - write_macdb($macdb);
> + cfs_lock_file(
> + $macdb_filename,
> + undef,
> + sub {
> + my $db = read_macdb();
> + fold_spellings($db, lc($mac));
> + $db->{macs}->{ lc($mac) }->{ip4} = $ip4 if defined($ip4);
> + $db->{macs}->{ lc($mac) }->{ip6} = $ip6 if defined($ip6);
> + write_macdb($db);
> + },
> + );
> + warn "$@" if $@;
>
> - return ($macdb->{macs}->{$mac}->{ip4}, $macdb->{macs}->{$mac}->{ip6});
> + return ($ip4, $ip6);
> }> 1;
would it make sense to move all of this case handling into read_macdb itself? Currently
we're doing it at every call site of read_macdb in Ipams.pm
> diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl
> index d324538e..476e7f8d 100755
> --- a/src/test/run_test_vnets_blackbox.pl
> +++ b/src/test/run_test_vnets_blackbox.pl
> @@ -1118,6 +1118,124 @@ sub test_dnsmasq_mapping_push {
>
> run_test(\&test_dnsmasq_mapping_push);
>
> +sub test_ipam_cache_misses {
> + my $test_name = (split(/::/, (caller(0))[3]))[-1];
> + my $zoneid = "TESTZONE";
> + my $vnetid = "testvnet";
> + my $mac = "da:65:8f:18:9b:6f";
> +
> + create_zone({
> + type => "simple",
> + dhcp => "dnsmasq",
> + ipam => "pve",
> + zone => $zoneid,
> + });
> + create_vnet({
> + type => "vnet",
> + zone => $zoneid,
> + vnet => $vnetid,
> + });
> + create_subnet({
> + type => "subnet",
> + vnet => $vnetid,
> + subnet => "10.0.0.0/24",
> + gateway => "10.0.0.1",
> + 'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> + });
> +
> + my @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
> + is(scalar(grep { defined } @ips), 0, "$test_name: an unknown MAC has no answer");
> + ok(!exists $test_state->{macdb}->{macs}->{$mac}, "$test_name: the miss is not cached");
> +
> + # an entry holding no address, left behind by an earlier miss, does not
> + # hide a record created since
> + create_ip({
> + zone => $zoneid,
> + vnet => $vnetid,
> + mac => $mac,
> + ip => "10.0.0.50",
> + });
> + $test_state->{macdb}->{macs}->{$mac} = { ip4 => undef, ip6 => undef };
> + @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
> + is($ips[0], "10.0.0.50", "$test_name: an address-less entry counts as a miss");
> + is(
> + $test_state->{macdb}->{macs}->{$mac}->{ip4},
> + "10.0.0.50",
> + "$test_name: the answer is cached",
> + );
> +}
> +
> +run_test(\&test_ipam_cache_misses);
> +
> +sub test_ipam_cache_case {
> + my $test_name = (split(/::/, (caller(0))[3]))[-1];
> + my $zoneid = "TESTZONE";
> + my $vnetid = "testvnet";
> + my $mac = "da:65:8f:18:9b:6f";
> +
> + # an entry written under the config's spelling before the cache was keyed in
> + # lower case is read and released like any other
> + create_zone({
> + type => "simple",
> + dhcp => "dnsmasq",
> + ipam => "pve",
> + zone => $zoneid,
> + });
> + create_vnet({
> + type => "vnet",
> + zone => $zoneid,
> + vnet => $vnetid,
> + });
> + create_subnet({
> + type => "subnet",
> + vnet => $vnetid,
> + subnet => "10.0.0.0/24",
> + gateway => "10.0.0.1",
> + 'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> + });
> + $test_state->{macdb}->{macs}->{ uc($mac) } = { ip4 => '10.0.0.150' };
> + my ($ip4) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
> + is($ip4, '10.0.0.150', "$test_name: an old entry is found however it is spelled");
> + PVE::Network::SDN::Ipams::del_cache_mac_ip($mac, '10.0.0.150');
> + ok(
> + !(grep { lc($_) eq $mac } keys $test_state->{macdb}->{macs}->%*),
> + "$test_name: and released",
> + );
> +
> + # a write spelled either way moves such an entry to the lower-case key
> + # and keeps what it held
> + $test_state->{macdb}->{macs}->{ uc($mac) } = { ip4 => '10.0.0.150' };
> + PVE::Network::SDN::Ipams::add_cache_mac_ip(uc($mac), 'fd00::150');
> + is_deeply(
> + [grep { lc($_) eq $mac } keys $test_state->{macdb}->{macs}->%*],
> + [$mac],
> + "$test_name: a write leaves one entry for the MAC",
> + );
> + is_deeply(
> + $test_state->{macdb}->{macs}->{$mac},
> + { ip4 => '10.0.0.150', ip6 => 'fd00::150' },
> + "$test_name: holding both addresses",
> + );
> +
> + # the lookup's write moves an address-less entry under another spelling as well
> + create_ip({
> + zone => $zoneid,
> + vnet => $vnetid,
> + mac => $mac,
> + ip => "10.0.0.160",
> + });
> + $test_state->{macdb}->{macs} = { uc($mac) => { ip4 => undef, ip6 => undef } };
> + my @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
> + is($ips[0], '10.0.0.160', "$test_name: a lookup past an address-less entry asks the IPAM");
> + is_deeply(
> + [sort keys $test_state->{macdb}->{macs}->%*],
> + [$mac],
> + "$test_name: and its write leaves one entry for the MAC",
> + );
> +}
> +
> +run_test(\&test_ipam_cache_case);
> +
> sub test_dnsmasq_dual_stack_and_sweep {
> my $test_name = (split(/::/, (caller(0))[3]))[-1];
> my $zoneid = "TESTZONE";
next prev parent reply other threads:[~2026-10-20 13:10 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-20 7:59 [PATCH container/docs/manager/network/proxmox{-ebpf,-perl-rs}/qemu-server v3 00/17] sdn: implement DHCP for all zones using eBPF Hannes Laimer
2026-10-20 7:59 ` [PATCH proxmox-ebpf v3 01/17] dhcp: add the per-tap DHCPv4 responder program Hannes Laimer
2026-10-20 7:59 ` [PATCH proxmox-ebpf v3 02/17] dhcp: add the subsystem that loads the responder and keeps its records Hannes Laimer
2026-10-20 13:09 ` Stefan Hanreich
2026-10-21 7:09 ` Hannes Laimer
2026-10-20 7:59 ` [PATCH proxmox-perl-rs v3 03/17] pve-rs: sdn: add bindings for the eBPF DHCP responder Hannes Laimer
2026-10-20 13:10 ` Stefan Hanreich
2026-10-21 7:32 ` Hannes Laimer
2026-10-20 7:59 ` [PATCH pve-network v3 04/17] sdn: dhcp: replace a stale dnsmasq reservation instead of dropping it Hannes Laimer
2026-10-20 13:15 ` Stefan Hanreich
2026-10-20 7:59 ` [PATCH pve-network v3 05/17] sdn: dhcp: update reservations when ip mappings change Hannes Laimer
2026-10-20 13:42 ` Stefan Hanreich
2026-10-21 7:21 ` Hannes Laimer
2026-10-20 7:59 ` [PATCH pve-network v3 06/17] sdn: ipam: key the MAC cache by lower-case MAC, stop caching misses Hannes Laimer
2026-10-20 13:10 ` Stefan Hanreich [this message]
2026-10-21 6:17 ` Hannes Laimer
2026-10-20 7:59 ` [PATCH pve-network v3 07/17] sdn: dhcp: make the lease time configurable per subnet Hannes Laimer
2026-10-20 13:17 ` Stefan Hanreich
2026-10-20 7:59 ` [PATCH pve-network v3 08/17] sdn: dhcp: require a backend only if a zone on this node uses it Hannes Laimer
2026-10-20 13:17 ` Stefan Hanreich
2026-10-20 7:59 ` [PATCH pve-network v3 09/17] sdn: dhcp: add an eBPF backend answering on each guest interface Hannes Laimer
2026-10-20 13:10 ` Stefan Hanreich
2026-10-21 6:16 ` Hannes Laimer
2026-10-20 7:59 ` [PATCH pve-network v3 10/17] sdn: zones: report interface plugs and unplugs to the DHCP backends Hannes Laimer
2026-10-20 13:17 ` Stefan Hanreich
2026-10-20 7:59 ` [PATCH pve-network v3 11/17] sdn: dhcp: push IP mapping edits to the guest's node Hannes Laimer
2026-10-20 13:09 ` Stefan Hanreich
2026-10-21 6:44 ` Hannes Laimer
2026-10-20 8:00 ` [PATCH pve-network v3 12/17] sdn: zones: allow the ebpf DHCP backend on every zone type Hannes Laimer
2026-10-20 8:00 ` [PATCH qemu-server v3 13/17] network: sdn: keep the DHCP backends current on guest NICs Hannes Laimer
2026-10-20 13:38 ` Stefan Hanreich
2026-10-20 8:00 ` [PATCH pve-container v3 14/17] net: report NIC plugs and removals to the SDN DHCP backends Hannes Laimer
2026-10-20 13:36 ` Stefan Hanreich
2026-10-21 7:29 ` Hannes Laimer
2026-10-20 8:00 ` [PATCH pve-manager v3 15/17] ui: sdn: add zone DHCP backend selector and subnet DHCP options Hannes Laimer
2026-10-20 13:19 ` Stefan Hanreich
2026-10-20 8:00 ` [PATCH pve-manager v3 16/17] sdn: bring up the DHCP backends at boot before the guests start Hannes Laimer
2026-10-20 13:21 ` Stefan Hanreich
2026-10-21 7:17 ` Hannes Laimer
2026-10-20 8:00 ` [PATCH pve-docs v3 17/17] sdn: dhcp: add eBPF backend section Hannes Laimer
2026-10-20 13:11 ` [PATCH container/docs/manager/network/proxmox{-ebpf,-perl-rs}/qemu-server v3 00/17] sdn: implement DHCP for all zones using eBPF Stefan Hanreich
2026-10-21 7:35 ` Hannes Laimer
2026-10-21 8:46 ` superseded: " Hannes Laimer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=05f6ea75-8cd0-4472-b663-3e0130d546c2@proxmox.com \
--to=s.hanreich@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.