From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id CF3BC1FF0AA for ; Tue, 20 Oct 2026 15:10:11 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 1136A216C9; Tue, 20 Oct 2026 15:10:09 +0200 (CEST) Message-ID: <05f6ea75-8cd0-4472-b663-3e0130d546c2@proxmox.com> Date: Tue, 20 Oct 2026 15:10:00 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Stefan Hanreich Subject: Re: [PATCH pve-network v3 06/17] sdn: ipam: key the MAC cache by lower-case MAC, stop caching misses To: pve-devel@lists.proxmox.com References: <20261020080005.470757-1-h.laimer@proxmox.com> <20261020080005.470757-7-h.laimer@proxmox.com> Content-Language: en-US In-Reply-To: <20261020080005.470757-7-h.laimer@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.403 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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: ETGYXKTSH2LLSGKODOTVF7GRO3FWBFIH X-Message-ID-Hash: ETGYXKTSH2LLSGKODOTVF7GRO3FWBFIH X-MailFrom: s.hanreich@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 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 > --- > 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";