[libvirt] [PATCH 0/3] fix bad error log on libvirtd restart when using OVS
This BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1596176 and the log message of patch 3 explain the problem. Laine Stump (3): util: new function virNetDevOpenvswitchInterfaceGetMaster() util: add some debug log to virNetDevGetMaster network: properly check for taps that are connected to an OVS bridge src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 21 ++++++++++++++-- src/util/virnetdev.c | 1 + src/util/virnetdevopenvswitch.c | 55 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 6 +++++ 5 files changed, 82 insertions(+), 2 deletions(-) -- 2.14.4
This function retrieves the name of the OVS bridge that the given netdev is attached to. This separate function is necessary because OVS set the IFLA_MASTER attribute to "ovs-system" for all netdevs that are attached to an OVS bridge, so the standard method of retrieving the master can't be used. Signed-off-by: Laine Stump <laine@laine.org> --- src/libvirt_private.syms | 1 + src/util/virnetdevopenvswitch.c | 55 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 6 +++++ 3 files changed, 62 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 98913a577a..386f53eeb9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2364,6 +2364,7 @@ virNetDevMidonetUnbindPort; virNetDevOpenvswitchAddPort; virNetDevOpenvswitchGetMigrateData; virNetDevOpenvswitchGetVhostuserIfname; +virNetDevOpenvswitchInterfaceGetMaster; virNetDevOpenvswitchInterfaceStats; virNetDevOpenvswitchRemovePort; virNetDevOpenvswitchSetMigrateData; diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index f86f698430..af3f2a773d 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -404,6 +404,61 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname, return ret; } + +/** + * virNetDeOpenvswitchGetMaster: + * @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)). + * + * NB: This function is needed because the IFLA_MASTER attribute of an + * interface in a netlink dump (see virNetDevGetMaster()) will always + * return "ovs-system" for any interface that is attached to an OVS + * switch. When that happens, virNetDevOpenvswitchInterfaceGetMaster() + * must be called to get the "real" master of the interface. + */ +int +virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master) +{ + virCommandPtr cmd = NULL; + int ret = -1; + int exitstatus; + + *master = NULL; + cmd = virCommandNew(OVSVSCTL); + virNetDevOpenvswitchAddTimeout(cmd); + virCommandAddArgList(cmd, "iface-to-br", ifname, NULL); + virCommandSetOutputBuffer(cmd, master); + + if (virCommandRun(cmd, &exitstatus) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to run command to get OVS master for " + "interface %s"), ifname); + goto cleanup; + } + + /* non-0 exit code just means that the interface has no master in OVS */ + if (exitstatus != 0) + VIR_FREE(*master); + + if (*master) { + /* truncate at the first newline */ + char *nl = strchr(*master, '\n'); + if (nl) + *nl = '\0'; + } + + VIR_DEBUG("OVS master for %s is %s", ifname, *master ? *master : "(none)"); + + ret = 0; + cleanup: + return ret; +} + + /** * virNetDevOpenvswitchVhostuserGetIfname: * @path: the path of the unix socket diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 6f6e620c22..331e483018 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -47,6 +47,9 @@ int virNetDevOpenvswitchAddPort(const char *brname, int virNetDevOpenvswitchRemovePort(const char *brname, const char *ifname) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; @@ -57,6 +60,9 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname, virDomainInterfaceStatsPtr stats) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + int virNetDevOpenvswitchGetVhostuserIfname(const char *path, char **ifname) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE; -- 2.14.4
On 07/02/2018 01:46 AM, Laine Stump wrote:
This function retrieves the name of the OVS bridge that the given netdev is attached to. This separate function is necessary because OVS set the IFLA_MASTER attribute to "ovs-system" for all netdevs that are attached to an OVS bridge, so the standard method of retrieving the master can't be used.
Signed-off-by: Laine Stump <laine@laine.org> --- src/libvirt_private.syms | 1 + src/util/virnetdevopenvswitch.c | 55 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 6 +++++ 3 files changed, 62 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 98913a577a..386f53eeb9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2364,6 +2364,7 @@ virNetDevMidonetUnbindPort; virNetDevOpenvswitchAddPort; virNetDevOpenvswitchGetMigrateData; virNetDevOpenvswitchGetVhostuserIfname; +virNetDevOpenvswitchInterfaceGetMaster; virNetDevOpenvswitchInterfaceStats; virNetDevOpenvswitchRemovePort; virNetDevOpenvswitchSetMigrateData; diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index f86f698430..af3f2a773d 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -404,6 +404,61 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname, return ret; }
+ +/** + * virNetDeOpenvswitchGetMaster: + * @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)). + * + * NB: This function is needed because the IFLA_MASTER attribute of an + * interface in a netlink dump (see virNetDevGetMaster()) will always + * return "ovs-system" for any interface that is attached to an OVS + * switch. When that happens, virNetDevOpenvswitchInterfaceGetMaster() + * must be called to get the "real" master of the interface. + */ +int +virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master) +{ + virCommandPtr cmd = NULL; + int ret = -1; + int exitstatus; + + *master = NULL; + cmd = virCommandNew(OVSVSCTL);
I'd put an empty line in between these two ^^ because from data POV we have two different blocks (one initializes variables, the other constructs @cmd). But it is really a nit pick.
+ virNetDevOpenvswitchAddTimeout(cmd); + virCommandAddArgList(cmd, "iface-to-br", ifname, NULL); + virCommandSetOutputBuffer(cmd, master); + + if (virCommandRun(cmd, &exitstatus) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to run command to get OVS master for " + "interface %s"), ifname); + goto cleanup; + } + + /* non-0 exit code just means that the interface has no master in OVS */ + if (exitstatus != 0) + VIR_FREE(*master);
A-ha! We indeed need to do this. I was under impression that just like other functions of ours if they fail no allocation is done. But this is different so we need to call VIR_FREE(). Michal
On 07/02/2018 02:11 AM, Michal Prívozník wrote:
On 07/02/2018 01:46 AM, Laine Stump wrote:
+ virNetDevOpenvswitchAddTimeout(cmd); + virCommandAddArgList(cmd, "iface-to-br", ifname, NULL); + virCommandSetOutputBuffer(cmd, master); + + if (virCommandRun(cmd, &exitstatus) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to run command to get OVS master for " + "interface %s"), ifname); + goto cleanup; + } + + /* non-0 exit code just means that the interface has no master in OVS */ + if (exitstatus != 0) + VIR_FREE(*master); A-ha! We indeed need to do this. I was under impression that just like other functions of ours if they fail no allocation is done. But this is different so we need to call VIR_FREE().
Right. If ovs-vsctl is run and returns non-0, there still might have been some sort of output to stdout (or there might not have been, but I don't want to rely on it).
This makes it easier to see why libvirt has decided it must re-attach a tap device to its bridge. Signed-off-by: Laine Stump <laine@laine.org> --- src/util/virnetdev.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index b250af9e2c..c20022fbc9 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -977,6 +977,7 @@ virNetDevGetMaster(const char *ifname, char **master) goto cleanup; } + VIR_DEBUG("IFLA_MASTER for %s is %s", ifname, *master ? *master : "(none)"); ret = 0; cleanup: VIR_FREE(nlData); -- 2.14.4
When libvirtd is restarted, it checks that each guest tap device is still attached to the bridge device that the configuration info says it should be connected to. If not, the tap will be disconnected from [wherever it is] and connected to [wherever it should be]. The previous code that did this did not account for: 1) the IFLA_MASTER attribute in a netdev's ifinfo will be set to "ovs-system" for any tap device connected to an OVS bridge, *not* to the name of the bridge it is attached to. 2) virNetDevRemovePort() only works for devices that are attached to a standard Linux host bridge. If a device is currently attached to an OVS bridge, then virNetDevOpenvswitchRemovePort() must be called instead. The result was an error message like this: error : virNetDevBridgeRemovePort:743 : Unable to remove bridge ovs-system port vnet1: Operation not supported This patch remedies those problems, and adds a couple of information log messages to aid in debugging any future problem. Resolves: https://bugzilla.redhat.com/1596176 Signed-off-by: Laine Stump <laine@laine.org> --- src/network/bridge_driver.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index ac849581ec..da3c32e305 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -64,6 +64,7 @@ #include "virnetdev.h" #include "virnetdevip.h" #include "virnetdevbridge.h" +#include "virnetdevopenvswitch.h" #include "virnetdevtap.h" #include "virnetdevvportprofile.h" #include "virpci.h" @@ -4823,19 +4824,35 @@ networkNotifyActualDevice(virDomainDefPtr dom, /* see if we're connected to the correct bridge */ if (netdef->bridge) { + bool useOVS = false; + if (virNetDevGetMaster(iface->ifname, &master) < 0) goto error; + /* IFLA_MASTER for a tap on an OVS switch is always "ovs-system" */ + if (STREQ_NULLABLE(master, "ovs-system")) { + useOVS = true; + VIR_FREE(master); + if (virNetDevOpenvswitchInterfaceGetMaster(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)); + if (master) { + VIR_INFO("Removing %s from %s", iface->ifname, master); + if (useOVS) + ignore_value(virNetDevOpenvswitchRemovePort(master, iface->ifname)); + else + 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()) */ + VIR_INFO("Attaching %s to %s", iface->ifname, netdef->bridge); if (virNetDevTapAttachBridge(iface->ifname, netdef->bridge, &iface->mac, dom->uuid, virDomainNetGetActualVirtPortProfile(iface), -- 2.14.4
On 07/02/2018 01:46 AM, Laine Stump wrote:
This BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1596176
and the log message of patch 3 explain the problem.
Laine Stump (3): util: new function virNetDevOpenvswitchInterfaceGetMaster() util: add some debug log to virNetDevGetMaster network: properly check for taps that are connected to an OVS bridge
src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 21 ++++++++++++++-- src/util/virnetdev.c | 1 + src/util/virnetdevopenvswitch.c | 55 +++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevopenvswitch.h | 6 +++++ 5 files changed, 82 insertions(+), 2 deletions(-)
ACK series. Michal
participants (2)
-
Laine Stump -
Michal Prívozník