[PATCH pve-network 01/10] 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, no longer counts as a hit.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/PVE/Network/SDN/Ipams.pm        | 16 ++++++--
 src/test/run_test_vnets_blackbox.pl | 58 ++++++++++++++++++++++++++++-
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Network/SDN/Ipams.pm b/src/PVE/Network/SDN/Ipams.pm
index 179bdf76defc..7e7bec27b5ed 100644
--- a/src/PVE/Network/SDN/Ipams.pm
+++ b/src/PVE/Network/SDN/Ipams.pm
@@ -136,16 +136,24 @@ 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};
+    my $cached = $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})
+        if $cached && (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, 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 9f4c424f3d2f..9724f1a8e4ef 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) = @_;
@@ -321,6 +322,34 @@ sub create_subnet {
     PVE::API2::Network::SDN::Subnets->create($params);
 }
 
+# a DHCP zone with one vnet, each subnet with its first address as the
+# gateway and a range over its hosts 100 to 200
+sub create_dhcp_zone {
+    my ($zoneid, $vnetid, @cidrs) = @_;
+    create_zone({ type => "simple", dhcp => "dnsmasq", ipam => "pve", zone => $zoneid });
+    create_dhcp_vnet($zoneid, $vnetid, @cidrs);
+}
+
+sub create_dhcp_vnet {
+    my ($zoneid, $vnetid, @cidrs) = @_;
+    create_vnet({ type => "vnet", zone => $zoneid, vnet => $vnetid });
+    create_dhcp_subnet($vnetid, $_) for @cidrs;
+}
+
+sub create_dhcp_subnet {
+    my ($vnetid, $cidr) = @_;
+    my $net = NetAddr::IP->new($cidr);
+    my $addr = sub { my ($ip) = @_; return $ip->version() == 4 ? $ip->addr() : $ip->short(); };
+    create_subnet({
+        type => "subnet",
+        vnet => $vnetid,
+        subnet => $cidr,
+        gateway => $addr->($net->first()),
+        'dhcp-range' =>
+            ["start-address=" . $addr->($net + 100) . ",end-address=" . $addr->($net + 200)],
+    });
+}
+
 sub get_ipam_entries {
     return PVE::API2::Network::SDN::Ipams->ipamindex({ ipam => "pve" });
 }
@@ -421,6 +450,33 @@ sub test_without_subnet {
 }
 run_test(\&test_without_subnet);
 
+sub test_ipam_cache_lookup {
+    my $test_name = (split(/::/, (caller(0))[3]))[-1];
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+    my $mac = "da:65:8f:18:9b:6f";
+
+    create_dhcp_zone($zoneid, $vnetid, "10.0.0.0/24");
+
+    my $cache = decode_json(encode_json($test_state->{macdb}));
+    PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
+    is_deeply($test_state->{macdb}, $cache, "$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 };
+    my @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_lookup);
+
 sub test_nic_join {
     my ($test_name, $subnets) = @_;
 
-- 
2.47.3