[PATCH pve-network 08/10] sdn: ipam: keep the MAC cache per vnet

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

The same MAC can sit on several vnets, but the cache of its addresses
was keyed by the MAC alone. A mapping created or edited on one vnet
changed the cached answer on every other vnet as well. A guest with
that MAC on another vnet then got a reservation for an address
outside its own vnet.

The cache now holds the addresses per vnet, and a zone's dnsmasq
reservations hold a line for each vnet a MAC sits on, each written
from the cache of that vnet. A MAC matches in any spelling.

Entries from before, keyed by the MAC alone, stay as written, since
nodes not yet upgraded still use them. They answer for a vnet only
with addresses in its subnets, so a running guest keeps its
reservation across the upgrade. A node not yet upgraded does not
update the per-vnet entries, so a release checks that the record
still belongs to its MAC and leaves it alone otherwise. The API
refuses to delete such a mapping.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/PVE/API2/Network/SDN/Ips.pm            |   5 +-
 src/PVE/Network/SDN/Dhcp.pm                |   2 +-
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm        | 125 ++++++------
 src/PVE/Network/SDN/Dhcp/Plugin.pm         |   6 +-
 src/PVE/Network/SDN/Ipams.pm               | 124 ++++++------
 src/PVE/Network/SDN/Ipams/NetboxPlugin.pm  |  11 +-
 src/PVE/Network/SDN/Ipams/PVEPlugin.pm     |  13 +-
 src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm |  16 +-
 src/PVE/Network/SDN/Ipams/Plugin.pm        |   4 +-
 src/PVE/Network/SDN/Subnets.pm             |  11 +-
 src/PVE/Network/SDN/Vnets.pm               |  12 +-
 src/test/run_test_ipams.pl                 |  37 ++++
 src/test/run_test_vnets_blackbox.pl        | 217 +++++++++++++++++----
 13 files changed, 404 insertions(+), 179 deletions(-)

diff --git a/src/PVE/API2/Network/SDN/Ips.pm b/src/PVE/API2/Network/SDN/Ips.pm
index b1faee71c6ba..ecab7fc99b6c 100644
--- a/src/PVE/API2/Network/SDN/Ips.pm
+++ b/src/PVE/API2/Network/SDN/Ips.pm
@@ -62,11 +62,14 @@ __PACKAGE__->register_method({
         my $mac = extract_param($param, 'mac');
         my $ip = extract_param($param, 'ip');
 
-        eval { PVE::Network::SDN::Vnets::del_ip($vnet, $ip, '', $mac); };
+        my $released = eval { PVE::Network::SDN::Vnets::del_ip($vnet, $ip, '', $mac); };
         die "$@\n" if $@;
 
         PVE::Network::SDN::Dhcp::update_mapping($vnet, $mac);
 
+        # the cache entry is cleared, the IPAM record belongs to another MAC
+        die "IP '$ip' is not mapped to $mac\n" if defined($released) && !$released;
+
         return undef;
     },
 });
diff --git a/src/PVE/Network/SDN/Dhcp.pm b/src/PVE/Network/SDN/Dhcp.pm
index 4650270df09f..28e83d11630b 100644
--- a/src/PVE/Network/SDN/Dhcp.pm
+++ b/src/PVE/Network/SDN/Dhcp.pm
@@ -41,7 +41,7 @@ sub update_mapping {
         return if defined($zone->{nodes}) && !$zone->{nodes}->{ PVE::INotify::nodename() };
 
         PVE::Network::SDN::Dhcp::Plugin->lookup($zone->{dhcp})
-            ->update_ip_mapping($zoneid, $mac);
+            ->update_ip_mapping($zoneid, $vnetid, $mac);
     };
     # the records are written, so a failed update only warns
     warn "could not update dhcp mapping for $mac: $@" if $@;
diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index 4767fb5162ad..1acc7c42318b 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -13,7 +13,6 @@ use Net::DBus;
 
 use PVE::Cluster;
 use PVE::RESTEnvironment qw(log_warn);
-use PVE::Network::SDN::Ipams;
 
 my $DNSMASQ_CONFIG_ROOT = '/etc/dnsmasq.d';
 my $DNSMASQ_DEFAULT_ROOT = '/etc/default';
@@ -54,98 +53,90 @@ sub update_lease {
 }
 
 sub update_ip_mapping {
-    my ($class, $dhcpid, $mac) = @_;
+    my ($class, $dhcpid, $vnetid, $mac) = @_;
     $mac = lc($mac);
 
     my $ethers_file = ethers_file($dhcpid);
     my $ethers_tmp_file = "$ethers_file.tmp";
 
-    my ($ip4, $ip6);
-    my $reservation = undef;
+    my $ip4;
     my $changed = undef;
 
     my $rewriteFn = sub {
-        # the lines are decided from the cache, which this process may hold an
-        # old read of, and the local lock does not refresh it
+        # the lines are decided from the cache and the applied config, which
+        # the local lock does not refresh
         PVE::Cluster::cfs_update();
 
-        # read under the ethers lock. A snapshot from before it would sweep a
-        # line another push wrote meanwhile, and an older push would put its
-        # addresses back
-
-        my $macs = PVE::Network::SDN::Ipams::read_macdb()->{macs};
-
-        my $entry = $macs->{$mac};
-        ($ip4, $ip6) = ($entry->{ip4}, $entry->{ip6}) if $entry;
-        if ($ip4 || $ip6) {
-            $reservation = $mac;
-            $reservation .= ",$ip4" if $ip4;
-            $reservation .= ",[$ip6]" if $ip6;
+        # the file holds a line per vnet a MAC sits on, and a line belongs to
+        # the vnet of the applied subnet that holds its address
+        my $vnets = PVE::Network::SDN::Zones::get_vnets($dhcpid, 1) // {};
+        my $subnets = PVE::Network::SDN::Vnets::get_subnets(undef, 1) // {};
+        $subnets = {
+            map { $_ => $subnets->{$_} } grep { $vnets->{ $subnets->{$_}->{vnet} } }
+                keys %$subnets
+        };
+        my $vnet_of = sub {
+            my ($ip) = @_;
+            my (undef, $subnet) =
+                eval { PVE::Network::SDN::Subnets::find_ip_subnet($ip, $subnets) };
+            return $subnet ? $subnet->{vnet} : undef;
+        };
+
+        # read under the ethers lock, a read from before it would undo a push
+        # that ran meanwhile. The lines of other MACs are checked against the
+        # cache alone, so under the lock only the changed MAC may ask the IPAM
+        my $reservation = sub {
+            my ($vnet, $line_mac, $cached_only) = @_;
+            my ($line_ip4, $line_ip6) =
+                PVE::Network::SDN::Vnets::get_ips_from_mac($vnet, $line_mac, 1, $cached_only);
+            return (undef, undef) if !$line_ip4 && !$line_ip6;
+            my $line = $line_mac;
+            $line .= ",$line_ip4" if $line_ip4;
+            $line .= ",[$line_ip6]" if $line_ip6;
             # an external IPAM spells addresses its own way, while the file holds one spelling
-            $reservation = lc($reservation);
-        }
+            return (lc($line), $line_ip4);
+        };
 
         file_set_contents($ethers_file, '') if !-e $ethers_file;
         open(my $in, '<', $ethers_file) or die "Could not open file '$ethers_file' $!\n";
         open(my $out, '>', $ethers_tmp_file)
             or die "Could not open file '$ethers_tmp_file' $!\n";
 
-        my $kept = undef;
-
+        my %written;
         while (my $line = <$in>) {
             chomp($line);
             next if $line !~ m/\S/;
-            my ($parsed_mac, $parsed_ip1, $parsed_ip2) = split(/,/, $line);
-            # a line from before the file was written in lower case keeps its writer's spelling
+            my ($parsed_mac, $parsed_ip) = split(/,/, $line);
             $parsed_mac = lc($parsed_mac);
-
-            # the MAC's own reservation is rewritten. An unchanged one stays in place
-            if ($parsed_mac eq $mac) {
-                if (defined($reservation) && !$kept && lc($line) eq $reservation) {
-                    $kept = 1;
-                    print $out "$line\n";
-                } else {
-                    $changed = 1;
-                }
-                next;
-            }
-
-            my ($parsed_ip4, $parsed_ip6);
-            if ($parsed_ip2) {
-                $parsed_ip4 = $parsed_ip1;
-                $parsed_ip6 = $parsed_ip2;
-            } elsif (defined($parsed_ip1) && Net::IP::ip_is_ipv4($parsed_ip1)) {
-                $parsed_ip4 = $parsed_ip1;
-            } elsif (defined($parsed_ip1)) {
-                $parsed_ip6 = $parsed_ip1;
-            }
-            $parsed_ip6 = $1 if $parsed_ip6 && $parsed_ip6 =~ m/\[(\S+)\]/;
-            # a line from before addresses were lower-cased may spell one in upper case
-            $parsed_ip6 = lc($parsed_ip6) if $parsed_ip6;
-
-            # a line that no longer matches the cache is replaced by the cache's
-            # current one. Dropping it left that guest unknown to dnsmasq until
-            # its next start
-            my $current = $macs->{$parsed_mac};
-            if (
-                !$current
-                || ($parsed_ip4 && $current->{ip4} && $current->{ip4} ne $parsed_ip4)
-                || ($parsed_ip6 && $current->{ip6} && lc($current->{ip6}) ne $parsed_ip6)
-            ) {
+            $parsed_ip = $1 if defined($parsed_ip) && $parsed_ip =~ m/\[(\S+)\]/;
+            my $vnet = defined($parsed_ip) ? $vnet_of->($parsed_ip) : undef;
+            my $key = defined($vnet) ? "$parsed_mac,$vnet" : undef;
+
+            # a line that no longer matches is replaced, since dropping it left
+            # that guest unknown to dnsmasq until its next start
+            my ($want) =
+                eval { $key && !$written{$key} ? $reservation->($vnet, $parsed_mac, 1) : () };
+            if ($@) {
+                warn "could not read the reservation of $parsed_mac: $@";
+                print $out "$line\n";
+            } elsif (!defined($want)) {
                 $changed = 1;
-                next if !$current;
-                my $replacement = $parsed_mac;
-                $replacement .= ",$current->{ip4}" if $current->{ip4};
-                $replacement .= ",[$current->{ip6}]" if $current->{ip6};
-                print $out lc($replacement) . "\n";
                 next;
+            } elsif (lc($line) ne $want) {
+                $changed = 1;
+                print $out "$want\n";
+            } else {
+                print $out "$line\n";
             }
-
-            print $out "$line\n";
+            $written{$key}++;
         }
 
-        if (defined($reservation) && !$kept) {
-            print $out "$reservation\n";
+        # the changed MAC's own line may have been dropped above, since only
+        # the cache was asked for it, so it is written from the IPAM's answer
+        my $want;
+        ($want, $ip4) = $reservation->($vnetid, $mac);
+        if (defined($want) && !$written{"$mac,$vnetid"}) {
+            print $out "$want\n";
             $changed = 1;
         }
 
diff --git a/src/PVE/Network/SDN/Dhcp/Plugin.pm b/src/PVE/Network/SDN/Dhcp/Plugin.pm
index a3a5ae24403f..3484df19b6dc 100644
--- a/src/PVE/Network/SDN/Dhcp/Plugin.pm
+++ b/src/PVE/Network/SDN/Dhcp/Plugin.pm
@@ -22,10 +22,10 @@ sub private {
     return $defaultData;
 }
 
-# the MAC's records changed, so make the backend serve the current ones. With no
-# record left its reservation goes
+# the MAC's records on the vnet changed, so make the backend serve the current
+# ones. With no record left its reservation on the vnet goes
 sub update_ip_mapping {
-    my ($class, $dhcpid, $mac) = @_;
+    my ($class, $dhcpid, $vnetid, $mac) = @_;
     die 'implement in sub class';
 }
 
diff --git a/src/PVE/Network/SDN/Ipams.pm b/src/PVE/Network/SDN/Ipams.pm
index 9a31eaf39f58..5b2217c45242 100644
--- a/src/PVE/Network/SDN/Ipams.pm
+++ b/src/PVE/Network/SDN/Ipams.pm
@@ -35,40 +35,10 @@ sub json_writer {
     return encode_json($data);
 }
 
-# Writers spell a MAC their own way, the generated ones in upper case and the
-# API in either, and nodes not yet upgraded look an entry up under the spelling
-# their caller hands them, so the file keeps its keys as written. Readers get a
-# view with every MAC in lower case. Two spellings that disagree on an address
-# are left out of it, since one of them is stale and nothing here says which,
-# and so is an entry without an address. The next lookup then asks the IPAM
-my sub fold_macdb {
-    my ($db) = @_;
-
-    my $macs = {};
-    my %clash;
-    for my $key (sort keys %{ $db->{macs} // {} }) {
-        my $entry = $db->{macs}->{$key};
-        for my $family (grep { defined($entry->{$_}) } qw(ip4 ip6)) {
-            my $seen = $macs->{ lc($key) }->{$family};
-            $clash{ lc($key) } = 1 if defined($seen) && lc($seen) ne lc($entry->{$family});
-            $macs->{ lc($key) }->{$family} //= $entry->{$family};
-        }
-    }
-    delete $macs->{$_} for keys %clash;
-    return { %$db, macs => $macs };
-}
-
-# the file as written, which the writers change and write back
-sub read_macdb_file {
-    my () = @_;
-
-    return cfs_read_file($macdb_filename);
-}
-
 sub read_macdb {
     my () = @_;
 
-    return fold_macdb(read_macdb_file());
+    return cfs_read_file($macdb_filename);
 }
 
 sub write_macdb {
@@ -77,25 +47,56 @@ sub write_macdb {
     cfs_write_file($macdb_filename, $data);
 }
 
-# a write goes to every spelling the file holds for the MAC, so each reader
-# finds it under its own, and a MAC new to the file is written in lower case
-my sub set_cache_entry {
-    my ($db, $mac, $family, $ip) = @_;
+# the map under the MAC alone keeps its keys as written, since nodes not yet
+# upgraded still use it under the MAC as their caller spelled it. Spellings
+# that disagree on an address are both left out, since one of them is stale
+my sub old_cache_entry {
+    my ($db, $mac, $subnets) = @_;
 
-    my $macs = $db->{macs} //= {};
-    my @keys = grep { lc($_) eq lc($mac) } keys %$macs;
-    $macs->{$_}->{$family} = $ip for (@keys ? @keys : (lc($mac)));
+    my $entry = {};
+    my $macs = $db->{macs} // {};
+    for my $key (grep { lc($_) eq lc($mac) } sort keys %$macs) {
+        for my $family (qw(ip4 ip6)) {
+            my $ip = $macs->{$key}->{$family} // next;
+            return undef if defined($entry->{$family}) && lc($entry->{$family}) ne lc($ip);
+            $entry->{$family} = $ip;
+        }
+    }
+    for my $family (keys %$entry) {
+        delete $entry->{$family}
+            if !eval { PVE::Network::SDN::Subnets::find_ip_subnet($entry->{$family}, $subnets) };
+    }
+    return %$entry ? $entry : undef;
+}
+
+# an entry from before the cache was kept per vnet fills the families a
+# vnet's entry does not have. Where the two disagree, a node still writing
+# one map may have moved the address since, so the lookup is a miss
+my sub cache_entry {
+    my ($db, $mac, $vnetid, $subnets) = @_;
+
+    my $entry = (($db->{vnets} // {})->{$vnetid} // {})->{ lc($mac) };
+    $entry = undef if $entry && !defined($entry->{ip4}) && !defined($entry->{ip6});
+    my $old = old_cache_entry($db, $mac, $subnets);
+    return undef if !$entry && !$old;
+    for my $family (qw(ip4 ip6)) {
+        my ($ip, $old_ip) = ($entry->{$family}, $old->{$family});
+        return undef if defined($ip) && defined($old_ip) && lc($ip) ne lc($old_ip);
+    }
+    return { map { $_ => $entry->{$_} // $old->{$_} } qw(ip4 ip6) };
 }
 
 sub add_cache_mac_ip {
-    my ($mac, $ip) = @_;
+    my ($vnetid, $mac, $ip) = @_;
+    $mac = lc($mac);
+    my $family = Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6';
 
     cfs_lock_file(
         $macdb_filename,
         undef,
         sub {
-            my $db = read_macdb_file();
-            set_cache_entry($db, $mac, Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6', $ip);
+            my $db = read_macdb();
+            $db->{vnets}->{$vnetid}->{$mac}->{$family} = $ip;
             write_macdb($db);
         },
     );
@@ -103,19 +104,29 @@ sub add_cache_mac_ip {
 }
 
 sub del_cache_mac_ip {
-    my ($mac, $ip) = @_;
+    my ($vnetid, $mac, $ip) = @_;
+    $mac = lc($mac);
+    my $family = Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6';
 
     cfs_lock_file(
         $macdb_filename,
         undef,
         sub {
-            my $db = read_macdb_file();
-            my $family = Net::IP::ip_is_ipv4($ip) ? 'ip4' : 'ip6';
+            my $db = read_macdb();
+            my $vnet_entries = ($db->{vnets} // {})->{$vnetid};
+            if (my $entry = $vnet_entries ? $vnet_entries->{$mac} : undef) {
+                delete $entry->{$family};
+                delete $vnet_entries->{$mac} if !%$entry;
+                delete $db->{vnets}->{$vnetid} if !%$vnet_entries;
+                delete $db->{vnets} if !%{ $db->{vnets} };
+            }
+            # the old map must not hand a released address back
             my $macs = $db->{macs} // {};
-            for my $key (grep { lc($_) eq lc($mac) } keys %$macs) {
-                delete $macs->{$key}->{$family};
-                delete $macs->{$key}
-                    if !defined($macs->{$key}->{ip4}) && !defined($macs->{$key}->{ip6});
+            for my $key (grep { lc($_) eq $mac } keys %$macs) {
+                my $old = $macs->{$key};
+                next if lc($old->{$family} // '') ne lc($ip);
+                delete $old->{$family};
+                delete $macs->{$key} if !defined($old->{ip4}) && !defined($old->{ip6});
             }
             write_macdb($db);
         },
@@ -169,15 +180,13 @@ sub complete_sdn_vnet {
 }
 
 sub get_ips_from_mac {
-    my ($mac, $zoneid, $zone, $subnets) = @_;
+    my ($vnetid, $mac, $zoneid, $zone, $subnets, $cached_only) = @_;
 
     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}->{ lc($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 $cached = cache_entry(read_macdb(), $mac, $vnetid, $subnets);
+    return ($cached->{ip4}, $cached->{ip6}) if $cached;
+    return if $cached_only;
 
     my $plugin_config = get_plugin_config($zone);
     my $plugin = PVE::Network::SDN::Ipams::Plugin->lookup($plugin_config->{type});
@@ -195,9 +204,10 @@ sub get_ips_from_mac {
             return
                 if (PVE::Cluster::cfs_file_version($macdb_filename) // '') ne ($version // '');
 
-            my $db = read_macdb_file();
-            set_cache_entry($db, $mac, 'ip4', $ip4) if defined($ip4);
-            set_cache_entry($db, $mac, 'ip6', $ip6) if defined($ip6);
+            my $db = read_macdb();
+            my $entry = $db->{vnets}->{$vnetid}->{ lc($mac) } //= {};
+            $entry->{ip4} = $ip4 if defined($ip4);
+            $entry->{ip6} = $ip6 if defined($ip6);
             write_macdb($db);
         },
     );
diff --git a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
index 01e7f347d796..82c9e80b6c6e 100644
--- a/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/NetboxPlugin.pm
@@ -360,7 +360,7 @@ sub add_range_next_freeip {
 }
 
 sub del_ip {
-    my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr) = @_;
+    my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr, $mac) = @_;
 
     return if !$ip;
 
@@ -369,6 +369,15 @@ sub del_ip {
         warn "could not find id for ip $ip";
         return;
     }
+    if ($mac) {
+        my $record =
+            eval { netbox_api_request($plugin_config, "GET", "/ipam/ip-addresses/$ip_id/") };
+        my ($owner) = (($record // {})->{description} // '') =~ m/^mac:(\S+)$/;
+        if ($owner && lc($owner) ne lc($mac)) {
+            warn "not releasing IP '$ip' for $mac, it is held by $owner\n";
+            return 0;
+        }
+    }
 
     eval { netbox_api_request($plugin_config, "DELETE", "/ipam/ip-addresses/$ip_id/"); };
     if ($@) {
diff --git a/src/PVE/Network/SDN/Ipams/PVEPlugin.pm b/src/PVE/Network/SDN/Ipams/PVEPlugin.pm
index 43d0a3daaf9a..86e594a4c604 100644
--- a/src/PVE/Network/SDN/Ipams/PVEPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/PVEPlugin.pm
@@ -249,11 +249,12 @@ sub add_range_next_freeip {
 }
 
 sub del_ip {
-    my ($class, $plugin_config, $subnetid, $subnet, $ip) = @_;
+    my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr, $mac) = @_;
 
     my $cidr = $subnet->{cidr};
     my $zone = $subnet->{zone};
 
+    my $released = 1;
     cfs_lock_file(
         $ipamdb_file,
         undef,
@@ -271,11 +272,18 @@ sub del_ip {
                 warn "IP '$ip' does not exist in IPAM DB\n";
                 return;
             }
+            my $owner = $dbsubnet->{ips}->{$ip}->{mac};
+            if ($mac && $owner && lc($owner) ne lc($mac)) {
+                warn "not releasing IP '$ip' for $mac, it is held by $owner\n";
+                $released = 0;
+                return;
+            }
             delete $dbsubnet->{ips}->{$ip};
             write_db($db);
         },
     );
     die "$@" if $@;
+    return $released;
 }
 
 sub get_ips_from_mac {
@@ -297,7 +305,8 @@ sub get_ips_from_mac {
         my $ips = $dbzone->{subnets}->{$subnet}->{ips} // next;
         for my $ip (keys %$ips) {
             my $ipobject = $ips->{$ip};
-            if ($ipobject->{mac} && $ipobject->{mac} eq $mac) {
+            # a record keeps the MAC as its writer spelled it
+            if ($ipobject->{mac} && lc($ipobject->{mac}) eq lc($mac)) {
                 if (Net::IP::ip_is_ipv4($ip)) {
                     $ip4 = $ip;
                 } else {
diff --git a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
index c3f014d45585..e1b30e293ab3 100644
--- a/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
+++ b/src/PVE/Network/SDN/Ipams/PhpIpamPlugin.pm
@@ -237,7 +237,7 @@ sub add_range_next_freeip {
 }
 
 sub del_ip {
-    my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr) = @_;
+    my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr, $mac) = @_;
 
     return if !$ip;
 
@@ -248,6 +248,18 @@ sub del_ip {
 
     my $ip_id = get_ip_id($url, $ip, $headers);
     return if !$ip_id;
+    if ($mac) {
+        my $record = eval {
+            PVE::Network::SDN::api_request(
+                "GET", "$url/addresses/$ip_id/", $headers, undef, $fingerprint,
+            );
+        };
+        my $owner = (($record // {})->{data} // {})->{mac};
+        if ($owner && lc($owner) ne lc($mac)) {
+            warn "not releasing IP '$ip' for $mac, it is held by $owner\n";
+            return 0;
+        }
+    }
 
     eval {
         PVE::Network::SDN::api_request(
@@ -257,6 +269,8 @@ sub del_ip {
     if ($@) {
         die "error delete ip $ip: $@" if !$noerr;
     }
+
+    return 1;
 }
 
 sub get_ips_from_mac {
diff --git a/src/PVE/Network/SDN/Ipams/Plugin.pm b/src/PVE/Network/SDN/Ipams/Plugin.pm
index 14f8fe9e67fe..6e23d5434d4b 100644
--- a/src/PVE/Network/SDN/Ipams/Plugin.pm
+++ b/src/PVE/Network/SDN/Ipams/Plugin.pm
@@ -132,8 +132,10 @@ sub add_range_next_freeip {
     die "please implement inside plugin";
 }
 
+# a MAC names the guest the address is released for, a record holding the
+# address for another MAC is then left alone and 0 returned
 sub del_ip {
-    my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr) = @_;
+    my ($class, $plugin_config, $subnetid, $subnet, $ip, $noerr, $mac) = @_;
 
     die "please implement inside plugin";
 }
diff --git a/src/PVE/Network/SDN/Subnets.pm b/src/PVE/Network/SDN/Subnets.pm
index 1f27fac45db1..4d431f1bd27a 100644
--- a/src/PVE/Network/SDN/Subnets.pm
+++ b/src/PVE/Network/SDN/Subnets.pm
@@ -263,7 +263,7 @@ sub add_next_free_ip {
 
         die $@ if $@;
 
-        eval { PVE::Network::SDN::Ipams::add_cache_mac_ip($mac, $ip); };
+        eval { PVE::Network::SDN::Ipams::add_cache_mac_ip($subnet->{vnet}, $mac, $ip); };
         warn $@ if $@;
     }
 
@@ -322,7 +322,7 @@ sub add_ip {
         };
         die $@ if $@;
 
-        eval { PVE::Network::SDN::Ipams::add_cache_mac_ip($mac, $ip) if $mac; };
+        eval { PVE::Network::SDN::Ipams::add_cache_mac_ip($subnet->{vnet}, $mac, $ip) if $mac; };
         warn $@ if $@;
     }
 
@@ -410,14 +410,15 @@ sub del_ip {
         verify_dns_zone($reversednszone, $reversedns);
     }
 
+    my $released;
     if ($ipamid) {
         my $ipam_cfg = PVE::Network::SDN::Ipams::config();
         my $plugin_config = $ipam_cfg->{ids}->{$ipamid};
         my $plugin = PVE::Network::SDN::Ipams::Plugin->lookup($plugin_config->{type});
-        $plugin->del_ip($plugin_config, $subnetid, $subnet, $ip);
+        $released = $plugin->del_ip($plugin_config, $subnetid, $subnet, $ip, undef, $mac);
 
         if ($mac) {
-            eval { PVE::Network::SDN::Ipams::del_cache_mac_ip($mac, $ip) };
+            eval { PVE::Network::SDN::Ipams::del_cache_mac_ip($subnet->{vnet}, $mac, $ip) };
             warn $@ if $@;
         }
     }
@@ -431,6 +432,8 @@ sub del_ip {
     if ($@) {
         warn $@;
     }
+
+    return $released;
 }
 
 1;
diff --git a/src/PVE/Network/SDN/Vnets.pm b/src/PVE/Network/SDN/Vnets.pm
index b1a2192ac767..d8824218d9ef 100644
--- a/src/PVE/Network/SDN/Vnets.pm
+++ b/src/PVE/Network/SDN/Vnets.pm
@@ -191,18 +191,20 @@ sub del_ip {
 }
 
 sub get_ips_from_mac {
-    my ($vnetid, $mac) = @_;
+    my ($vnetid, $mac, $running, $cached_only) = @_;
 
-    my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid);
+    my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid, $running);
     return if !$vnet;
 
     my $zoneid = $vnet->{zone};
-    my $zone = PVE::Network::SDN::Zones::get_zone($zoneid);
+    my $zone = PVE::Network::SDN::Zones::get_zone($zoneid, $running);
 
     return if !$zone->{ipam} || !$zone->{dhcp};
 
-    my $subnets = PVE::Network::SDN::Vnets::get_subnets($vnetid);
-    return PVE::Network::SDN::Ipams::get_ips_from_mac($mac, $zoneid, $zone, $subnets);
+    my $subnets = PVE::Network::SDN::Vnets::get_subnets($vnetid, $running);
+    return PVE::Network::SDN::Ipams::get_ips_from_mac(
+        $vnetid, $mac, $zoneid, $zone, $subnets, $cached_only,
+    );
 }
 
 sub del_ips_from_mac {
diff --git a/src/test/run_test_ipams.pl b/src/test/run_test_ipams.pl
index f9c44fe1a3b9..0d3fed120d3f 100755
--- a/src/test/run_test_ipams.pl
+++ b/src/test/run_test_ipams.pl
@@ -219,5 +219,42 @@ foreach my $path (@plugins) {
 
 }
 
+# an address released for a MAC stays when the external IPAM records it
+# for another MAC
+{
+    my $subnet = { zone => 'z', cidr => '10.0.0.0/24' };
+    my $owner;
+    my @deletes;
+    my $sdn_module = Test::MockModule->new("PVE::Network::SDN");
+    $sdn_module->mock(
+        api_request => sub {
+            my ($method, $url) = @_;
+            push @deletes, $url if $method eq 'DELETE';
+            return {
+                description => "mac:$owner",
+                data => { mac => $owner },
+            };
+        },
+    );
+    my $netbox = Test::MockModule->new('PVE::Network::SDN::Ipams::NetboxPlugin');
+    $netbox->mock(subnet_prefix => sub { return (1, 2); }, get_ip_id => sub { return 3; });
+    my $phpipam = Test::MockModule->new('PVE::Network::SDN::Ipams::PhpIpamPlugin');
+    $phpipam->mock(subnet_id => sub { return 7; }, get_ip_id => sub { return 3; });
+
+    for my $plugin (
+        'PVE::Network::SDN::Ipams::NetboxPlugin',
+        'PVE::Network::SDN::Ipams::PhpIpamPlugin',
+    ) {
+        my $config = { url => 'u', token => 't' };
+        @deletes = ();
+        $owner = 'da:65:8f:18:9b:70';
+        $plugin->del_ip($config, 'z-10.0.0.0-24', $subnet, '10.0.0.50', 1, 'DA:65:8F:18:9B:6F');
+        is(scalar(@deletes), 0, "$plugin keeps the record of another MAC");
+        $owner = 'da:65:8f:18:9b:6f';
+        $plugin->del_ip($config, 'z-10.0.0.0-24', $subnet, '10.0.0.50', 1, 'DA:65:8F:18:9B:6F');
+        is(scalar(@deletes), 1, "$plugin releases the MAC's own record");
+    }
+}
+
 done_testing();
 
diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl
index 27b1295ed556..8f703cec9968 100755
--- a/src/test/run_test_vnets_blackbox.pl
+++ b/src/test/run_test_vnets_blackbox.pl
@@ -194,7 +194,7 @@ $mocked_sdn_ipams->mock(
         my ($cfg) = @_;
         $test_state->{ipam_config} = $cfg;
     },
-    read_macdb_file => sub {
+    read_macdb => sub {
         return PVE::Network::SDN::Ipams::json_reader(undef, encode_json($test_state->{macdb}));
     },
     write_macdb => sub {
@@ -491,74 +491,97 @@ sub test_ipam_cache_lookup {
     # 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 };
+    $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",
+    is_deeply(
+        $test_state->{macdb},
+        {
+            macs => { $mac => { ip4 => undef, ip6 => undef } },
+            vnets => { $vnetid => { $mac => { ip4 => "10.0.0.50" } } },
+        },
+        "$test_name: the answer is cached, the old entry left as written",
     );
 
     # 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};
+    delete $test_state->{macdb}->{vnets};
     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");
+    ok(!exists $test_state->{macdb}->{vnets}, "$test_name: but leaves a changed cache alone");
 
-    # an entry written under the config's spelling, as a node not yet upgraded
-    # writes it, is read and released like any other and keeps its key
+    # an entry written under the config's spelling before the cache was keyed in
+    # lower case is read and released like any other
     my $other = "da:65:8f:18:9b:70";
     $test_state->{macdb}->{macs} = {
         uc($mac) => { ip4 => '10.0.0.150' },
         uc($other) => { ip4 => '10.0.0.170' },
     };
     my ($ip4) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
-    is($ip4, '10.0.0.150', "$test_name: an entry is found however it is spelled");
-    PVE::Network::SDN::Ipams::del_cache_mac_ip($mac, '10.0.0.150');
+    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($vnetid, $mac, '10.0.0.150');
     is_deeply(
         $test_state->{macdb}->{macs},
         { uc($other) => { ip4 => '10.0.0.170' } },
         "$test_name: and released, the other entry left as written",
     );
 
-    # a write in another spelling goes to the spelling the file holds
+    # a write by this code leaves an old entry as a node not yet upgraded
+    # wrote it, so that node still finds it under its own spelling
     $test_state->{macdb}->{macs} = { uc($mac) => { ip4 => '10.0.0.150' } };
-    PVE::Network::SDN::Ipams::add_cache_mac_ip($mac, 'fd00::150');
+    PVE::Network::SDN::Ipams::add_cache_mac_ip($vnetid, uc($mac), 'fd00::150');
     is_deeply(
         $test_state->{macdb}->{macs},
-        { uc($mac) => { ip4 => '10.0.0.150', ip6 => 'fd00::150' } },
-        "$test_name: a write keeps the key as written",
+        { uc($mac) => { ip4 => '10.0.0.150' } },
+        "$test_name: a write leaves the old entry as written",
+    );
+    is_deeply(
+        [PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac)],
+        ['10.0.0.150', 'fd00::150'],
+        "$test_name: holding both addresses",
     );
 
-    # a lookup past an address-less entry asks the IPAM and fills the entry
-    # under its spelling
-    $test_state->{macdb}->{macs} = { uc($mac) => { ip4 => undef, ip6 => undef } };
+    # a vnet's entry and an old one that disagree are not trusted either, a
+    # node not yet upgraded may have moved the address since
+    $test_state->{macdb} = {
+        macs => { $mac => { ip4 => '10.0.0.151' } },
+        vnets => { $vnetid => { $mac => { ip4 => '10.0.0.150', ip6 => 'fd00::150' } } },
+    };
     @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
-    is($ips[0], '10.0.0.50', "$test_name: a lookup past an address-less entry asks the IPAM");
+    is(
+        $ips[0],
+        '10.0.0.50',
+        "$test_name: an old entry disagreeing with the vnet's counts as a miss",
+    );
     is_deeply(
-        $test_state->{macdb}->{macs},
-        { uc($mac) => { ip4 => '10.0.0.50', ip6 => undef } },
-        "$test_name: and fills the entry under its spelling",
+        $test_state->{macdb}->{vnets}->{$vnetid}->{$mac},
+        { ip4 => '10.0.0.50', ip6 => 'fd00::150' },
+        "$test_name: and the answer replaces the vnet's entry",
     );
 
     # two spellings that disagree are not trusted, the lookup asks the IPAM
-    $test_state->{macdb}->{macs} = {
-        uc($mac) => { ip4 => '10.0.0.150' },
-        $mac => { ip4 => '10.0.0.151' },
+    $test_state->{macdb} = {
+        macs => {
+            uc($mac) => { ip4 => '10.0.0.150' },
+            $mac => { ip4 => '10.0.0.151' },
+        },
     };
     @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
     is($ips[0], '10.0.0.50', "$test_name: a clash of spellings counts as a miss");
-    is_deeply(
-        $test_state->{macdb}->{macs},
-        { uc($mac) => { ip4 => '10.0.0.50' }, $mac => { ip4 => '10.0.0.50' } },
-        "$test_name: and its write brings both spellings in line",
-    );
+
+    # an old entry's address outside the vnet's subnets is another vnet's
+    $test_state->{macdb} = { macs => { uc($mac) => { ip4 => '10.9.0.150' } } };
+    @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
+    is($ips[0], '10.0.0.50', "$test_name: an old address of another vnet counts as a miss");
+
+    # the record keeps the MAC as its writer spelled it, a lookup in another
+    # spelling finds it
+    $test_state->{macdb} = {};
+    @ips = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, uc($mac));
+    is($ips[0], '10.0.0.50', "$test_name: a lookup in another spelling finds the record");
 
     # a mapping whose record is gone is still deleted, and its cache entry
     # with it
@@ -1128,15 +1151,18 @@ sub test_dnsmasq_stale_reservation_replaced {
     my $stale = "da:65:8f:18:9b:01";
     my $gone = "da:65:8f:18:9b:02";
     my $added = "da:65:8f:18:9b:03";
+    create_dhcp_zone("TESTZONE", "testvnet", "10.0.1.0/24");
     $test_state->{macdb} = {
-        macs => {
-            $stale => { ip4 => '10.0.1.11' },
-            $added => { ip4 => '10.0.1.13' },
+        vnets => {
+            testvnet => {
+                $stale => { ip4 => '10.0.1.11' },
+                $added => { ip4 => '10.0.1.13' },
+            },
         },
     };
     PVE::Tools::file_set_contents($TMP_ETHERS_FILE, "$stale,10.0.1.10\n$gone,10.0.1.12\n");
 
-    PVE::Network::SDN::Dhcp::Dnsmasq->update_ip_mapping('TESTZONE', $added);
+    PVE::Network::SDN::Dhcp::Dnsmasq->update_ip_mapping('TESTZONE', 'testvnet', $added);
 
     is(
         PVE::Tools::file_get_contents($TMP_ETHERS_FILE),
@@ -1297,8 +1323,127 @@ sub test_ipam_per_vnet {
             "$test_name: vnet$n answers with its own address",
         );
     }
+
+    # a mapping edit on one vnet leaves the other's answer alone
+    PVE::API2::Network::SDN::Ips->ipupdate(
+        { zone => $zoneid, vnet => 'vnet1', mac => $mac, ip => '10.1.0.51' });
+    is_deeply(
+        [PVE::Network::SDN::Vnets::get_ips_from_mac('vnet2', $mac)],
+        ['10.2.0.50', undef],
+        "$test_name: an edit on the first vnet leaves the second's answer alone",
+    );
+    is_deeply(
+        [PVE::Network::SDN::Vnets::get_ips_from_mac('vnet1', $mac)],
+        ['10.1.0.51', undef],
+        "$test_name: and changes the first's",
+    );
+
+    # an entry from before the cache was kept per vnet answers for any vnet,
+    # and a mapping edit elsewhere does not touch it
+    $test_state->{macdb} = { macs => { $mac => { ip4 => '10.2.0.60' } } };
+    create_ip({ zone => $zoneid, vnet => 'vnet1', mac => $mac, ip => '10.1.0.52' });
+    is_deeply(
+        [PVE::Network::SDN::Vnets::get_ips_from_mac('vnet2', $mac)],
+        ['10.2.0.60', undef],
+        "$test_name: an old entry still answers for the other vnet",
+    );
 }
 
 run_test(\&test_ipam_per_vnet);
 
+sub test_ipam_edit_stale_entry {
+    my $test_name = (split(/::/, (caller(0))[3]))[-1];
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+    my $mac = "da:65:8f:18:9b:6f";
+    my $other = "da:65:8f:18:9b:70";
+
+    create_dhcp_zone($zoneid, $vnetid, "10.0.0.0/24");
+
+    # a node not yet upgraded released the MAC's address and handed it to
+    # another guest, while the vnet's cache entry still holds it
+    create_ip({ zone => $zoneid, vnet => $vnetid, mac => $other, ip => '10.0.0.50' });
+    $test_state->{macdb}->{vnets}->{$vnetid}->{$mac} = { ip4 => '10.0.0.50' };
+
+    PVE::API2::Network::SDN::Ips->ipupdate(
+        { zone => $zoneid, vnet => $vnetid, mac => $mac, ip => '10.0.0.60' });
+    is(
+        get_ip4(get_ips_from_mac($other)),
+        '10.0.0.50',
+        "$test_name: an edit leaves the other guest's record alone",
+    );
+    is(get_ip4(get_ips_from_mac($mac)), '10.0.0.60', "$test_name: and records the new address");
+    is(
+        $test_state->{macdb}->{vnets}->{$vnetid}->{$mac}->{ip4},
+        '10.0.0.60',
+        "$test_name: which the cache holds now",
+    );
+
+    # a deletion of a mapping the IPAM does not hold is refused
+    $test_state->{macdb}->{vnets}->{$vnetid}->{$mac} = { ip4 => '10.0.0.50' };
+    eval {
+        PVE::API2::Network::SDN::Ips->ipdelete(
+            { zone => $zoneid, vnet => $vnetid, mac => $mac, ip => '10.0.0.50' });
+    };
+    like($@, qr/not mapped to/, "$test_name: a deletion for another guest's address is refused");
+}
+
+run_test(\&test_ipam_edit_stale_entry);
+
+sub test_dnsmasq_reservation_per_vnet {
+    my $test_name = (split(/::/, (caller(0))[3]))[-1];
+    my $zoneid = "TESTZONE";
+    my $mac = "da:65:8f:18:9b:6f";
+
+    # the same MAC on two vnets of one zone has a reservation on each, and a
+    # start on one vnet changes that vnet's line only
+    create_zone({ type => "simple", dhcp => "dnsmasq", ipam => "pve", zone => $zoneid });
+    for my $n (1, 2) {
+        create_dhcp_vnet($zoneid, "vnet$n", "10.$n.0.0/24");
+        create_ip({ zone => $zoneid, vnet => "vnet$n", mac => $mac, ip => "10.$n.0.50" });
+    }
+
+    my $reservations = sub {
+        return [sort grep { $_ } split(/\n/, PVE::Tools::file_get_contents($TMP_ETHERS_FILE))];
+    };
+
+    nic_start("vnet$_", $mac, "999", "testhostname") for (1, 2, 1);
+    is_deeply(
+        $reservations->(),
+        ["$mac,10.1.0.50", "$mac,10.2.0.50"],
+        "$test_name: each vnet keeps its reservation",
+    );
+
+    PVE::API2::Network::SDN::Ips->ipupdate(
+        { zone => $zoneid, vnet => 'vnet1', mac => $mac, ip => '10.1.0.51' });
+    nic_start('vnet1', $mac, "999", "testhostname");
+    is_deeply(
+        $reservations->(),
+        ["$mac,10.1.0.51", "$mac,10.2.0.50"],
+        "$test_name: an edit replaces the reservation of its vnet only",
+    );
+
+    # the other lines are checked against the cache alone, so a line the
+    # cache does not hold is dropped without the IPAM being asked
+    delete $test_state->{macdb}->{vnets}->{vnet2};
+    nic_start('vnet1', $mac, "999", "testhostname");
+    is_deeply(
+        $reservations->(),
+        ["$mac,10.1.0.51"],
+        "$test_name: a line the cache does not hold is dropped",
+    );
+
+    # the changed MAC's own line is answered by the IPAM when the cache does
+    # not hold it
+    delete $test_state->{macdb}->{vnets}->{vnet1};
+    PVE::Network::SDN::Dhcp::Dnsmasq->update_ip_mapping($zoneid, 'vnet1', $mac);
+    is_deeply(
+        $reservations->(),
+        ["$mac,10.1.0.51"],
+        "$test_name: the changed MAC's line is kept without a cache entry",
+    );
+}
+
+run_test(\&test_dnsmasq_reservation_per_vnet);
+
 done_testing();
-- 
2.47.3