9:59 a.m.
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(a)redhat.com>
---
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;
--
2.21.0
11:10 a.m.
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(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)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).
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;
11:38 a.m.
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(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)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.
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.
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 :|
11:49 a.m.
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(a)redhat.com>
>
> Reviewed-by: Laine Stump <laine(a)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...)
1936
days inactive
1936
days old
3 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Laine Stump