[PATCH pve-network v4 04/20] sdn: ipam: stop caching IPAM misses

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

A lookup that missed the cache stored the IPAM's empty answer, and
any entry counted as a hit. So a record created later directly in the
IPAM stayed hidden, and a guest start allocated another address. An
empty answer is no longer stored, and an entry without an address, as
earlier misses left them, is dropped when the cache is read.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---

Notes:
    v4:
     * new, split out of the MAC cache patch

 src/PVE/Network/SDN/Ipams.pm        | 19 ++++++++---
 src/test/run_test_vnets_blackbox.pl | 52 ++++++++++++++++++++++++++++-
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Network/SDN/Ipams.pm b/src/PVE/Network/SDN/Ipams.pm
index 179bdf76..79591ea0 100644
--- a/src/PVE/Network/SDN/Ipams.pm
+++ b/src/PVE/Network/SDN/Ipams.pm
@@ -23,10 +23,17 @@ PVE::Network::SDN::Ipams::Plugin->init();
 my $macdb_filename = "sdn/mac-cache.json";
 cfs_register_file($macdb_filename, \&json_reader, \&json_writer);
 
+# an entry without an address was left behind by a lookup that missed, and
+# says nothing about its MAC
 sub json_reader {
     my ($filename, $data) = @_;
 
-    return defined($data) && length($data) > 0 ? decode_json($data) : {};
+    my $db = defined($data) && length($data) > 0 ? decode_json($data) : {};
+    for my $mac (keys %{ $db->{macs} // {} }) {
+        my $entry = $db->{macs}->{$mac};
+        delete $db->{macs}->{$mac} if !defined($entry->{ip4}) && !defined($entry->{ip6});
+    }
+    return $db;
 }
 
 sub json_writer {
@@ -140,12 +147,16 @@ sub get_ips_from_mac {
 
     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, 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);
 
-    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 9f4c424f..d8e35231 100755
--- a/src/test/run_test_vnets_blackbox.pl
+++ b/src/test/run_test_vnets_blackbox.pl
@@ -8,6 +8,7 @@ use File::Slurp;
 use List::Util qw(first all);
 use NetAddr::IP qw(:lower);
 
+use JSON;
 use Test::More;
 use Test::MockModule;
 use Test::Differences;
@@ -193,7 +194,7 @@ $mocked_sdn_ipams->mock(
         $test_state->{ipam_config} = $cfg;
     },
     read_macdb => sub {
-        return $test_state->{macdb};
+        return PVE::Network::SDN::Ipams::json_reader(undef, encode_json($test_state->{macdb}));
     },
     write_macdb => sub {
         my ($cfg) = @_;
@@ -421,6 +422,55 @@ sub test_without_subnet {
 }
 run_test(\&test_without_subnet);
 
+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_nic_join {
     my ($test_name, $subnets) = @_;
 
-- 
2.47.3