[libvirt PATCH 1/2] network: don't wait for IPv6 DAD completion when starting a network

[Daniel P. Berrangé]

On Fri, Sep 04, 2020 at 02:01:14PM -0400, Laine Stump wrote:
> 0f7436ca54 added code during virtual network startup to wait for DAD
> (Duplicate Address Detection) to complete if there were any IPv6
> addresses on the network. This wait was needed because (according to
> the commit log) "created problems when [the "dummy" tap device] is set
> to IFF_DOWN prior to DAD completing".
> 
> That commit in turn referenced commit db488c7917, which had added the
> code to set the dummy tap device IFF_DOWN, commenting "DAD has
> happened (dnsmasq waits for it)", and in its commit message pointed
> out that if we just got rid of the dummy tap device this wouldn't be
> needed.
> 
> Now that the dummy tap device has indeed been removed (commit
> ee6c936fbb), there is no longer any need to set it IFF_DOWN, and thus
> nothing requiring us to wait for DAD to complete. At any rate, with
> the dummy tap device removed, leaving nothing else on the bridge when
> it is first started, DAD never completes, leading to failure to start
> any IPv6 network.
> 
> So, yes, this patch removes the wait for DAD completion, and IPv6
> networks can once again start, and their associated dnsmasq process
> starts successfully (this is the problem that the DAD wait was
> originally intended to fix)

I'll note that  when the virtual network first starts, the IPv6
address is shown as "global tentative" and "netstat -u" does not
show any dnsmasq listening on the IPv6 address.

Once I start the first guest, then it removes  "tentative" state
and dnsmasq is now listening on IPv6 address.

In contrast wth the old patch, the "tentative" state is gone
immediately and it listens on IPv6.

IIUC, the removal "tenative" signals that DAD is complete.

IOW, with our new setup DAD is not complete until the first
guest runs.

This difference in behaviour doesn't seem to have any functional
problem. I can start the guest, it gets IPv6 connectivity and
can reach the interwebs fine.

The only nagging feeling in the back of my mind is whether the
the original dnsmasq problems with DAD were in fact solved by
some fix in dnsmasq, and the magic kernel behaviour wrt to a
bridge with no NICs is a distraction.

Anyway, I feel we can stick with this and revisit if anyone
complains later.

> 
> Signed-off-by: Laine Stump <laine at redhat.com>
> ---
>  src/network/bridge_driver.c | 32 --------------------------------
>  1 file changed, 32 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 5c00befc16..87d7acab06 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2286,32 +2286,6 @@ networkAddRouteToBridge(virNetworkObjPtr obj,
>      return 0;
>  }
>  
> -static int
> -networkWaitDadFinish(virNetworkObjPtr obj)
> -{
> -    virNetworkDefPtr def = virNetworkObjGetDef(obj);
> -    virNetworkIPDefPtr ipdef;
> -    g_autofree virSocketAddrPtr *addrs = NULL;
> -    virSocketAddrPtr addr = NULL;
> -    size_t naddrs = 0;
> -    int ret = -1;
> -
> -    VIR_DEBUG("Begin waiting for IPv6 DAD on network %s", def->name);
> -
> -    while ((ipdef = virNetworkDefGetIPByIndex(def, AF_INET6, naddrs))) {
> -        addr = &ipdef->address;
> -        if (VIR_APPEND_ELEMENT_COPY(addrs, naddrs, addr) < 0)
> -            goto cleanup;
> -    }
> -
> -    ret = (naddrs == 0) ? 0 : virNetDevIPWaitDadFinish(addrs, naddrs);
> -
> - cleanup:
> -    VIR_DEBUG("Finished waiting for IPv6 DAD on network %s with status %d",
> -              def->name, ret);
> -    return ret;
> -}
> -
>  
>  static int
>  networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
> @@ -2444,12 +2418,6 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>      if (v6present && networkStartRadvd(driver, obj) < 0)
>          goto error;
>  
> -    /* dnsmasq does not wait for DAD to complete before daemonizing,
> -     * so we need to wait for it ourselves.
> -     */
> -    if (v6present && networkWaitDadFinish(obj) < 0)
> -        goto error;
> -
>      if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
>          goto error;

Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|