From: Hannes Laimer <h.laimer@proxmox.com>
To: Stefan Hanreich <s.hanreich@proxmox.com>, 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: Wed, 21 Oct 2026 08:17:36 +0200 [thread overview]
Message-ID: <93b14d92-e3d7-4988-af86-0c3cb1c7c6da@proxmox.com> (raw)
In-Reply-To: <05f6ea75-8cd0-4472-b663-3e0130d546c2@proxmox.com>
On 2026-10-20 15:10, Stefan Hanreich wrote:
> 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
>
yes, ill include something in v4!
>> 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-21 6:17 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
2026-10-21 6:17 ` Hannes Laimer [this message]
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=93b14d92-e3d7-4988-af86-0c3cb1c7c6da@proxmox.com \
--to=h.laimer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=s.hanreich@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox