[PATCH pve-network v3 06/17] sdn: ipam: key the MAC cache by lower-case MAC, stop caching misses

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

Generated guest MACs are upper case, but the API accepts either
spelling. The cache was keyed by the MAC as its writer spelled it. A
deletion in the other spelling missed the entry, so the cache kept
answering with the deleted address. Entries cached before this
change keep their spelling. A lookup and a deletion still find them,
and the next write for their MAC moves them to the lower-case key.

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
counts as a miss.

The lookup also 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.

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 179bdf76..d8051d8d 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 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, whatever spelling its writers use
 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 from before the cache was keyed in lower case carry their writer's spelling
+            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 from before the cache was keyed in lower case carry their writer's
+    # spelling, and 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, so 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, since 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 d324538e..476e7f8d 100755
--- a/src/test/run_test_vnets_blackbox.pl
+++ b/src/test/run_test_vnets_blackbox.pl
@@ -1118,6 +1118,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 written under the config's spelling before the cache was keyed in
+    # lower case is read and released like any other
+    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 under another spelling 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