Re: [PATCH] network: fix connection usage counts after restart

[Laine Stump <laine@xxxxxxxxx>]

On 9/13/19 10:59 AM, Daniel P. Berrangé wrote:
> Since the introduction of the virNetworkPort object, the network driver
> has a persistent record of ports that have been created against the
> networks. Thus the hypervisor drivers no longer communicate to the
> network driver during libvirtd restart.
>
> This change, however, meant that the connection usage counts were
> no longer re-initialized during a libvirtd restart. To deal with this we
> must iterate over all virNetworkPortDefPtr objects we have and invoke
> the notify callback to record the connection usage count.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>


Reviewed-by: Laine Stump <laine@xxxxxxxxx>


(also tested for direct/bridge, direct/passthrough, hostdev, and normal 
tap-based virtual networks)


There is one issue with this though - it only re-adds connections that 
were in the port list, while previously (before introduction of 
virNetworkPortDef) we had iterated through all interfaces of all active 
domains when libvirtd started - this would catch those interfaces that 
had been "lost" by the network driver when a network with active domains 
was destroyed and then restarted. Now that we're only iterating through 
the list of what the network driver knows about, we're not restoring 
those on libvirtd restart. Of course what we *really* want to have 
happen is for those connections to be restored when the *network* is 
restarted, not require a libvirtd restart (that wasn't done in the past 
because there wasn't any avenue for the network driver to get a list of 
domains/interfaces that *should* be connected to a particular network).

I wonder if maybe we need the NetworkPort list for a network to persist 
in some manner when the network is destroyed...

> ---
>   src/conf/virnetworkobj.c    | 33 +++++++++++++++++++++++++++++++++
>   src/conf/virnetworkobj.h    |  9 +++++++++
>   src/libvirt_private.syms    |  1 +
>   src/network/bridge_driver.c | 17 +++++++++++++++++
>   4 files changed, 60 insertions(+)
>
> diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c
> index d63ead7fac..ca1d598cf9 100644
> --- a/src/conf/virnetworkobj.c
> +++ b/src/conf/virnetworkobj.c
> @@ -1844,6 +1844,39 @@ virNetworkObjPortListExport(virNetworkPtr net,
>   }
>   
>   
> +typedef struct _virNetworkObjPortListForEachData virNetworkObjPortListForEachData;
> +struct _virNetworkObjPortListForEachData {
> +    virNetworkPortListIter iter;
> +    void *opaque;
> +    bool err;
> +};
> +
> +static int
> +virNetworkObjPortForEachCallback(void *payload,
> +                                 const void *name ATTRIBUTE_UNUSED,
> +                                 void *opaque)
> +{
> +    virNetworkObjPortListForEachData *data = opaque;
> +
> +    if (!data->iter(payload, data->opaque))
> +        data->err = true;
> +
> +    return 0;
> +}
> +
> +int
> +virNetworkObjPortForEach(virNetworkObjPtr obj,
> +                         virNetworkPortListIter iter,
> +                         void *opaque)
> +{
> +    virNetworkObjPortListForEachData data = { iter, opaque, false };
> +    virHashForEach(obj->ports, virNetworkObjPortForEachCallback, &data);
> +    if (data.err)
> +        return -1;
> +    return 0;
> +}
> +
> +
>   static int
>   virNetworkObjLoadAllPorts(virNetworkObjPtr net,
>                             const char *stateDir)
> diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h
> index 1c28f0888c..a91b4304c6 100644
> --- a/src/conf/virnetworkobj.h
> +++ b/src/conf/virnetworkobj.h
> @@ -190,6 +190,15 @@ virNetworkObjPortListExport(virNetworkPtr net,
>                               virNetworkPortPtr **ports,
>                               virNetworkPortListFilter filter);
>   
> +typedef bool
> +(*virNetworkPortListIter)(virNetworkPortDefPtr portdef,
> +                          void *opaque);
> +
> +int
> +virNetworkObjPortForEach(virNetworkObjPtr obj,
> +                         virNetworkPortListIter iter,
> +                         void *opaque);
> +
>   int
>   virNetworkObjSaveStatus(const char *statusDir,
>                           virNetworkObjPtr net,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7fe10d2286..37afb07e21 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1097,6 +1097,7 @@ virNetworkObjLookupPort;
>   virNetworkObjMacMgrAdd;
>   virNetworkObjMacMgrDel;
>   virNetworkObjNew;
> +virNetworkObjPortForEach;
>   virNetworkObjPortListExport;
>   virNetworkObjRemoveInactive;
>   virNetworkObjReplacePersistentDef;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 7b44184616..0fee153cb8 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -527,6 +527,21 @@ networkBridgeDummyNicName(const char *brname)
>   }
>   
>   
> +static int
> +networkNotifyPort(virNetworkObjPtr obj,
> +                  virNetworkPortDefPtr port);
> +
> +static bool
> +networkUpdatePort(virNetworkPortDefPtr port,
> +                  void *opaque)
> +{
> +    virNetworkObjPtr obj = opaque;
> +
> +    networkNotifyPort(obj, port);
> +
> +    return false;
> +}
> +
>   static int
>   networkUpdateState(virNetworkObjPtr obj,
>                      void *opaque)
> @@ -591,6 +606,8 @@ networkUpdateState(virNetworkObjPtr obj,
>           goto cleanup;
>       }
>   
> +    virNetworkObjPortForEach(obj, networkUpdatePort, obj);
> +
>       /* Try and read dnsmasq/radvd pids of active networks */
>       if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) {
>           pid_t radvdPid;


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list