From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id B41ED1FF0AB for ; Wed, 21 Oct 2026 08:17:43 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 1C73521595; Wed, 21 Oct 2026 08:17:42 +0200 (CEST) Message-ID: <93b14d92-e3d7-4988-af86-0c3cb1c7c6da@proxmox.com> Date: Wed, 21 Oct 2026 08:17:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-network v3 06/17] sdn: ipam: key the MAC cache by lower-case MAC, stop caching misses To: Stefan Hanreich , pve-devel@lists.proxmox.com References: <20261020080005.470757-1-h.laimer@proxmox.com> <20261020080005.470757-7-h.laimer@proxmox.com> <05f6ea75-8cd0-4472-b663-3e0130d546c2@proxmox.com> From: Hannes Laimer Content-Language: en-US In-Reply-To: <05f6ea75-8cd0-4472-b663-3e0130d546c2@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792563456748 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.394 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: U3T3LHYCBI6M3BCQAW4NALZOZEV62M5I X-Message-ID-Hash: U3T3LHYCBI6M3BCQAW4NALZOZEV62M5I X-MailFrom: h.laimer@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 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 >> --- >> 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"; > > > > >