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 81E331FF0B3 for ; Wed, 09 Sep 2026 12:43:15 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 20C14216EF; Wed, 09 Sep 2026 12:42:13 +0200 (CEST) From: Hannes Laimer To: pve-devel@lists.proxmox.com Subject: [PATCH pve-network v2 05/16] sdn: ipam: do not cache negative per-MAC answers, lock the write Date: Wed, 9 Sep 2026 12:41:33 +0200 Message-ID: <20260909104144.1110031-6-h.laimer@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260909104144.1110031-1-h.laimer@proxmox.com> References: <20260909104144.1110031-1-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788950506435 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.561 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: T37S7UVPJYY2GDPVQOCTSG4MO5TYJEZA X-Message-ID-Hash: T37S7UVPJYY2GDPVQOCTSG4MO5TYJEZA 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: The lookup cache wrote an entry even when the plugin returned nothing, and that entry short-circuits every later lookup. So a record created after the first miss was never seen again. The read-modify-write also ran without the cluster lock the other cache writers take, allowing concurrent lookups to drop each other's entries. Only cache actual answers and take the lock for the write, keeping the common cache-hit path lock-free. An entry without an address, as installs running the old code have them, counts as a miss. The cache is keyed by the MAC in lower case, its writers spell it either way. A read and a delete cover an entry of the old code under another spelling as well, and a write moves it to the lower-case key. So a MAC keeps one entry, and a mapping released after the upgrade releases its cache entry too. 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 179bdf7..1612a11 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 of the old code 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, its writers spell it either way 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 of the old code carry the spelling their writer used + 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 of the old code carry the spelling their writer used, 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, 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, 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; diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl index 8273715..c1878dc 100755 --- a/src/test/run_test_vnets_blackbox.pl +++ b/src/test/run_test_vnets_blackbox.pl @@ -1083,6 +1083,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 the old code wrote under the config's spelling is read and + # released like one of the new code + 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 of the old code 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"; -- 2.47.3