From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id D62B91FF0AB for ; Wed, 21 Oct 2026 08:44:39 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 735152141F; Wed, 21 Oct 2026 08:44:37 +0200 (CEST) Message-ID: <4aa82210-d5e2-4fe8-8489-26216796adac@proxmox.com> Date: Wed, 21 Oct 2026 08:44:29 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-network v3 11/17] sdn: dhcp: push IP mapping edits to the guest's node To: Stefan Hanreich , pve-devel@lists.proxmox.com References: <20261020080005.470757-1-h.laimer@proxmox.com> <20261020080005.470757-12-h.laimer@proxmox.com> From: Hannes Laimer Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792565069878 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.242 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 4RY32WLY3XSWHMN4QK3V2UNH2YZHGBX2 X-Message-ID-Hash: 4RY32WLY3XSWHMN4QK3V2UNH2YZHGBX2 X-MailFrom: h.laimer@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 2026-10-20 15:10, Stefan Hanreich wrote: > On 10/20/26 10:02 AM, Hannes Laimer wrote: >> IP mappings live in the cluster-wide IPAM and can be edited through >> the API on any node. The DHCP backends hold their copy on each node, >> dnsmasq in a reservation file and the ebpf responder in a kernel map. >> An edit only updated the backend of the node it was made on, so a >> guest on another node kept getting the old mapping. >> >> The new endpoint POST /nodes/{node}/sdn/dhcp-mapping updates the >> mapping of one MAC on that node from the current records. An edit >> made on a node other than the guest's calls it on the guest's node. >> Only that node answers the guest's DHCP requests, so no other node is >> called. The others catch up when the guest starts on one of them. >> >> The call runs in the background, so the request returns without >> waiting on the other node. A call that fails logs its error to the >> journal of the node the edit was made on. One that gets no answer is >> stopped after a minute. The record stays as written either way, and >> the guest's node picks it up at the latest when the guest next starts >> there. >> >> Signed-off-by: Hannes Laimer >> --- >> src/PVE/API2/Network/SDN/Ips.pm | 3 + >> src/PVE/API2/Network/SDN/Nodes/Status.pm | 37 ++++++++++ >> src/PVE/Network/SDN/Dhcp.pm | 74 +++++++++++++++++++ >> src/test/run_test_vnets_blackbox.pl | 90 ++++++++++++++++++++++++ >> 4 files changed, 204 insertions(+) >> >> diff --git a/src/PVE/API2/Network/SDN/Ips.pm b/src/PVE/API2/Network/SDN/Ips.pm >> index d7b682dd..5b45de7c 100644 >> --- a/src/PVE/API2/Network/SDN/Ips.pm >> +++ b/src/PVE/API2/Network/SDN/Ips.pm >> @@ -47,6 +47,7 @@ __PACKAGE__->register_method({ >> die "$@\n" if $@; >> >> PVE::Network::SDN::Dhcp::update_mapping($vnet, $mac); >> + PVE::Network::SDN::Dhcp::notify_guest_node($vnet, $mac); >> >> return undef; >> }, >> @@ -85,6 +86,7 @@ __PACKAGE__->register_method({ >> PVE::Network::SDN::Vnets::add_ip($vnet, $ip, '', $mac, undef); >> >> PVE::Network::SDN::Dhcp::update_mapping($vnet, $mac); >> + PVE::Network::SDN::Dhcp::notify_guest_node($vnet, $mac); >> >> return undef; >> }, >> @@ -138,6 +140,7 @@ __PACKAGE__->register_method({ >> die "$error\n" if $error; >> >> PVE::Network::SDN::Dhcp::update_mapping($vnet, $mac); >> + PVE::Network::SDN::Dhcp::notify_guest_node($vnet, $mac); >> >> return undef; >> }, >> diff --git a/src/PVE/API2/Network/SDN/Nodes/Status.pm b/src/PVE/API2/Network/SDN/Nodes/Status.pm >> index 7977e0c8..fe750a85 100644 >> --- a/src/PVE/API2/Network/SDN/Nodes/Status.pm >> +++ b/src/PVE/API2/Network/SDN/Nodes/Status.pm >> @@ -9,6 +9,9 @@ use PVE::API2::Network::SDN::Nodes::Vnets; >> >> use PVE::JSONSchema qw(get_standard_option); >> >> +use PVE::Network::SDN::Vnets; >> +use PVE::Network::SDN::Dhcp; >> + >> use PVE::RESTHandler; >> use base qw(PVE::RESTHandler); >> >> @@ -27,6 +30,40 @@ __PACKAGE__->register_method({ >> path => 'vnets', >> }); >> >> +__PACKAGE__->register_method({ >> + name => 'dhcp_mapping', >> + path => 'dhcp-mapping', >> + method => 'POST', >> + description => >> + 'Re-apply the DHCP mapping of a MAC address on this node from the current records.', >> + permissions => { >> + check => ['perm', '/sdn/zones/{zone}/{vnet}', ['SDN.Allocate']], >> + }, >> + protected => 1, >> + proxyto => 'node', >> + parameters => { >> + additionalProperties => 0, >> + properties => { >> + node => get_standard_option('pve-node'), >> + zone => get_standard_option('pve-sdn-zone-id'), >> + vnet => get_standard_option('pve-sdn-vnet-id'), >> + mac => get_standard_option('mac-addr'), >> + }, >> + }, >> + returns => { type => 'null' }, >> + code => sub { >> + my ($param) = @_; >> + >> + my $vnet = PVE::Network::SDN::Vnets::get_vnet($param->{vnet}, 1); >> + die "vnet '$param->{vnet}' does not exist in zone '$param->{zone}'\n" >> + if !$vnet || $vnet->{zone} ne $param->{zone}; >> + >> + PVE::Network::SDN::Dhcp::update_mapping($param->{vnet}, $param->{mac}); >> + >> + return undef; >> + }, >> +}); >> + >> __PACKAGE__->register_method({ >> name => 'sdnindex', >> path => '', >> diff --git a/src/PVE/Network/SDN/Dhcp.pm b/src/PVE/Network/SDN/Dhcp.pm >> index cf805a03..1f43745f 100644 >> --- a/src/PVE/Network/SDN/Dhcp.pm >> +++ b/src/PVE/Network/SDN/Dhcp.pm >> @@ -3,6 +3,8 @@ package PVE::Network::SDN::Dhcp; >> use strict; >> use warnings; >> >> +use POSIX qw(); >> + >> use PVE::Cluster; >> >> use PVE::Network::SDN; >> @@ -86,6 +88,78 @@ sub guest_nics { >> return $nics; >> } >> >> +sub guest_node_by_mac { >> + my ($vnetid, $mac) = @_; >> + >> + for my $nic (guest_nics()->@*) { >> + return $nic->{node} >> + if lc($nic->{mac} // '') eq lc($mac) && $nic->{bridge} eq $vnetid; >> + } >> + >> + return undef; >> +} > > seems like we're always (or at least most of the time) querying by MAC > so would assembling a hash keyed by MAC addresses in the first place be sensible? > yes, keyed by lc mac with a list per mac since one can appear on several guests will do in v4 > >> +sub notify_guest_node { >> + my ($vnetid, $mac) = @_; >> + >> + return if !$mac; >> + >> + # best effort like the push itself, since the record is written by now >> + my ($zoneid, $node) = eval { >> + my $vnet = PVE::Network::SDN::Vnets::get_vnet($vnetid, 1); >> + return if !$vnet; >> + >> + my $zone = PVE::Network::SDN::Zones::get_zone($vnet->{zone}, 1); >> + return if !$zone || !$zone->{ipam} || !$zone->{dhcp}; >> + >> + return ($vnet->{zone}, guest_node_by_mac($vnetid, $mac)); >> + }; >> + if ($@) { >> + warn "could not find the node to push the dhcp mapping of $mac to: $@"; >> + return; >> + } >> + return if !$node || $node eq PVE::INotify::nodename(); >> + >> + # double fork, the grandchild is reparented to init so nothing on >> + # the request path ever waits on it >> + my $pid = fork(); >> + if (!defined($pid)) { >> + warn "could not fork for the dhcp mapping push to $node: $!\n"; >> + return; >> + } >> + if ($pid) { >> + waitpid($pid, 0); >> + return; >> + } >> + >> + POSIX::setsid(); >> + my $child = fork(); >> + POSIX::_exit(1) if !defined($child); >> + POSIX::_exit(0) if $child; >> + >> + # nothing of the request stays with the push, and since the daemon's >> + # stderr is closed a failed push reports through the journal >> + open(STDIN, '<', '/dev/null') or POSIX::_exit(1); >> + open(STDOUT, '>', '/dev/null') or POSIX::_exit(1); >> + open(STDERR, '|-', 'logger', '-t', 'pve-sdn', '-p', 'daemon.warning') >> + or POSIX::_exit(1); >> + # a node that takes the connection but never answers must not keep the >> + # push around >> + exec( >> + 'timeout', >> + '60', >> + 'pvesh', >> + 'create', >> + "/nodes/$node/sdn/dhcp-mapping", >> + '--zone', >> + $zoneid, >> + '--vnet', >> + $vnetid, >> + '--mac', >> + $mac, >> + ) or POSIX::_exit(1); >> +} >> + > > There've been efforts of moving away from pvesh - see my other patch > series introducing the api client instead of calling pvesh here: > [1] > https://lore.proxmox.com/all/20260709140625.275618-1-s.hanreich@proxmox.com/ > > Might be the better option here as well? i did consider that, but at the time it seemed like "a lot" for something where a different precedent exists already, but yes, `pvesh` did seem *meh* here. also wasn't aware of [1] > > Also, there is proxyto_callback which we could use to proxy to the correct > node in the first place? yes, this should work, and be the better approach here. the only thing it introduces is a cluster endpoint whose edit fails when another node isn't reachable, which we don't have yet.. but tbf, it's niche, and arguably also correct here will include in v4! > >> sub tap_plug { >> my ($bridge, $iface, $mac) = @_; >> >> diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl >> index 783d0d47..9a4dbb9c 100755 >> --- a/src/test/run_test_vnets_blackbox.pl >> +++ b/src/test/run_test_vnets_blackbox.pl >> @@ -49,6 +49,7 @@ sub clear_test_state { >> dnsmasq_calls => [], >> ebpf_calls => [], >> order => [], >> + notify_calls => [], >> warnings => [], >> ipam_config => { >> 'ids' => { >> @@ -238,6 +239,11 @@ $mocked_sdn_dhcp_dnsmasq->mock( >> update_lease => sub { }, >> ); >> >> +my $mocked_sdn_dhcp = Test::MockModule->new('PVE::Network::SDN::Dhcp'); >> +$mocked_sdn_dhcp->mock( >> + notify_guest_node => sub { push $test_state->{notify_calls}->@*, [@_]; }, >> +); >> + >> my $mocked_pve_rs_dhcp = Test::MockModule->new('PVE::RS::SDN::Dhcp'); >> $mocked_pve_rs_dhcp->mock( >> detach => sub { >> @@ -1280,6 +1286,90 @@ sub test_dhcp_backend_needed_per_node { >> >> run_test(\&test_dhcp_backend_needed_per_node); >> >> +sub test_mapping_push_target { >> + 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", >> + }); >> + >> + # 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' }, >> + 101 => { type => 'lxc', node => 'third' }, >> + }, >> + }; >> + $test_state->{guest_nets} = { >> + 1 => { net0 => "virtio=$mac,bridge=$vnetid" }, >> + 100 => { net0 => "virtio=" . uc($mac) . ",bridge=$vnetid" }, >> + 101 => { net0 => "name=eth0,bridge=vmbr0,hwaddr=00:11:22:33:44:55" }, >> + }; >> + is( >> + PVE::Network::SDN::Dhcp::guest_node_by_mac($vnetid, $mac), >> + 'other', >> + "$test_name: the MAC resolves to the node of its guest, a guest the list lacks is skipped", >> + ); >> + is( >> + PVE::Network::SDN::Dhcp::guest_node_by_mac($vnetid, '00:11:22:33:44:55'), >> + undef, >> + "$test_name: a MAC on another bridge does not resolve", >> + ); >> + is( >> + PVE::Network::SDN::Dhcp::guest_node_by_mac($vnetid, '00:11:22:33:44:66'), >> + undef, >> + "$test_name: an unknown MAC does not resolve", >> + ); >> + >> + # every mapping edit through the API pokes the guest's node once >> + $test_state->{notify_calls} = []; >> + create_ip({ >> + zone => $zoneid, >> + vnet => $vnetid, >> + mac => $mac, >> + ip => "10.0.0.50", >> + }); >> + update_ip({ >> + zone => $zoneid, >> + vnet => $vnetid, >> + mac => $mac, >> + ip => "10.0.0.51", >> + }); >> + delete_ip({ >> + zone => $zoneid, >> + vnet => $vnetid, >> + mac => $mac, >> + ip => "10.0.0.51", >> + }); >> + eq_or_diff( >> + $test_state->{notify_calls}, >> + [[$vnetid, $mac], [$vnetid, $mac], [$vnetid, $mac]], >> + "$test_name: create, edit and delete each poke the guest's node", >> + ); >> + delete $test_state->{vmlist}; >> + delete $test_state->{guest_nets}; >> +} >> + >> +run_test(\&test_mapping_push_target); >> + >> sub test_ipam_cache_misses { >> my $test_name = (split(/::/, (caller(0))[3]))[-1]; >> my $zoneid = "TESTZONE"; > > > > >