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

[Laine Stump]

On 9/13/19 12:38 PM, Daniel P. Berrangé wrote:
> On Fri, Sep 13, 2019 at 12:10:34PM -0400, Laine Stump wrote:
>> 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 at redhat.com>
>>
>> Reviewed-by: Laine Stump <laine at laine.org>
>>
>>
>> (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).
> The restart problem is with this code:
>
> void
> virDomainNetNotifyActualDevice(virConnectPtr conn,
>                                 virDomainDefPtr dom,
>                                 virDomainNetDefPtr iface)
> {
>      if (!virUUIDIsValid(iface->data.network.portid)) {
>          if (virDomainNetCreatePort(conn, dom, iface,
>                                     VIR_NETWORK_PORT_CREATE_RECLAIM) < 0)
>              return;
>      }
>
>      ....
> }
>
>
> we're assuming the portid exists if we have it recorded. This is not
> valid if the network has been destroyed and recreated.
>
> We could easily make this check if the port needs re-creating
> which will fix restart handling.


The other problem is that directly below this code, we're checking only 
for TYPE_BRIDGE, but not for TYPE_NETWORK, which is also needed since we 
found out that we have to preserve TYPE_NETWORK when it's a tap-based 
virtual network connected to a bridge. (I was about to test a patch 
doing this, hadn't paid attention to the fact that the call to 
virDomainNetCreatePort() would fail...)


> I'm in two minds about the idea of preserving ports across restart
> of the network. If anything I wish we simply rejected restarts
> with OPERATION_INVALID if anything is connected still, but that
> might be too annoying for people who've come to rely on this
> hack.


Yeah, if we had forbidden it from the very beginning then it would be 
one thing, but it's been allowed since the very beginning, and 
especially before the ability to modify a running network was added it 
was taken advantage of a lot, e.g. by people who wanted to update their 
static dhcp host list without needing to shutdown every single guest.


It really would be cool if all the ports connected to a network were 
simply unreachable while a network was down (and optionally the guests 
were told that the cable was disconnected). That would require an API in 
the opposite direction of everything we have though (maybe it could be 
accomplished if the network driver just knew the name of the tap device 
for each networkport...)