Re: [libvirt] [RFC/PATCH 1/2] network: add dnsmasq option 'dhcp-authoritative'
On 09/21/2016 04:49 AM, Martin Wilck wrote:
The dnsmasq man page recommends that dhcp-authoritative "should
be
set when dnsmasq is definitely the only DHCP server on a network".
This is the case for libvirt-managed virtual networks.
The effect of this is that VMs that fail to renew their DHCP lease
in time (e.g. if the VM or host is suspended) will be able to
re-acquire the lease even if it's expired, unless the IP address has
been taken by some other host. This avoids various annoyances caused
by changing VM IP addresses.
We had earlier (finally!) agreed in principle on pushing this patch, but
were in freeze at the time, and I later forgot to do it.
I checked to be sure that the dnsmasq on one of the oldest Linux distros
that we still support does have the dhcp-authoritative option. It does
(it's dnsmasq-2.48-something) so we don't need any extra code to verify
the option is there.
I do have a couple comments below, so don't hit the "next" button yet! :-)
---
src/network/bridge_driver.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 7b99aca..cb4fb1c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1258,8 +1258,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
/* Note: the following is IPv4 only */
if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) {
- if (ipdef->nranges || ipdef->nhosts)
+ if (ipdef->nranges || ipdef->nhosts) {
virBufferAddLit(&configbuf, "dhcp-no-override\n");
+ virBufferAddLit(&configbuf, "dhcp-authoritative\n");
+ }
You accidentally got a tab character in the line below, so I replaced it
with spaces so that make syntax-check passes.
if (ipdef->tftproot) {
virBufferAddLit(&configbuf, "enable-tftp\n");
Although your cover letter included diff statistics for the
networkxml2conftest test cases that needed the extra line for
dhcp-authoritative, those diffs weren't included in the patch itself.
Fortunately it's simple to regenerate the test case outputs
(VIR_TEST_REGENERATE_OUTPUT=1 tests/networkxml2conftest), so I
regenerated and verified them.
ACK, and pushed. Thanks for the submission, and sorry for the delay (and
the seemingly protracted debate about a single line - we've been burned
in the past by unforeseen consequences of adding a seemingly innocuous
dnsmasq option to our dnsmasq conf files, and I was nervous about that
happening again, so I wanted to make sure we explored all possibilities.)