[PATCH pve-network v6 06/13] sdn: dhcp: update reservations when IP mappings change

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

Creating, updating or deleting an IP mapping through the API changed
the IPAM and the MAC cache, but never the zone's dnsmasq reservation
file. dnsmasq could keep serving the old reservation, or none for a
new mapping, until the guest's next start.

A guest start, an API edit and the deletion of a NIC's addresses now
all rewrite the MAC's reservation from the cache. The reservations are
a file on each node, and only the guest's node answers its DHCP
requests. So an API edit is proxied to the node of the guest with that
MAC on the vnet, unless that node is offline. The other nodes catch up
when the guest starts on one of them.

The edit holds the guest's migration lock, so a migration cannot move
the guest during the edit. An edit that meets a migration, or finds
the guest moved to another node, fails before writing anything. If the
reservation update fails, the API edit fails with an error saying the
IPAM is already updated. A guest start and the deletion of a NIC's
addresses only warn.

Each rewrite reads the current cache only once it holds the
reservation file's lock. A rewrite working from an older read would
undo the lines another one had just written. The MAC's line is written
in full from the cache, so a guest gaining an IPv6 address next to its
IPv4 one gets it reserved too.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 debian/control                      |   2 +
 src/PVE/API2/Network/SDN/Ips.pm     | 116 +++++++++++--
 src/PVE/Network/SDN/Dhcp.pm         |  51 +++---
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 107 +++++++-----
 src/PVE/Network/SDN/Dhcp/Plugin.pm  |   6 +-
 src/PVE/Network/SDN/Vnets.pm        |  12 +-
 src/test/run_test_vnets_blackbox.pl | 248 +++++++++++++++++++++++++++-
 7 files changed, 455 insertions(+), 87 deletions(-)

diff --git a/debian/control b/debian/control
index 268f40186b6b..bcdc8e50fe76 100644
--- a/debian/control
+++ b/debian/control
@@ -6,6 +6,7 @@ Build-Depends: debhelper-compat (= 13),
                lintian,
                libfile-slurp-perl,
                libpve-common-perl (>= 9.2.4),
+               libpve-guest-common-perl,
                libnet-subnet-perl,
                libpve-rs-perl (>= 0.15.0),
                libtest-mockmodule-perl,
@@ -42,6 +43,7 @@ Description: Proxmox VE's SDN (Software Defined Network) stack
 Package: libpve-network-api-perl
 Architecture: all
 Depends: libpve-common-perl (>= 9.0.2),
+         libpve-guest-common-perl,
          pve-firewall (>= 5.1.0~),
          pve-cluster (>= 9.0.1),
          libpve-network-perl (= ${binary:Version}),
diff --git a/src/PVE/API2/Network/SDN/Ips.pm b/src/PVE/API2/Network/SDN/Ips.pm
index 04741d8750a4..8a6106ba29a7 100644
--- a/src/PVE/API2/Network/SDN/Ips.pm
+++ b/src/PVE/API2/Network/SDN/Ips.pm
@@ -3,9 +3,13 @@ package PVE::API2::Network::SDN::Ips;
 use strict;
 use warnings;
 
+use PVE::Cluster;
+use PVE::GuestHelpers;
+use PVE::INotify;
 use PVE::Tools qw(extract_param);
 
 use PVE::Network::SDN::Vnets;
+use PVE::Network::SDN::Zones;
 use PVE::Network::SDN::Dhcp;
 
 use PVE::JSONSchema qw(get_standard_option);
@@ -13,12 +17,73 @@ use PVE::RESTHandler;
 
 use base qw(PVE::RESTHandler);
 
+# only the guest's node answers its DHCP requests, so the edit runs there.
+# With that node offline it runs here, and the node catches up at the
+# guest's next start
+my $proxy_to_guest_node = sub {
+    my ($rpcenv, $proxyto, $param) = @_;
+
+    my $vnet = PVE::Network::SDN::Vnets::get_vnet($param->{vnet}, 1);
+    my $zone = $vnet ? PVE::Network::SDN::Zones::get_zone($vnet->{zone}, 1) : undef;
+    return 'localhost' if !$zone || !$zone->{dhcp};
+
+    my ($node) = PVE::Network::SDN::Dhcp::guest_node_by_mac($param->{vnet}, $param->{mac});
+    my $members = PVE::Cluster::get_members();
+    return 'localhost' if !$node || ($members && !($members->{$node} // {})->{online});
+    return $node;
+};
+
+# the edit runs under the guest's migration lock on its node, so a migration
+# cannot move the guest between the IPAM write and the reservation update,
+# and an edit that finds the guest moved away fails before it writes anything
+my $with_guest_lock = sub {
+    my ($vnet, $mac, $code) = @_;
+
+    my (undef, $vmid) = PVE::Network::SDN::Dhcp::guest_node_by_mac($vnet, $mac);
+    return $code->() if !defined($vmid);
+
+    # an error of the edit itself is kept apart from the lock's own failure
+    my ($res, $edit_err);
+    eval {
+        PVE::GuestHelpers::guest_migration_lock(
+            $vmid,
+            1,
+            sub {
+                $res = eval {
+                    # a migration that ended while the lock was waited for moved
+                    # the config, and the vmlist of the request's start has not
+                    # seen it
+                    PVE::Cluster::cfs_update();
+                    my $target =
+                        $proxy_to_guest_node->(undef, undef, { vnet => $vnet, mac => $mac });
+                    die "guest $vmid moved to $target, try again\n"
+                        if $target ne 'localhost' && $target ne PVE::INotify::nodename();
+                    $code->();
+                };
+                $edit_err = $@;
+            },
+        );
+    };
+    die "guest $vmid is locked, maybe by a migration\n" if $@;
+    die $edit_err if $edit_err;
+    return $res;
+};
+
+my $push_mapping = sub {
+    my ($vnet, $mac) = @_;
+
+    eval { PVE::Network::SDN::Dhcp::update_mapping($vnet, $mac) };
+    # the records are written by then and stay
+    die "IPAM is updated for $mac, but DHCP is not: $@" if $@;
+};
+
 __PACKAGE__->register_method({
     name => 'ipdelete',
     path => '',
     method => 'DELETE',
     description => 'Delete IP Mappings in a VNet',
     protected => 1,
+    proxyto_callback => $proxy_to_guest_node,
     permissions => {
         description => "Requires 'SDN.Allocate' on '/sdn/zones/<zone>/<vnet>'",
         user => 'all',
@@ -55,8 +120,16 @@ __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); };
-        die "$@\n" if $@;
+        $with_guest_lock->(
+            $vnet,
+            $mac,
+            sub {
+                eval { PVE::Network::SDN::Vnets::del_ip($vnet, $ip, '', $mac); };
+                die "$@\n" if $@;
+
+                $push_mapping->($vnet, $mac);
+            },
+        );
 
         return undef;
     },
@@ -68,6 +141,7 @@ __PACKAGE__->register_method({
     method => 'POST',
     description => 'Create IP Mapping in a VNet',
     protected => 1,
+    proxyto_callback => $proxy_to_guest_node,
     permissions => {
         description => "Requires 'SDN.Allocate' on '/sdn/zones/<zone>/<vnet>'",
         user => 'all',
@@ -104,7 +178,14 @@ __PACKAGE__->register_method({
         my $mac = extract_param($param, 'mac');
         my $ip = extract_param($param, 'ip');
 
-        PVE::Network::SDN::Vnets::add_ip($vnet, $ip, '', $mac, undef);
+        $with_guest_lock->(
+            $vnet,
+            $mac,
+            sub {
+                PVE::Network::SDN::Vnets::add_ip($vnet, $ip, '', $mac, undef);
+                $push_mapping->($vnet, $mac);
+            },
+        );
 
         return undef;
     },
@@ -115,6 +196,7 @@ __PACKAGE__->register_method({
     method => 'PUT',
     description => 'Update IP Mapping in a VNet',
     protected => 1,
+    proxyto_callback => $proxy_to_guest_node,
     permissions => {
         description => "Requires 'SDN.Allocate' on '/sdn/zones/<zone>/<vnet>'",
         user => 'all',
@@ -155,19 +237,29 @@ __PACKAGE__->register_method({
         my $vmid = extract_param($param, 'vmid');
         my $ip = extract_param($param, 'ip');
 
-        my ($old_ip4, $old_ip6) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnet, $mac);
-        my $old_ip = (Net::IP::ip_get_version($ip) == 4) ? $old_ip4 : $old_ip6;
+        $with_guest_lock->(
+            $vnet,
+            $mac,
+            sub {
+                my ($old_ip4, $old_ip6) =
+                    PVE::Network::SDN::Vnets::get_ips_from_mac($vnet, $mac);
+                my $old_ip = (Net::IP::ip_get_version($ip) == 4) ? $old_ip4 : $old_ip6;
 
-        PVE::Network::SDN::Vnets::del_ip($vnet, $old_ip, '', $mac);
+                PVE::Network::SDN::Vnets::del_ip($vnet, $old_ip, '', $mac);
 
-        eval { PVE::Network::SDN::Vnets::add_ip($vnet, $ip, '', $mac, $vmid); };
-        my $error = $@;
+                eval { PVE::Network::SDN::Vnets::add_ip($vnet, $ip, '', $mac, $vmid); };
+                my $error = $@;
 
-        if ($error) {
-            PVE::Network::SDN::Vnets::add_ip($vnet, $old_ip, '', $mac, $vmid);
-        }
+                if ($error) {
+                    PVE::Network::SDN::Vnets::add_ip($vnet, $old_ip, '', $mac, $vmid);
+                }
+
+                die "$error\n" if $error;
+
+                $push_mapping->($vnet, $mac);
+            },
+        );
 
-        die "$error\n" if $error;
         return undef;
     },
 });
diff --git a/src/PVE/Network/SDN/Dhcp.pm b/src/PVE/Network/SDN/Dhcp.pm
index f6ce842c8cc2..11bb6df8c23a 100644
--- a/src/PVE/Network/SDN/Dhcp.pm
+++ b/src/PVE/Network/SDN/Dhcp.pm
@@ -6,7 +6,6 @@ use warnings;
 use PVE::Cluster;
 
 use PVE::Network::SDN;
-use PVE::Network::SDN::Ipams;
 use PVE::Network::SDN::Subnets;
 use PVE::Network::SDN::Dhcp::Plugin;
 use PVE::Network::SDN::Dhcp::Dnsmasq;
@@ -22,37 +21,47 @@ sub plugin_types {
     return PVE::Network::SDN::Dhcp::Plugin->lookup_types();
 }
 
-sub add_mapping {
-    my ($vnetid, $mac, $ip4, $ip6) = @_;
+sub update_mapping {
+    my ($vnetid, $mac) = @_;
+
+    # a record without a MAC has no guest NIC behind it, so no backend serves it
+    return if !$mac;
 
-    my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid);
+    # the backend to push to is the applied one, since a zone edited but
+    # not applied has nothing running yet
+    my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid, 1);
     return if !$vnet;
 
     my $zoneid = $vnet->{zone};
-    my $zone = PVE::Network::SDN::Zones::get_zone($zoneid);
-
-    return if !$zone->{ipam} || !$zone->{dhcp};
+    my $zone = PVE::Network::SDN::Zones::get_zone($zoneid, 1);
+    return if !$zone || !$zone->{ipam} || !$zone->{dhcp};
 
-    my $dhcptype = $zone->{dhcp};
+    # a zone confined to other nodes has no vnet bridge on this node
+    return if defined($zone->{nodes}) && !$zone->{nodes}->{ PVE::INotify::nodename() };
 
-    my $macdb = PVE::Network::SDN::Ipams::read_macdb();
-    my $dhcp_plugin = PVE::Network::SDN::Dhcp::Plugin->lookup($dhcptype);
-    $dhcp_plugin->add_ip_mapping($zoneid, $macdb, $mac, $ip4, $ip6);
+    PVE::Network::SDN::Dhcp::Plugin->lookup($zone->{dhcp})->update_ip_mapping($zoneid, $mac);
 }
 
-sub remove_mapping {
+sub guest_node_by_mac {
     my ($vnetid, $mac) = @_;
 
-    my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid);
-    return if !$vnet;
-
-    my $zoneid = $vnet->{zone};
-    my $zone = PVE::Network::SDN::Zones::get_zone($zoneid);
-
-    return if !$zone->{ipam} || !$zone->{dhcp};
+    return if !$mac;
+
+    my $vmlist = PVE::Cluster::get_vmlist();
+    my $nets = PVE::Cluster::get_guest_config_properties([map { "net$_" } 0 .. 31]);
+    for my $vmid (sort keys %$nets) {
+        my $guest = $vmlist->{ids}->{$vmid} // next;
+        for my $net (values %{ $nets->{$vmid} }) {
+            my ($bridge) = $net =~ m/(?:^|,)bridge=([^,]+)/ or next;
+            next if $bridge ne $vnetid;
+            # a VM names the MAC as the value of its model, a container as hwaddr
+            my ($nic_mac) = $net =~ m/(?:^|,)[a-z0-9_-]+=([0-9a-f]{2}(?::[0-9a-f]{2}){5})(?:,|$)/i
+                or next;
+            return ($guest->{node}, $vmid) if lc($nic_mac) eq lc($mac);
+        }
+    }
 
-    my $dhcp_plugin = PVE::Network::SDN::Dhcp::Plugin->lookup($zone->{dhcp});
-    $dhcp_plugin->del_ip_mapping($zoneid, $mac);
+    return;
 }
 
 sub regenerate_config {
diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index c736d02d8f23..5864ead87d29 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -11,7 +11,9 @@ use PVE::Tools qw(file_set_contents run_command lock_file);
 use File::Copy;
 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';
@@ -51,83 +53,101 @@ sub update_lease {
     $manager->AddDhcpLease($ip4, $mac, \@hostname, undef, 0, 0, 0) if $ip4;
 }
 
-sub add_ip_mapping {
-    my ($class, $dhcpid, $macdb, $mac, $ip4, $ip6) = @_;
+sub update_ip_mapping {
+    my ($class, $dhcpid, $mac) = @_;
     $mac = lc($mac);
 
     my $ethers_file = ethers_file($dhcpid);
     my $ethers_tmp_file = "$ethers_file.tmp";
 
-    my $reload = undef;
+    my ($ip4, $ip6);
+    my $reservation = undef;
+    my $changed = undef;
+    my @resume_answer;
 
-    my $appendFn = sub {
+    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
+        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;
+            # an external IPAM spells addresses its own way, while the file holds one spelling
+            $reservation = lc($reservation);
+        }
+
+        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 $match = undef;
+        my $kept = undef;
 
-        my $line_no = 0;
         while (my $line = <$in>) {
-            $line_no++;
             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
+            $parsed_mac = lc($parsed_mac);
 
-            if (!defined($parsed_mac)) {
-                warn "failed to parse MAC from $dhcpid ethers file on line $line_no: '$line'\n";
+            # 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;
             }
-            # the cache is read in lower case, a line from before keeps its writer's spelling
-            $parsed_mac = lc($parsed_mac);
 
             my ($parsed_ip4, $parsed_ip6);
             if ($parsed_ip2) {
                 $parsed_ip4 = $parsed_ip1;
                 $parsed_ip6 = $parsed_ip2;
-            } elsif (Net::IP::ip_is_ipv4($parsed_ip1)) {
+            } elsif (defined($parsed_ip1) && Net::IP::ip_is_ipv4($parsed_ip1)) {
                 $parsed_ip4 = $parsed_ip1;
-            } else {
+            } 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, a dropped one would leave that guest unknown to
             # dnsmasq until its next start
+            my $current = $macs->{$parsed_mac};
             if (
-                !defined($macdb->{macs}->{$parsed_mac})
-                || ($parsed_ip4
-                    && $macdb->{macs}->{$parsed_mac}->{'ip4'}
-                    && $macdb->{macs}->{$parsed_mac}->{'ip4'} ne $parsed_ip4)
-                || ($parsed_ip6
-                    && $macdb->{macs}->{$parsed_mac}->{'ip6'}
-                    && $macdb->{macs}->{$parsed_mac}->{'ip6'} ne $parsed_ip6)
+                !$current
+                || ($parsed_ip4 && $current->{ip4} && $current->{ip4} ne $parsed_ip4)
+                || ($parsed_ip6 && $current->{ip6} && lc($current->{ip6}) ne $parsed_ip6)
             ) {
-                $reload = 1;
-                # the MAC being added gets its line below, like a missing one
-                my $current = $macdb->{macs}->{$parsed_mac};
-                next if $parsed_mac eq $mac || !$current;
+                $changed = 1;
+                next if !$current;
                 my $replacement = $parsed_mac;
                 $replacement .= ",$current->{ip4}" if $current->{ip4};
                 $replacement .= ",[$current->{ip6}]" if $current->{ip6};
-                print $out "$replacement\n" if $replacement ne $parsed_mac;
+                print $out lc($replacement) . "\n";
                 next;
             }
 
-            if ($parsed_mac eq $mac) {
-                $match = 1 if $ip4 && $parsed_ip4 && $ip4;
-                $match = 1 if $ip6 && $parsed_ip6 && $ip6;
-            }
-
             print $out "$line\n";
         }
 
-        if (!$match) {
-            my $reservation = $mac;
-            $reservation .= ",$ip4" if $ip4;
-            $reservation .= ",[$ip6]" if $ip6;
+        if (defined($reservation) && !$kept) {
             print $out "$reservation\n";
-            $reload = 1;
+            $changed = 1;
         }
 
         close $in;
@@ -136,16 +156,17 @@ sub add_ip_mapping {
         chmod 0644, $ethers_file;
     };
 
-    PVE::Tools::lock_file($ethers_file, 10, $appendFn);
+    # the rewrite replaces the reservation file, so a lock on the file itself
+    # would not outlive the rename
+    PVE::Tools::lock_file("$ethers_file.lock", 10, $rewriteFn);
+    die "Unable to update $mac in the dnsmasq configuration: $@" if $@;
 
-    if ($@) {
-        warn "Unable to add $mac to the dnsmasq configuration: $@\n";
-        return;
-    }
+    systemctl_service('reload', "dnsmasq\@$dhcpid") if $changed;
 
-    my $service_name = "dnsmasq\@$dhcpid";
-    systemctl_service('reload', $service_name) if $reload;
-    update_lease($dhcpid, $ip4, $mac);
+    if ($ip4) {
+        eval { update_lease($dhcpid, $ip4, $mac) };
+        log_warn("could not update the dnsmasq lease of $mac: $@") if $@;
+    }
 }
 
 sub configure_subnet {
diff --git a/src/PVE/Network/SDN/Dhcp/Plugin.pm b/src/PVE/Network/SDN/Dhcp/Plugin.pm
index b5d32fa13c70..472ff1534743 100644
--- a/src/PVE/Network/SDN/Dhcp/Plugin.pm
+++ b/src/PVE/Network/SDN/Dhcp/Plugin.pm
@@ -22,8 +22,10 @@ sub private {
     return $defaultData;
 }
 
-sub add_ip_mapping {
-    my ($class, $dhcpid, $macdb, $mac, $ip4, $ip6) = @_;
+# the MAC's records changed, so make the backend serve the current ones. With no
+# record left its reservation goes. Dies if the backend could not be updated
+sub update_ip_mapping {
+    my ($class, $dhcpid, $mac) = @_;
     die 'implement in sub class';
 }
 
diff --git a/src/PVE/Network/SDN/Vnets.pm b/src/PVE/Network/SDN/Vnets.pm
index ea66aa2177e1..56516ed9de83 100644
--- a/src/PVE/Network/SDN/Vnets.pm
+++ b/src/PVE/Network/SDN/Vnets.pm
@@ -214,6 +214,12 @@ sub del_ips_from_mac {
     PVE::Network::SDN::Vnets::del_ip($vnetid, $ip4, $hostname, $mac) if $ip4;
     PVE::Network::SDN::Vnets::del_ip($vnetid, $ip6, $hostname, $mac) if $ip6;
 
+    if ($ip4 || $ip6) {
+        # a NIC going away does not fail on its DHCP backend
+        eval { PVE::Network::SDN::Dhcp::update_mapping($vnetid, $mac) };
+        warn $@ if $@;
+    }
+
     return ($ip4, $ip6);
 }
 
@@ -228,11 +234,15 @@ sub add_dhcp_mapping {
     return if !$zone->{ipam} || !$zone->{dhcp};
 
     my ($ip4, $ip6) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
+    # one push at the end covers the allocated and the existing records alike
     add_next_free_cidr($vnetid, $name, $mac, "$vmid", undef, 1, 4) if !$ip4;
     add_next_free_cidr($vnetid, $name, $mac, "$vmid", undef, 1, 6) if !$ip6;
 
     ($ip4, $ip6) = PVE::Network::SDN::Vnets::get_ips_from_mac($vnetid, $mac);
-    PVE::Network::SDN::Dhcp::add_mapping($vnetid, $mac, $ip4, $ip6) if $ip4 || $ip6;
+    return if !$ip4 && !$ip6;
+    # a guest start does not fail on its DHCP backend
+    eval { PVE::Network::SDN::Dhcp::update_mapping($vnetid, $mac) };
+    warn $@ if $@;
 }
 
 =head3 api_get_vnet($cfg, $id)
diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl
index 164ef4b4592e..f641b8087e9b 100755
--- a/src/test/run_test_vnets_blackbox.pl
+++ b/src/test/run_test_vnets_blackbox.pl
@@ -47,6 +47,7 @@ sub clear_test_state {
         vnets_config => {},
         macdb => {},
         ipamdb => {},
+        dnsmasq_calls => [],
         ipam_config => {
             'ids' => {
                 'pve' => {
@@ -222,7 +223,9 @@ $mocked_sdn_dhcp_dnsmasq->mock(
     assert_dnsmasq_installed => sub { return 1; },
     before_configure => sub { },
     ethers_file => sub { return "/tmp/ethers"; },
-    systemctl_service => sub { },
+    systemctl_service => sub {
+        push $test_state->{dnsmasq_calls}->@*, $_[0];
+    },
     update_lease => sub { },
 );
 
@@ -254,10 +257,31 @@ $mocked_rpc_env_obj->mock(
     check_any => sub { return 1; },
 );
 
+my $mocked_pve_inotify = Test::MockModule->new('PVE::INotify');
+$mocked_pve_inotify->mock(
+    nodename => sub { return 'localnode'; },
+);
+
+my $mocked_guest_helpers = Test::MockModule->new('PVE::GuestHelpers');
+$mocked_guest_helpers->mock(
+    guest_migration_lock => sub {
+        my ($vmid, $timeout, $code) = @_;
+        return $code->();
+    },
+);
+
 my $mocked_pve_cluster_obj = Test::MockModule->new('PVE::Cluster');
 $mocked_pve_cluster_obj->mock(
     check_cfs_quorum => sub { return 1; },
     cfs_lock_domain => $mocked_cfs_lock_domain,
+    # a vmlist change a test parks here lands with the next refresh, as a
+    # change made meanwhile on the cluster does
+    cfs_update => sub {
+        $test_state->{vmlist} = delete $test_state->{vmlist_pending}
+            if $test_state->{vmlist_pending};
+    },
+    get_vmlist => sub { return $test_state->{vmlist} // { ids => {} }; },
+    get_guest_config_properties => sub { return $test_state->{guest_nets} // {}; },
 );
 
 # ------- TEST FUNCTIONS --------------
@@ -303,6 +327,11 @@ sub create_zone {
     return $zone;
 }
 
+sub update_zone {
+    my ($zoneid, $params) = @_;
+    PVE::API2::Network::SDN::Zones->update({ zone => $zoneid, %$params });
+}
+
 sub get_vnet {
     my ($id) = @_;
     return eval { PVE::API2::Network::SDN::Vnets->read({ vnet => $id }); };
@@ -373,6 +402,16 @@ sub create_ip {
     return PVE::API2::Network::SDN::Ips->ipcreate($param);
 }
 
+sub update_ip {
+    my ($param) = @_;
+    return PVE::API2::Network::SDN::Ips->ipupdate($param);
+}
+
+sub delete_ip {
+    my ($param) = @_;
+    return PVE::API2::Network::SDN::Ips->ipdelete($param);
+}
+
 sub run_test {
     my $test = shift;
     clear_test_state();
@@ -1138,18 +1177,211 @@ sub test_dnsmasq_stale_reservation_replaced {
     };
     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->add_ip_mapping(
-        'TESTZONE', $test_state->{macdb}, $added, '10.0.1.13', undef,
-    );
+    PVE::Network::SDN::Dhcp::Dnsmasq->update_ip_mapping('TESTZONE', $added);
 
-    my @lines = sort grep { /\S/ } split(/\n/, PVE::Tools::file_get_contents($TMP_ETHERS_FILE));
-    eq_or_diff(
-        \@lines,
-        ["$stale,10.0.1.11", "$added,10.0.1.13"],
+    is(
+        PVE::Tools::file_get_contents($TMP_ETHERS_FILE),
+        "$stale,10.0.1.11\n$added,10.0.1.13\n",
         "$test_name: the stale line is replaced, the unknown one dropped",
     );
 }
 
 run_test(\&test_dnsmasq_stale_reservation_replaced);
 
+# -------------- dnsmasq mapping pushes
+
+sub test_dnsmasq_mapping_push {
+    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");
+
+    # the first push of a zone finds no ethers file yet
+    unlink($TMP_ETHERS_FILE);
+
+    my $ethers = sub { PVE::Tools::file_get_contents($TMP_ETHERS_FILE) };
+
+    eval { nic_start($vnetid, $mac, "999", "testhostname"); };
+    if ($@) {
+        fail("$test_name: nic_start: $@");
+        return;
+    }
+    is($ethers->(), "$mac,10.0.0.100\n", "$test_name: guest start reserves the allocation");
+
+    # the records exist already, so the restart pushes them without a change
+    $test_state->{dnsmasq_calls} = [];
+    nic_start($vnetid, $mac, "999", "testhostname");
+    is($ethers->(), "$mac,10.0.0.100\n", "$test_name: restart keeps the reservation");
+    is_deeply(
+        $test_state->{dnsmasq_calls},
+        [],
+        "$test_name: restart without a change does not reload",
+    );
+
+    update_ip({
+        zone => $zoneid,
+        vnet => $vnetid,
+        mac => $mac,
+        ip => "10.0.0.150",
+    });
+    is($ethers->(), "$mac,10.0.0.150\n", "$test_name: mapping edit replaces the reservation");
+
+    delete_ip({
+        zone => $zoneid,
+        vnet => $vnetid,
+        mac => $mac,
+        ip => "10.0.0.150",
+    });
+    is($ethers->(), "", "$test_name: mapping delete drops the reservation");
+
+    # a record without a MAC has no NIC to serve, so nothing is pushed for it
+    # and no guest is looked for, with guests on the vnet or not
+    my @warnings;
+    {
+        local $SIG{__WARN__} = sub { push @warnings, @_; };
+        local $test_state->{guest_nets} =
+            { 100 => { net0 => "virtio=da:65:8f:18:9b:71,bridge=$vnetid" } };
+        local $test_state->{vmlist} = { ids => { 100 => { node => 'localnode' } } };
+        create_ip({ zone => $zoneid, vnet => $vnetid, ip => "10.0.0.160" });
+        delete_ip({ zone => $zoneid, vnet => $vnetid, ip => "10.0.0.160" });
+    }
+    eq_or_diff(\@warnings, [], "$test_name: a record without a MAC warns nothing");
+
+    # a reservation of a MAC the cache does not know is swept by the next
+    # rewrite, and both addresses of the guest go on one line
+    create_dhcp_subnet($vnetid, "8888::/64");
+    PVE::Tools::file_set_contents($TMP_ETHERS_FILE, "aa:bb:cc:dd:ee:ff,10.0.0.77\n");
+    nic_start($vnetid, $mac, "999", "testhostname");
+    is(
+        $ethers->(),
+        "$mac,10.0.0.100,[8888::64]\n",
+        "$test_name: dual-stack reservation on one line, the unknown MAC swept",
+    );
+
+    # a zone confined to other nodes runs no dnsmasq here, so an edit is not
+    # pushed on this node
+    update_zone($zoneid, { nodes => 'other' });
+    create_ip({
+        zone => $zoneid,
+        vnet => $vnetid,
+        mac => $mac,
+        ip => "10.0.0.150",
+    });
+    is(
+        $ethers->(),
+        "$mac,10.0.0.100,[8888::64]\n",
+        "$test_name: a zone confined to other nodes gets no push here",
+    );
+}
+
+run_test(\&test_dnsmasq_mapping_push);
+
+sub test_failed_mapping_push {
+    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");
+    $mocked_sdn_dhcp_dnsmasq->mock(update_ip_mapping => sub { die "rewrite failed\n"; });
+
+    my @warnings;
+    {
+        local $SIG{__WARN__} = sub { push @warnings, @_; };
+        eval { nic_start($vnetid, $mac, "999", "testhostname"); };
+        is($@, '', "$test_name: a guest start does not fail on its backend");
+    }
+    like(join('', @warnings), qr/rewrite failed/, "$test_name: the guest start warns");
+
+    eval { update_ip({ zone => $zoneid, vnet => $vnetid, mac => $mac, ip => "10.0.0.150" }); };
+    like(
+        $@,
+        qr/IPAM is updated for \Q$mac\E, but DHCP is not: rewrite failed/,
+        "$test_name: an API edit fails",
+    );
+    is(get_ip4(get_ips_from_mac($mac)), "10.0.0.150", "$test_name: the edit is saved");
+
+    $mocked_sdn_dhcp_dnsmasq->unmock('update_ip_mapping');
+}
+
+run_test(\&test_failed_mapping_push);
+
+sub test_mapping_edit_target {
+    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");
+
+    # the guest behind a MAC is found by its NIC on the vnet, and the config
+    # writes the MAC in upper case
+    $test_state->{vmlist} = {
+        ids => {
+            100 => { type => 'qemu', node => 'other' },
+        },
+    };
+    $test_state->{guest_nets} = {
+        1 => { net0 => "virtio=$mac,bridge=$vnetid" },
+        100 => { net0 => "virtio=" . uc($mac) . ",bridge=$vnetid" },
+    };
+    is_deeply(
+        [PVE::Network::SDN::Dhcp::guest_node_by_mac($vnetid, $mac)],
+        ['other', 100],
+        "$test_name: the MAC resolves to the node of its guest, a guest the list lacks is skipped",
+    );
+
+    # a mapping edit through the API runs on the guest's node
+    my $proxyto = PVE::API2::Network::SDN::Ips->map_method_by_name('ipupdate')->{proxyto_callback};
+    is(
+        $proxyto->(undef, undef, { zone => $zoneid, vnet => $vnetid, mac => $mac }),
+        'other',
+        "$test_name: a mapping edit runs on the guest's node",
+    );
+    is(
+        $proxyto->(undef, undef, { zone => $zoneid, vnet => $vnetid, mac => '00:11:22:33:44:66' }),
+        'localhost',
+        "$test_name: one for a MAC without a guest runs here",
+    );
+
+    # the edit holds the guest's migration lock, a migration holding it fails
+    # the edit, and a guest found on another node under the lock fails it too
+    $test_state->{vmlist}->{ids}->{100}->{node} = 'localnode';
+    create_ip({ zone => $zoneid, vnet => $vnetid, mac => $mac, ip => "10.0.0.50" });
+    $mocked_guest_helpers->mock(
+        guest_migration_lock => sub { die "can't lock file '/var/lock/x' - got timeout\n" });
+    eval { update_ip({ zone => $zoneid, vnet => $vnetid, mac => $mac, ip => "10.0.0.60" }) };
+    like(
+        $@,
+        qr/^guest 100 is locked, maybe by a migration$/,
+        "$test_name: an edit during a migration fails",
+    );
+    $mocked_guest_helpers->mock(
+        guest_migration_lock => sub { my ($vmid, $timeout, $code) = @_; return $code->(); });
+    $test_state->{vmlist}->{ids}->{100}->{node} = 'other';
+    eval { update_ip({ zone => $zoneid, vnet => $vnetid, mac => $mac, ip => "10.0.0.60" }) };
+    like($@, qr/guest 100 moved to other/, "$test_name: an edit for a guest found elsewhere fails");
+    is(get_ip4(get_ips_from_mac($mac)), "10.0.0.50", "$test_name: and wrote nothing");
+    # a migration that ends while the lock is waited for is seen under the lock
+    $test_state->{vmlist}->{ids}->{100}->{node} = 'localnode';
+    $test_state->{vmlist_pending} = { ids => { 100 => { node => 'other' } } };
+    eval { update_ip({ zone => $zoneid, vnet => $vnetid, mac => $mac, ip => "10.0.0.60" }) };
+    like($@, qr/guest 100 moved to other/,
+        "$test_name: a move not yet seen is seen under the lock");
+    is(get_ip4(get_ips_from_mac($mac)), "10.0.0.50", "$test_name: and wrote nothing either");
+    $test_state->{vmlist}->{ids}->{100}->{node} = 'localnode';
+    update_ip({ zone => $zoneid, vnet => $vnetid, mac => $mac, ip => "10.0.0.60" });
+    is(
+        get_ip4(get_ips_from_mac($mac)),
+        "10.0.0.60",
+        "$test_name: one for a guest here goes through",
+    );
+    delete $test_state->{vmlist};
+    delete $test_state->{guest_nets};
+}
+
+run_test(\&test_mapping_edit_target);
+
 done_testing();
-- 
2.47.3