[PATCH pve-network v2 02/11] sdn: ipam: write the lookup's cache entry under the cluster lock

[Hannes Laimer <h.laimer@proxmox.com>]

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 <h.laimer@proxmox.com>
---
 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