10:03 a.m.
It's been a long-standing problem that when you stop and restart a
libvirt network, the guest tap devices are no longer connected to the
network. Until now the only way to recover from this was to either
shutdown and restart all the affected guests, or to detach and then
re-attach all affected guest network devices.
These patches permit the situation to be remedied by just restarting
libvirtd - when the hypervisor drivers are notifying the network
drivers of each connection, the network can retrieve the current
"master" for the tap device and compare it to the network's bridge
device. If they don't match, then it disconnects the tap from any
incorrect bridge and connects to the correct bridge.
***
For now, that's as far as we go - *semi*-automated (since you need to
restart libvirtd for it to happen). My intent is for this to be
completely automated, but the logic to do that is a bit "convoluted"
so I've deferred trying to implement it until I've given it more
thought - a few different trains of thought have led to dead-ends, and
so far only one seems reasonably doable:
* add a networkStartCallback list to the network driver state object.
* as each hypervisor driver that uses the network driver is
initialized, it will call an internal-only function in the network
driver to register a callback.
* whenever a network is started, it will call all the registered
callback functions (sending the name of the network being started,
and some HV-specific context pointer).
* Each hypervisor's callback function will look through all of its
active domains for interfaces that are using the given network, and
for each such interface, will call networkNotifyActualDevice() (the
function that has been updated in Patch 3/3 to reconnect taps to
bridges).
The "convoluted" part here is that we need to make sure there is
enough (but not too much!) locking/refcounting both when adding items
to the callback list (which should only be done single threaded, since
it happens during the driver initializations) and when the
networkStart() function (in some state of lockedness/refcountedness)
calls over to some e.g. qemu function that will then need to do some
amount of locking (at least on each domain as it is processed) and
then calls back into the network driver (which will need "some amount"
of locking/refcounting). Since we're calling from network driver into
qemu into network driver, while the normal nesting is just calling
from qemu into network, I want to be sure there is no possibility for
a deadlock. (Also, I want to avoid making the list of callbacks any
more complicated than absolutely necessary - the "in" thing to do
these days seems to be to allocate a hash table, but there's a lot of
extra code required for that (see util/virclosecallbacks.[hc]) which
seems like overkill for a list of maximum 4-5 items that will *never*
change after the driver initialization - if anyone has ideas/opinions
about that, please speak up.
Laine Stump (2):
util: new function virNetDevGetMaster()
util: new function virNetDevTapAttachBridge()
root (1):
network: reconnect tap devices during networkNotifyActualDevice
src/libvirt_private.syms | 2 +
src/network/bridge_driver.c | 30 +++++++++++-
src/util/virnetdev.c | 49 +++++++++++++++++++
src/util/virnetdev.h | 3 ++
src/util/virnetdevtap.c | 111 +++++++++++++++++++++++++++++---------------
src/util/virnetdevtap.h | 12 +++++
6 files changed, 169 insertions(+), 38 deletions(-)
--
2.9.3
10:03 a.m.
New subject: [libvirt] [PATCH 1/3] util: new function virNetDevGetMaster()
This function provides the bridge/bond device that the given network
device is attached to. The return value is 0 or -1, and the master
device is a char** argument to the function - this is needed in order
to allow for a "success" return from a device that has no master.
---
src/libvirt_private.syms | 1 +
src/util/virnetdev.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virnetdev.h | 3 +++
3 files changed, 53 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c96fe2a..1d783b6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1994,6 +1994,7 @@ virNetDevGetFeatures;
virNetDevGetIndex;
virNetDevGetLinkInfo;
virNetDevGetMAC;
+virNetDevGetMaster;
virNetDevGetMTU;
virNetDevGetName;
virNetDevGetOnline;
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 0d19432..756dcdd 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -931,6 +931,55 @@ int virNetDevGetIndex(const char *ifname ATTRIBUTE_UNUSED,
#endif /* ! SIOCGIFINDEX */
+#if defined(__linux__) && defined(HAVE_LIBNL)
+/**
+ * virNetDevGetMaster:
+ * @ifname: name of interface we're interested in
+ * @master: used to return a string containing the name of @ifname's
"master"
+ * (this is the bridge or bond device that this device is attached to)
+ *
+ * Returns 0 on success, -1 on failure (if @ifname has no master
+ * @master will be NULL, but return value will still be 0 (success)).
+ */
+int
+virNetDevGetMaster(const char *ifname, char **master)
+{
+ int ret = -1;
+ void *nlData = NULL;
+ struct nlattr *tb[IFLA_MAX + 1] = {NULL, };
+
+ *master = NULL;
+
+ if (virNetlinkDumpLink(ifname, -1, &nlData, tb, 0, 0) < 0)
+ goto cleanup;
+
+ if (tb[IFLA_MASTER]) {
+ if (!(*master = virNetDevGetName(*(int *)RTA_DATA(tb[IFLA_MASTER]))))
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(nlData);
+ return ret;
+}
+
+
+#else
+
+
+int
+virNetDevGetMaster(const char *ifname, char **master)
+{
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to get device master from netlink on this
platform"));
+ return -1;
+}
+
+
+#endif /* defined(__linux__) && defined(HAVE_LIBNL) */
+
+
#if defined(SIOCGIFVLAN) && defined(HAVE_STRUCT_IFREQ) &&
HAVE_DECL_GET_VLAN_VID_CMD
int virNetDevGetVLanID(const char *ifname, int *vlanid)
{
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 0551af6..9da5d85 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -165,6 +165,9 @@ int virNetDevGetIndex(const char *ifname, int *ifindex)
int virNetDevGetVLanID(const char *ifname, int *vlanid)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+int virNetDevGetMaster(const char *ifname, char **master)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+
int virNetDevValidateConfig(const char *ifname,
const virMacAddr *macaddr, int ifindex)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
--
2.9.3
10:03 a.m.
New subject: [libvirt] [PATCH 2/3] util: new function virNetDevTapAttachBridge()
This patch splits out the part of virNetDevTapCreateInBridgePort()
that would need to be re-done if an existing tap device had to be
re-attached to a bridge, and puts it into a separate function. This
can be used both when an existing domain interface config is updated
to change its connection, and also to re-attach to the "same" bridge
when a network has been stopped and restarted. So far it is used for
nothing.
---
src/libvirt_private.syms | 1 +
src/util/virnetdevtap.c | 111 +++++++++++++++++++++++++++++++----------------
src/util/virnetdevtap.h | 12 +++++
3 files changed, 87 insertions(+), 37 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1d783b6..905d55a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2106,6 +2106,7 @@ virNetDevOpenvswitchSetTimeout;
# util/virnetdevtap.h
+virNetDevTapAttachBridge;
virNetDevTapCreate;
virNetDevTapCreateInBridgePort;
virNetDevTapDelete;
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index 41e68f4..02ef7fd 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -505,6 +505,77 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED,
/**
+ * virNetDevTapAttachBridge:
+ * @tapname: the tap interface name (or name template)
+ * @brname: the bridge name
+ * @macaddr: desired MAC address
+ * @virtPortProfile: bridge/port specific configuration
+ * @virtVlan: vlan tag info
+ * @mtu: requested MTU for port (or 0 for "default")
+ * @actualMTU: MTU actually set for port (after accounting for bridge's MTU)
+ *
+ * This attaches an existing tap device (@tapname) to a bridge
+ * (@brname).
+ *
+ * Returns 0 in case of success or -1 on failure
+ */
+int
+virNetDevTapAttachBridge(const char *tapname,
+ const char *brname,
+ const virMacAddr *macaddr,
+ const unsigned char *vmuuid,
+ virNetDevVPortProfilePtr virtPortProfile,
+ virNetDevVlanPtr virtVlan,
+ unsigned int mtu,
+ unsigned int *actualMTU)
+{
+ /* If an MTU is specified for the new device, set it before
+ * attaching the device to the bridge, as it may affect the MTU of
+ * the bridge (in particular if it is the first device attached to
+ * the bridge, or if it is smaller than the current MTU of the
+ * bridge). If MTU isn't specified for the new device (i.e. 0),
+ * we need to set the interface MTU to the current MTU of the
+ * bridge (to avoid inadvertantly changing the bridge's MTU).
+ */
+ if (mtu > 0) {
+ if (virNetDevSetMTU(tapname, mtu) < 0)
+ goto error;
+ } else {
+ if (virNetDevSetMTUFromDevice(tapname, brname) < 0)
+ goto error;
+ }
+ if (actualMTU) {
+ int retMTU = virNetDevGetMTU(tapname);
+
+ if (retMTU < 0)
+ goto error;
+
+ *actualMTU = retMTU;
+ }
+
+
+ if (virtPortProfile) {
+ if (virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {
+ if (virNetDevMidonetBindPort(tapname, virtPortProfile) < 0)
+ goto error;
+ } else if (virtPortProfile->virtPortType ==
VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
+ if (virNetDevOpenvswitchAddPort(brname, tapname, macaddr, vmuuid,
+ virtPortProfile, virtVlan) < 0)
+ goto error;
+ }
+ } else {
+ if (virNetDevBridgeAddPort(brname, tapname) < 0)
+ goto error;
+ }
+
+ return 0;
+
+ error:
+ return -1;
+}
+
+
+/**
* virNetDevTapCreateInBridgePort:
* @brname: the bridge name
* @ifname: the interface name (or name template)
@@ -582,43 +653,9 @@ int virNetDevTapCreateInBridgePort(const char *brname,
if (virNetDevSetMAC(*ifname, &tapmac) < 0)
goto error;
- /* If an MTU is specified for the new device, set it before
- * attaching the device to the bridge, as it may affect the MTU of
- * the bridge (in particular if it is the first device attached to
- * the bridge, or if it is smaller than the current MTU of the
- * bridge). If MTU isn't specified for the new device (i.e. 0),
- * we need to set the interface MTU to the current MTU of the
- * bridge (to avoid inadvertantly changing the bridge's MTU).
- */
- if (mtu > 0) {
- if (virNetDevSetMTU(*ifname, mtu) < 0)
- goto error;
- } else {
- if (virNetDevSetMTUFromDevice(*ifname, brname) < 0)
- goto error;
- }
- if (actualMTU) {
- int retMTU = virNetDevGetMTU(*ifname);
-
- if (retMTU < 0)
- goto error;
-
- *actualMTU = retMTU;
- }
-
-
- if (virtPortProfile) {
- if (virtPortProfile->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) {
- if (virNetDevMidonetBindPort(*ifname, virtPortProfile) < 0)
- goto error;
- } else if (virtPortProfile->virtPortType ==
VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
- if (virNetDevOpenvswitchAddPort(brname, *ifname, macaddr, vmuuid,
- virtPortProfile, virtVlan) < 0)
- goto error;
- }
- } else {
- if (virNetDevBridgeAddPort(brname, *ifname) < 0)
- goto error;
+ if (virNetDevTapAttachBridge(*ifname, brname, macaddr, vmuuid,
+ virtPortProfile, virtVlan, mtu, actualMTU) < 0) {
+ goto error;
}
if (virNetDevSetOnline(*ifname, !!(flags & VIR_NETDEV_TAP_CREATE_IFUP)) < 0)
diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index 6441df4..6bb3b88 100644
--- a/src/util/virnetdevtap.h
+++ b/src/util/virnetdevtap.h
@@ -62,6 +62,18 @@ typedef enum {
VIR_NETDEV_TAP_CREATE_PERSIST = 1 << 3,
} virNetDevTapCreateFlags;
+int
+virNetDevTapAttachBridge(const char *tapname,
+ const char *brname,
+ const virMacAddr *macaddr,
+ const unsigned char *vmuuid,
+ virNetDevVPortProfilePtr virtPortProfile,
+ virNetDevVlanPtr virtVlan,
+ unsigned int mtu,
+ unsigned int *actualMTU)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+ ATTRIBUTE_RETURN_CHECK;
+
int virNetDevTapCreateInBridgePort(const char *brname,
char **ifname,
const virMacAddr *macaddr,
--
2.9.3
10:04 a.m.
New subject: [libvirt] [PATCH 3/3] network: reconnect tap devices during networkNotifyActualDevice
From: root <root(a)vhost2.laine.org>
If a network is destroyed and restarted, or its bridge is changed, any
tap devices that had previously been connected to the bridge will no
longer be connected. As a first step in automating a reconnection of
all tap devices when this happens, this patch modifies
networkNotifyActualDevice() (which is called once for every
<interface> of every active domain whenever libvirtd is restarted) to
reconnect any tap devices that it finds disconnected.
With this patch in place, you will need to restart libvirtd to
reconnect all the taps to their proper bridges. A future patch will
add a callback that hypervisor drivers can register with the network
driver to that the network driver can trigger this behavior
automatically whenever a network is started.
---
src/network/bridge_driver.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 32c5ab7..6f2d03a 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4605,7 +4605,8 @@ networkAllocateActualDevice(virDomainDefPtr dom,
* Called to notify the network driver when libvirtd is restarted and
* finds an already running domain. If appropriate it will force an
* allocation of the actual->direct.linkdev to get everything back in
- * order.
+ * order, or re-attach the interface's tap device to the network's
+ * bridge.
*
* Returns 0 on success, -1 on failure.
*/
@@ -4620,6 +4621,7 @@ networkNotifyActualDevice(virDomainDefPtr dom,
virNetworkForwardIfDefPtr dev = NULL;
size_t i;
int ret = -1;
+ char *master = NULL;
if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
return 0;
@@ -4644,6 +4646,31 @@ networkNotifyActualDevice(virDomainDefPtr dom,
netdef->bridge) < 0))
goto error;
+ /* see if we're connected to the correct bridge */
+ if (netdef->bridge) {
+ if (virNetDevGetMaster(iface->ifname, &master) < 0)
+ goto error;
+
+ if (STRNEQ_NULLABLE(netdef->bridge, master)) {
+ /* disconnect from current (incorrect) bridge */
+ if (master)
+ ignore_value(virNetDevBridgeRemovePort(master, iface->ifname));
+
+ /* attach/reattach to correct bridge.
+ * NB: we can't notify the guest of any MTU change anyway,
+ * so there is no point in trying to learn the actualMTU
+ * (final arg to virNetDevTapAttachBridge())
+ */
+ if (virNetDevTapAttachBridge(iface->ifname, netdef->bridge,
+ &iface->mac, dom->uuid,
+ virDomainNetGetActualVirtPortProfile(iface),
+ virDomainNetGetActualVlan(iface),
+ iface->mtu, NULL) < 0) {
+ goto error;
+ }
+ }
+ }
+
if (!iface->data.network.actual ||
(actualType != VIR_DOMAIN_NET_TYPE_DIRECT &&
actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV)) {
@@ -4776,6 +4803,7 @@ networkNotifyActualDevice(virDomainDefPtr dom,
ret = 0;
cleanup:
virNetworkObjEndAPI(&network);
+ VIR_FREE(master);
return ret;
error:
--
2.9.3
10:32 a.m.
On 03/21/2017 04:03 PM, Laine Stump wrote:
It's been a long-standing problem that when you stop and restart
a
libvirt network, the guest tap devices are no longer connected to the
network. Until now the only way to recover from this was to either
shutdown and restart all the affected guests, or to detach and then
re-attach all affected guest network devices.
These patches permit the situation to be remedied by just restarting
libvirtd - when the hypervisor drivers are notifying the network
drivers of each connection, the network can retrieve the current
"master" for the tap device and compare it to the network's bridge
device. If they don't match, then it disconnects the tap from any
incorrect bridge and connects to the correct bridge.
***
For now, that's as far as we go - *semi*-automated (since you need to
restart libvirtd for it to happen). My intent is for this to be
completely automated, but the logic to do that is a bit "convoluted"
so I've deferred trying to implement it until I've given it more
thought - a few different trains of thought have led to dead-ends, and
so far only one seems reasonably doable:
* add a networkStartCallback list to the network driver state object.
* as each hypervisor driver that uses the network driver is
initialized, it will call an internal-only function in the network
driver to register a callback.
* whenever a network is started, it will call all the registered
callback functions (sending the name of the network being started,
and some HV-specific context pointer).
* Each hypervisor's callback function will look through all of its
active domains for interfaces that are using the given network, and
for each such interface, will call networkNotifyActualDevice() (the
function that has been updated in Patch 3/3 to reconnect taps to
bridges).
The "convoluted" part here is that we need to make sure there is
enough (but not too much!) locking/refcounting both when adding items
to the callback list (which should only be done single threaded, since
it happens during the driver initializations) and when the
networkStart() function (in some state of lockedness/refcountedness)
calls over to some e.g. qemu function that will then need to do some
amount of locking (at least on each domain as it is processed) and
then calls back into the network driver (which will need "some amount"
of locking/refcounting). Since we're calling from network driver into
qemu into network driver, while the normal nesting is just calling
from qemu into network, I want to be sure there is no possibility for
a deadlock. (Also, I want to avoid making the list of callbacks any
more complicated than absolutely necessary - the "in" thing to do
these days seems to be to allocate a hash table, but there's a lot of
extra code required for that (see util/virclosecallbacks.[hc]) which
seems like overkill for a list of maximum 4-5 items that will *never*
change after the driver initialization - if anyone has ideas/opinions
about that, please speak up.
Laine Stump (2):
util: new function virNetDevGetMaster()
util: new function virNetDevTapAttachBridge()
root (1):
network: reconnect tap devices during networkNotifyActualDevice
You might want to reset the owner here ;-)
src/libvirt_private.syms | 2 +
src/network/bridge_driver.c | 30 +++++++++++-
src/util/virnetdev.c | 49 +++++++++++++++++++
src/util/virnetdev.h | 3 ++
src/util/virnetdevtap.c | 111 +++++++++++++++++++++++++++++---------------
src/util/virnetdevtap.h | 12 +++++
6 files changed, 169 insertions(+), 38 deletions(-)
ACK series.
Michal
2833
days inactive
2834
days old
4 comments
2 participants
participants (2)
-
Laine Stump -
Michal Privoznik