This is a demo of the problem. Set up two NATed networks with dnsmasq and start a domain with two interfaces, one from each network. Only one network will report DHCP lease in 'virsh net-dhcp-leases'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/leaseshelper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index a1780ca4e4..02759f2314 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -172,6 +172,8 @@ main(int argc, char **argv) case VIR_LEASE_ACTION_ADD: case VIR_LEASE_ACTION_OLD: /* Create new lease */ + sleep(60); + if (virLeaseNew(&lease_new, mac, clientid, ip, hostname, iaid, server_duid) < 0) goto cleanup; /* Custom ipv6 leases *will not* be created if the env-var DNSMASQ_MAC -- 2.26.2

On a DHCP transaction, dnsmasq runs our leases helper which updates corresponding JSON files. While one dnsmasq won't run the leaseshelper in parallel, two dnsmasqs (from two distinct networks) might. To avoid corrupting JSON file, the leaseshelper acquires PID file first. Well, the way it's acquiring it is not ideal - it calls virPidFileAcquirePath(wait = false); which means, that either it acquires the PID file instantly or returns an error and does not touch the JSON at all. This in turn means that there might be a leases record missing. With wait = true, this won't happen. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1840307 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/leaseshelper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index 02759f2314..c4258cae4e 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -160,7 +160,7 @@ main(int argc, char **argv) pid_file = g_strdup(RUNSTATEDIR "/leaseshelper.pid"); /* Try to claim the pidfile, exiting if we can't */ - if ((pid_file_fd = virPidFileAcquirePath(pid_file, false, getpid())) < 0) + if ((pid_file_fd = virPidFileAcquirePath(pid_file, true, getpid())) < 0) goto cleanup; /* Since interfaces can be hot plugged, we need to make sure that the -- 2.26.2

Some functions or code paths that may fail don't report error (e.g. when acquiring PID file fails) leading to a silent quit of the leaseshelper. This makes it super hard for us and users to debug what is happening. Fortunately, dnsmasq captures both stdout and stderr so we can write an error message there. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/leaseshelper.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index c4258cae4e..947f838126 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -131,8 +131,10 @@ main(int argc, char **argv) * events for expired leases. So, libvirtd sets another env var for this * purpose */ if (!interface && - !(interface = getenv("VIR_BRIDGE_NAME"))) - goto cleanup; + !(interface = getenv("VIR_BRIDGE_NAME"))) { + fprintf(stderr, _("interface not set\n")); + exit(EXIT_FAILURE); + } ip = argv[3]; mac = argv[2]; @@ -160,13 +162,21 @@ main(int argc, char **argv) pid_file = g_strdup(RUNSTATEDIR "/leaseshelper.pid"); /* Try to claim the pidfile, exiting if we can't */ - if ((pid_file_fd = virPidFileAcquirePath(pid_file, true, getpid())) < 0) + if ((pid_file_fd = virPidFileAcquirePath(pid_file, true, getpid())) < 0) { + fprintf(stderr, + _("Unable to acquire PID file: %s\n errno=%d"), + pid_file, errno); goto cleanup; + } /* Since interfaces can be hot plugged, we need to make sure that the * corresponding custom lease file exists. If not, 'touch' it */ - if (virFileTouch(custom_lease_file, 0644) < 0) + if (virFileTouch(custom_lease_file, 0644) < 0) { + fprintf(stderr, + _("Unable to create: %s\n errno=%d"), + custom_lease_file, errno); goto cleanup; + } switch ((enum virLeaseActionFlags) action) { case VIR_LEASE_ACTION_ADD: -- 2.26.2