[PATCH pve-network v2 05/16] sdn: ipam: do not cache negative per-MAC answers, lock the write

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

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