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 A6ADD1FF09C for ; Mon, 02 Nov 2026 15:56:55 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 453D521763; Mon, 02 Nov 2026 15:56:40 +0100 (CET) From: Hannes Laimer To: pve-devel@lists.proxmox.com Subject: [PATCH pve-network v2 02/11] sdn: ipam: write the lookup's cache entry under the cluster lock Date: Mon, 2 Nov 2026 15:56:21 +0100 Message-ID: <20261102145630.1555953-3-h.laimer@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261102145630.1555953-1-h.laimer@proxmox.com> References: <20261102145630.1555953-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: 1793631396034 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.365 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: FUMFZQMZES7M4IC2DQMC7HIRYUVR5752 X-Message-ID-Hash: FUMFZQMZES7M4IC2DQMC7HIRYUVR5752 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 that stores the IPAM's answer 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. The IPAM is still asked outside the lock, since the lock is cluster-wide and a request to an external IPAM does not belong under it. A cache write that lands while the IPAM is asked may be newer than the answer, so the answer is then returned without being stored, and the next lookup stores it. Signed-off-by: Hannes Laimer --- src/PVE/Network/SDN/Ipams.pm | 24 +++++++++++++++++++----- src/test/run_test_vnets_blackbox.pl | 11 +++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/PVE/Network/SDN/Ipams.pm b/src/PVE/Network/SDN/Ipams.pm index 7e7bec27b5ed..ae2c4337ef8d 100644 --- a/src/PVE/Network/SDN/Ipams.pm +++ b/src/PVE/Network/SDN/Ipams.pm @@ -135,8 +135,9 @@ sub complete_sdn_vnet { sub get_ips_from_mac { my ($mac, $zoneid, $zone) = @_; - my $macdb = read_macdb(); - my $cached = $macdb->{macs}->{$mac}; + PVE::Cluster::cfs_update(); # the lock refreshes, so the snapshot must too + my $version = PVE::Cluster::cfs_file_version($macdb_filename); + my $cached = read_macdb()->{macs}->{$mac}; # an entry without an address was left behind by a lookup that missed, and # says nothing about its MAC return ($cached->{ip4}, $cached->{ip6}) @@ -149,9 +150,22 @@ sub get_ips_from_mac { # an empty answer is not cached, since the record may simply not exist yet return if !defined($ip4) && !defined($ip6); - $macdb->{macs}->{$mac}->{ip4} = $ip4 if defined($ip4); - $macdb->{macs}->{$mac}->{ip6} = $ip6 if defined($ip6); - write_macdb($macdb); + cfs_lock_file( + $macdb_filename, + undef, + sub { + # the IPAM was asked outside the lock, so a write since the miss may + # be newer than the answer. The fill is then left to the next lookup + return + if (PVE::Cluster::cfs_file_version($macdb_filename) // '') ne ($version // ''); + + my $db = read_macdb(); + $db->{macs}->{$mac}->{ip4} = $ip4 if defined($ip4); + $db->{macs}->{$mac}->{ip6} = $ip6 if defined($ip6); + write_macdb($db); + }, + ); + warn "$@" if $@; return ($ip4, $ip6); } diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl index fd3cfa949108..0362b7ca2f8d 100755 --- a/src/test/run_test_vnets_blackbox.pl +++ b/src/test/run_test_vnets_blackbox.pl @@ -473,6 +473,17 @@ sub test_ipam_cache_lookup { "10.0.0.50", "$test_name: the answer is cached", ); + + # a write to the cache between the miss and the fill, which the file's + # version shows, keeps the IPAM's answer out of the cache + delete $test_state->{macdb}->{macs}->{$mac}; + my $version = 0; + $mocked_pve_cluster_obj->mock(cfs_file_version => sub { return $version++ }); + @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac); + $mocked_pve_cluster_obj->unmock('cfs_file_version'); + is($ips[0], "10.0.0.50", "$test_name: the lookup still answers"); + ok(!exists $test_state->{macdb}->{macs}->{$mac}, + "$test_name: but leaves a changed cache alone"); } run_test(\&test_ipam_cache_lookup); -- 2.47.3