[PATCH pve-network v5 02/13] sdn: ipam: cache a MAC lookup's answer under the cluster lock

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

On a cache miss the MAC lookup stored the IPAM's answer without the
cluster lock the other cache writers take. It wrote back the whole
cache as read before the request, so a change made in between was
lost.

The lookup now stores the answer under that lock, and only if the
cache has not changed in the meantime. Such a change may be newer
than the answer, so the answer is then returned without being stored.

The IPAM is still asked outside the lock, since holding the lock
during a request to an external IPAM would make every cache write in
the cluster wait for the IPAM's answer.

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 37eb5567eae6..927ab16adba9 100755
--- a/src/test/run_test_vnets_blackbox.pl
+++ b/src/test/run_test_vnets_blackbox.pl
@@ -487,6 +487,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