[libvirt] [PATCH v3] network: Bring netdevs online later
Currently, MAC registration occurs during device creation, which is early enough that, during live migration, you end up with duplicate MAC addresses on still-running source and target devices, even though the target device isn't actually being used yet. This patch proposes to defer MAC registration until right before the guest can actually use the device -- In other words, right before starting guest CPUs. Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> --- Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461 Changes for v3: * Some minor formatting fixes. * in qemuNetworkIfaceConnect, set VIR_NETDEV_TAP_CREATE_IFUP unconditionally. * in qemuDomainAttachNetDevice, call qemuInterfaceStartDevice only for VIR_DOMAIN_NET_TYPE_DIRECT, _BRIDGE and _NETWORK. * in qemuProcessStartCPUs, use 'reason' to determine whether or not qemuInterfaceStartDevices needs to be called. Basically, it needs to be called for any reason that the system would be initializing (or re-initializing). src/Makefile.am | 3 +- src/conf/domain_conf.h | 2 ++ src/lxc/lxc_process.c | 4 ++- src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_hotplug.c | 8 +++++ src/qemu/qemu_interface.c | 78 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_interface.h | 32 ++++++++++++++++++ src/qemu/qemu_process.c | 7 ++++ src/util/virnetdevmacvlan.c | 8 +++-- src/util/virnetdevmacvlan.h | 2 ++ 10 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 src/qemu/qemu_interface.c create mode 100644 src/qemu/qemu_interface.h diff --git a/src/Makefile.am b/src/Makefile.am index fa741a8..035120e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_monitor_text.h \ qemu/qemu_monitor_json.c \ qemu/qemu_monitor_json.h \ - qemu/qemu_driver.c qemu/qemu_driver.h + qemu/qemu_driver.c qemu/qemu_driver.h \ + qemu/qemu_interface.c qemu/qemu_interface.h XENAPI_DRIVER_SOURCES = \ xenapi/xenapi_driver.c xenapi/xenapi_driver.h \ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0862bd7..5f328cf 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -951,6 +951,8 @@ struct _virDomainNetDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int linkstate; + /* vmOp value saved if deferring interface start */ + virNetDevVPortProfileOp vmOp; }; /* Used for prefix of ifname of any network name generated dynamically diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index ed30c37..b2256c0 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -300,6 +300,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, virNetDevBandwidthPtr bw; virNetDevVPortProfilePtr prof; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); + unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP; /* XXX how todo bandwidth controls ? * Since the 'net-ifname' is about to be moved to a different @@ -336,7 +337,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, - virDomainNetGetActualBandwidth(net), 0) < 0) + virDomainNetGetActualBandwidth(net), + macvlan_create_flags) < 0) goto cleanup; ret = res_ifname; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e5270bd..229dff4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname = res_ifname; } + /* Save vport profile op for later */ + net->vmOp = vmop; + virObjectUnref(cfg); return rc; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7bc19cd..530e6da 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -30,6 +30,7 @@ #include "qemu_domain.h" #include "qemu_command.h" #include "qemu_hostdev.h" +#include "qemu_interface.h" #include "domain_audit.h" #include "domain_nwfilter.h" #include "virlog.h" @@ -922,6 +923,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, priv->qemuCaps, tapfd, &tapfdSize) < 0) goto cleanup; iface_connected = true; + /* Set device online immediately */ + qemuInterfaceStartDevice(net); if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { @@ -937,6 +940,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) goto cleanup; iface_connected = true; + /* Set device online immediately */ + qemuInterfaceStartDevice(net); if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { @@ -2070,6 +2075,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; } + /* Set device online immediately */ + qemuInterfaceStartDevice(newdev); + newType = virDomainNetGetActualType(newdev); if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c new file mode 100644 index 0000000..dccfcc4 --- /dev/null +++ b/src/qemu/qemu_interface.c @@ -0,0 +1,78 @@ +/* + * qemu_interface.c: QEMU interface management + * + * Copyright IBM Corp. 2014 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#include <config.h> + +#include "qemu_interface.h" +#include "virnetdev.h" +#include "virnetdevtap.h" +#include "virnetdevmacvlan.h" +#include "virnetdevvportprofile.h" + +/** + * qemuInterfaceStartDevice: + * @net: net device to start + * + * Based upon the type of device provided, perform the appropriate + * work to set the device online. + */ +void +qemuInterfaceStartDevice(virDomainNetDefPtr net) +{ + switch (virDomainNetGetActualType(net)) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevTapDelete(net->ifname)); + } + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevVPortProfileDisassociate(net->ifname, + virDomainNetGetActualVirtPortProfile(net), + &net->mac, + virDomainNetGetActualDirectDev(net), + -1, + net->vmOp)); + } + break; + } +} + +/** + * qemuInterfaceStartDevices: + * @def: domain definition + * + * Set all ifaces associated with this domain to the online state. + */ +void +qemuInterfaceStartDevices(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->nnets; i++) { + qemuInterfaceStartDevice(def->nets[i]); + } + + return; +} diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h new file mode 100644 index 0000000..5810cda --- /dev/null +++ b/src/qemu/qemu_interface.h @@ -0,0 +1,32 @@ +/* + * qemu_interface.h: QEMU interface management + * + * Copyright IBM Corp. 2014 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#ifndef __QEMU_INTERFACE_H__ +# define __QEMU_INTERFACE_H__ + +# include "domain_conf.h" + +void qemuInterfaceStartDevice(virDomainNetDefPtr net); +void qemuInterfaceStartDevices(virDomainDefPtr def); + +#endif /* __QEMU_INTERFACE_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f391743..3fc50a0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -42,6 +42,7 @@ #include "qemu_hostdev.h" #include "qemu_hotplug.h" #include "qemu_migration.h" +#include "qemu_interface.h" #include "cpu/cpu.h" #include "datatypes.h" @@ -2947,6 +2948,12 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + /* Bring up netdevs before starting CPUs */ + if (reason != VIR_DOMAIN_RUNNING_UNPAUSED && + reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED) { + qemuInterfaceStartDevices(vm->def); + } + VIR_DEBUG("Using lock state '%s'", NULLSTR(priv->lockState)); if (virDomainLockProcessResume(driver->lockManager, cfg->uri, vm, priv->lockState) < 0) { diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index c83341c..1edf3ae 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -902,9 +902,11 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto link_del_exit; } - if (virNetDevSetOnline(cr_ifname, true) < 0) { - rc = -1; - goto disassociate_exit; + if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP) { + if (virNetDevSetOnline(cr_ifname, true) < 0) { + rc = -1; + goto disassociate_exit; + } } if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 41aa4e2..41b4014 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -44,6 +44,8 @@ typedef enum { VIR_NETDEV_MACVLAN_CREATE_NONE = 0, /* Create with a tap device */ VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, + /* Bring the interface up */ + VIR_NETDEV_MACVLAN_CREATE_IFUP = 1 << 1, } virNetDevMacVLanCreateFlags; int virNetDevMacVLanCreate(const char *ifname, -- 1.7.9.5
On 09/16/2014 04:50 PM, Matthew Rosato wrote:
Currently, MAC registration occurs during device creation, which is early enough that, during live migration, you end up with duplicate MAC addresses on still-running source and target devices, even though the target device isn't actually being used yet. This patch proposes to defer MAC registration until right before the guest can actually use the device -- In other words, right before starting guest CPUs.
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> ---
Ping
Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461
Changes for v3: * Some minor formatting fixes. * in qemuNetworkIfaceConnect, set VIR_NETDEV_TAP_CREATE_IFUP unconditionally. * in qemuDomainAttachNetDevice, call qemuInterfaceStartDevice only for VIR_DOMAIN_NET_TYPE_DIRECT, _BRIDGE and _NETWORK. * in qemuProcessStartCPUs, use 'reason' to determine whether or not qemuInterfaceStartDevices needs to be called. Basically, it needs to be called for any reason that the system would be initializing (or re-initializing).
src/Makefile.am | 3 +- src/conf/domain_conf.h | 2 ++ src/lxc/lxc_process.c | 4 ++- src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_hotplug.c | 8 +++++ src/qemu/qemu_interface.c | 78 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_interface.h | 32 ++++++++++++++++++ src/qemu/qemu_process.c | 7 ++++ src/util/virnetdevmacvlan.c | 8 +++-- src/util/virnetdevmacvlan.h | 2 ++ 10 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 src/qemu/qemu_interface.c create mode 100644 src/qemu/qemu_interface.h
diff --git a/src/Makefile.am b/src/Makefile.am index fa741a8..035120e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_monitor_text.h \ qemu/qemu_monitor_json.c \ qemu/qemu_monitor_json.h \ - qemu/qemu_driver.c qemu/qemu_driver.h + qemu/qemu_driver.c qemu/qemu_driver.h \ + qemu/qemu_interface.c qemu/qemu_interface.h
XENAPI_DRIVER_SOURCES = \ xenapi/xenapi_driver.c xenapi/xenapi_driver.h \ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0862bd7..5f328cf 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -951,6 +951,8 @@ struct _virDomainNetDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int linkstate; + /* vmOp value saved if deferring interface start */ + virNetDevVPortProfileOp vmOp; };
/* Used for prefix of ifname of any network name generated dynamically diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index ed30c37..b2256c0 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -300,6 +300,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, virNetDevBandwidthPtr bw; virNetDevVPortProfilePtr prof; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); + unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
/* XXX how todo bandwidth controls ? * Since the 'net-ifname' is about to be moved to a different @@ -336,7 +337,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, - virDomainNetGetActualBandwidth(net), 0) < 0) + virDomainNetGetActualBandwidth(net), + macvlan_create_flags) < 0) goto cleanup;
ret = res_ifname; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e5270bd..229dff4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname = res_ifname; }
+ /* Save vport profile op for later */ + net->vmOp = vmop; + virObjectUnref(cfg); return rc; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7bc19cd..530e6da 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -30,6 +30,7 @@ #include "qemu_domain.h" #include "qemu_command.h" #include "qemu_hostdev.h" +#include "qemu_interface.h" #include "domain_audit.h" #include "domain_nwfilter.h" #include "virlog.h" @@ -922,6 +923,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, priv->qemuCaps, tapfd, &tapfdSize) < 0) goto cleanup; iface_connected = true; + /* Set device online immediately */ + qemuInterfaceStartDevice(net); if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { @@ -937,6 +940,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) goto cleanup; iface_connected = true; + /* Set device online immediately */ + qemuInterfaceStartDevice(net); if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { @@ -2070,6 +2075,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }
+ /* Set device online immediately */ + qemuInterfaceStartDevice(newdev); + newType = virDomainNetGetActualType(newdev);
if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c new file mode 100644 index 0000000..dccfcc4 --- /dev/null +++ b/src/qemu/qemu_interface.c @@ -0,0 +1,78 @@ +/* + * qemu_interface.c: QEMU interface management + * + * Copyright IBM Corp. 2014 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#include <config.h> + +#include "qemu_interface.h" +#include "virnetdev.h" +#include "virnetdevtap.h" +#include "virnetdevmacvlan.h" +#include "virnetdevvportprofile.h" + +/** + * qemuInterfaceStartDevice: + * @net: net device to start + * + * Based upon the type of device provided, perform the appropriate + * work to set the device online. + */ +void +qemuInterfaceStartDevice(virDomainNetDefPtr net) +{ + switch (virDomainNetGetActualType(net)) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevTapDelete(net->ifname)); + } + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevVPortProfileDisassociate(net->ifname, + virDomainNetGetActualVirtPortProfile(net), + &net->mac, + virDomainNetGetActualDirectDev(net), + -1, + net->vmOp)); + } + break; + } +} + +/** + * qemuInterfaceStartDevices: + * @def: domain definition + * + * Set all ifaces associated with this domain to the online state. + */ +void +qemuInterfaceStartDevices(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->nnets; i++) { + qemuInterfaceStartDevice(def->nets[i]); + } + + return; +} diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h new file mode 100644 index 0000000..5810cda --- /dev/null +++ b/src/qemu/qemu_interface.h @@ -0,0 +1,32 @@ +/* + * qemu_interface.h: QEMU interface management + * + * Copyright IBM Corp. 2014 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#ifndef __QEMU_INTERFACE_H__ +# define __QEMU_INTERFACE_H__ + +# include "domain_conf.h" + +void qemuInterfaceStartDevice(virDomainNetDefPtr net); +void qemuInterfaceStartDevices(virDomainDefPtr def); + +#endif /* __QEMU_INTERFACE_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f391743..3fc50a0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -42,6 +42,7 @@ #include "qemu_hostdev.h" #include "qemu_hotplug.h" #include "qemu_migration.h" +#include "qemu_interface.h"
#include "cpu/cpu.h" #include "datatypes.h" @@ -2947,6 +2948,12 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ /* Bring up netdevs before starting CPUs */ + if (reason != VIR_DOMAIN_RUNNING_UNPAUSED && + reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED) { + qemuInterfaceStartDevices(vm->def); + } + VIR_DEBUG("Using lock state '%s'", NULLSTR(priv->lockState)); if (virDomainLockProcessResume(driver->lockManager, cfg->uri, vm, priv->lockState) < 0) { diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index c83341c..1edf3ae 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -902,9 +902,11 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto link_del_exit; }
- if (virNetDevSetOnline(cr_ifname, true) < 0) { - rc = -1; - goto disassociate_exit; + if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP) { + if (virNetDevSetOnline(cr_ifname, true) < 0) { + rc = -1; + goto disassociate_exit; + } }
if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 41aa4e2..41b4014 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -44,6 +44,8 @@ typedef enum { VIR_NETDEV_MACVLAN_CREATE_NONE = 0, /* Create with a tap device */ VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, + /* Bring the interface up */ + VIR_NETDEV_MACVLAN_CREATE_IFUP = 1 << 1, } virNetDevMacVLanCreateFlags;
int virNetDevMacVLanCreate(const char *ifname,
On Tue, Sep 16, 2014 at 04:50:53PM -0400, Matthew Rosato wrote:
Currently, MAC registration occurs during device creation, which is early enough that, during live migration, you end up with duplicate MAC addresses on still-running source and target devices, even though the target device isn't actually being used yet. This patch proposes to defer MAC registration until right before the guest can actually use the device -- In other words, right before starting guest CPUs.
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> ---
Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461
Changes for v3: * Some minor formatting fixes. * in qemuNetworkIfaceConnect, set VIR_NETDEV_TAP_CREATE_IFUP unconditionally. * in qemuDomainAttachNetDevice, call qemuInterfaceStartDevice only for VIR_DOMAIN_NET_TYPE_DIRECT, _BRIDGE and _NETWORK. * in qemuProcessStartCPUs, use 'reason' to determine whether or not qemuInterfaceStartDevices needs to be called. Basically, it needs to be called for any reason that the system would be initializing (or re-initializing).
To be honest, I must've looked at my previous reviews after reviewing this patch and I saw I am hesitant in the same places I was last time. Anyway, as I said, I'm not that certain with this part of the codebase, so bear with me here :)
src/Makefile.am | 3 +- src/conf/domain_conf.h | 2 ++ src/lxc/lxc_process.c | 4 ++- src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_hotplug.c | 8 +++++ src/qemu/qemu_interface.c | 78 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_interface.h | 32 ++++++++++++++++++ src/qemu/qemu_process.c | 7 ++++ src/util/virnetdevmacvlan.c | 8 +++-- src/util/virnetdevmacvlan.h | 2 ++ 10 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 src/qemu/qemu_interface.c create mode 100644 src/qemu/qemu_interface.h
diff --git a/src/Makefile.am b/src/Makefile.am index fa741a8..035120e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_monitor_text.h \ qemu/qemu_monitor_json.c \ qemu/qemu_monitor_json.h \ - qemu/qemu_driver.c qemu/qemu_driver.h + qemu/qemu_driver.c qemu/qemu_driver.h \ + qemu/qemu_interface.c qemu/qemu_interface.h
XENAPI_DRIVER_SOURCES = \ xenapi/xenapi_driver.c xenapi/xenapi_driver.h \ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0862bd7..5f328cf 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -951,6 +951,8 @@ struct _virDomainNetDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int linkstate; + /* vmOp value saved if deferring interface start */ + virNetDevVPortProfileOp vmOp; };
/* Used for prefix of ifname of any network name generated dynamically diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index ed30c37..b2256c0 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -300,6 +300,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, virNetDevBandwidthPtr bw; virNetDevVPortProfilePtr prof; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); + unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
/* XXX how todo bandwidth controls ? * Since the 'net-ifname' is about to be moved to a different @@ -336,7 +337,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, - virDomainNetGetActualBandwidth(net), 0) < 0) + virDomainNetGetActualBandwidth(net), + macvlan_create_flags) < 0) goto cleanup;
ret = res_ifname; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e5270bd..229dff4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname = res_ifname; }
+ /* Save vport profile op for later */ + net->vmOp = vmop; + virObjectUnref(cfg); return rc; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7bc19cd..530e6da 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -30,6 +30,7 @@ #include "qemu_domain.h" #include "qemu_command.h" #include "qemu_hostdev.h" +#include "qemu_interface.h" #include "domain_audit.h" #include "domain_nwfilter.h" #include "virlog.h" @@ -922,6 +923,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, priv->qemuCaps, tapfd, &tapfdSize) < 0) goto cleanup; iface_connected = true; + /* Set device online immediately */ + qemuInterfaceStartDevice(net); if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { @@ -937,6 +940,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) goto cleanup; iface_connected = true; + /* Set device online immediately */ + qemuInterfaceStartDevice(net); if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) {
Why are you starting the device even if we didn't do that before? Even if users attaching stopped devices on purpose don't concern me, why not just call it for type DIRECT ?
@@ -2070,6 +2075,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }
+ /* Set device online immediately */ + qemuInterfaceStartDevice(newdev); + newType = virDomainNetGetActualType(newdev);
if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
And in this hunk it changes the interface up even though the function can still fail. Either all three calls in this file make something new, not related to what you describe in the commit message, or I have missed something. And we also never set them back to the original state which is not very nice. Feel free to speak up, if there's a reason for doing this than we can also adjust documentation.
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c new file mode 100644 index 0000000..dccfcc4 --- /dev/null +++ b/src/qemu/qemu_interface.c @@ -0,0 +1,78 @@ +/* + * qemu_interface.c: QEMU interface management + * + * Copyright IBM Corp. 2014 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#include <config.h> + +#include "qemu_interface.h" +#include "virnetdev.h" +#include "virnetdevtap.h" +#include "virnetdevmacvlan.h" +#include "virnetdevvportprofile.h" + +/** + * qemuInterfaceStartDevice: + * @net: net device to start + * + * Based upon the type of device provided, perform the appropriate + * work to set the device online. + */ +void +qemuInterfaceStartDevice(virDomainNetDefPtr net) +{ + switch (virDomainNetGetActualType(net)) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevTapDelete(net->ifname)); + } + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevVPortProfileDisassociate(net->ifname, + virDomainNetGetActualVirtPortProfile(net), + &net->mac, + virDomainNetGetActualDirectDev(net), + -1, + net->vmOp)); + } + break; + } +} +
If you fail setting the interface online you clean up, but the caller cannot recognize whether you failed starting the device or not. That means that you can, for example, start the domain, it will have the device, but it won't just be down (maybe it'll be even up), but disconnected from the bridge and we will be looking pretty long time for the problem if a bug report with such criteria comes :)
+/** + * qemuInterfaceStartDevices: + * @def: domain definition + * + * Set all ifaces associated with this domain to the online state. + */ +void +qemuInterfaceStartDevices(virDomainDefPtr def) +{ + size_t i; + + for (i = 0; i < def->nnets; i++) { + qemuInterfaceStartDevice(def->nets[i]); + } + + return; +} diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h new file mode 100644 index 0000000..5810cda --- /dev/null +++ b/src/qemu/qemu_interface.h @@ -0,0 +1,32 @@ +/* + * qemu_interface.h: QEMU interface management + * + * Copyright IBM Corp. 2014 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#ifndef __QEMU_INTERFACE_H__ +# define __QEMU_INTERFACE_H__ + +# include "domain_conf.h" + +void qemuInterfaceStartDevice(virDomainNetDefPtr net); +void qemuInterfaceStartDevices(virDomainDefPtr def); + +#endif /* __QEMU_INTERFACE_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f391743..3fc50a0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -42,6 +42,7 @@ #include "qemu_hostdev.h" #include "qemu_hotplug.h" #include "qemu_migration.h" +#include "qemu_interface.h"
#include "cpu/cpu.h" #include "datatypes.h" @@ -2947,6 +2948,12 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ /* Bring up netdevs before starting CPUs */ + if (reason != VIR_DOMAIN_RUNNING_UNPAUSED && + reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED) { + qemuInterfaceStartDevices(vm->def); + } +
Again, I'm not sure we want to run this in so many cases, but looking at the code, most of the cases are covered by this. Anyway... You will not start the device when destination libvirtd is restarted during QEMU_MIGRATION_PHASE_FINISH2 see the first call from qemuProcessRecoverMigration(). It think it would be safest to run it only when resuming migration (or the cases that are affected) and you cannot determine that based on the @reason (unfortunately, because it looks clean). But it needs to be called only on circa 4 places if I looked correctly.
VIR_DEBUG("Using lock state '%s'", NULLSTR(priv->lockState)); if (virDomainLockProcessResume(driver->lockManager, cfg->uri, vm, priv->lockState) < 0) { diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index c83341c..1edf3ae 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -902,9 +902,11 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto link_del_exit; }
- if (virNetDevSetOnline(cr_ifname, true) < 0) { - rc = -1; - goto disassociate_exit; + if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP) { + if (virNetDevSetOnline(cr_ifname, true) < 0) { + rc = -1; + goto disassociate_exit; + } }
if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 41aa4e2..41b4014 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -44,6 +44,8 @@ typedef enum { VIR_NETDEV_MACVLAN_CREATE_NONE = 0, /* Create with a tap device */ VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, + /* Bring the interface up */ + VIR_NETDEV_MACVLAN_CREATE_IFUP = 1 << 1, } virNetDevMacVLanCreateFlags;
int virNetDevMacVLanCreate(const char *ifname, -- 1.7.9.5
I still see very much unnecessary things being done in this patch and that leads me to thinking I'm still missing something. Because I can express myself better with code than sentences, I'd ask what would a patch like the following one be missing (apart from the fact that I haven't even tried it, so it might not work)? diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h index ea201b3..514ff38 100644 --- i/src/conf/domain_conf.h +++ w/src/conf/domain_conf.h @@ -970,6 +970,8 @@ struct _virDomainNetDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int linkstate; + /* vmOp value saved if deferring interface start */ + virNetDevVPortProfileOp vmOp; }; /* Used for prefix of ifname of any network name generated dynamically diff --git i/src/lxc/lxc_process.c w/src/lxc/lxc_process.c index ed30c37..5e2e998 100644 --- i/src/lxc/lxc_process.c +++ w/src/lxc/lxc_process.c @@ -336,7 +336,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, - virDomainNetGetActualBandwidth(net), 0) < 0) + virDomainNetGetActualBandwidth(net), + VIR_NETDEV_MACVLAN_CREATE_IFUP) < 0) goto cleanup; ret = res_ifname; diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index eb72451..8106fff 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname = res_ifname; } + /* Save vport profile op for later */ + net->vmOp = vmop; + virObjectUnref(cfg); return rc; } diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 5cf235b..1b68114 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -1903,7 +1903,7 @@ static int qemuDomainResume(virDomainPtr dom) } else if (state == VIR_DOMAIN_PAUSED) { if (qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("resume operation failed")); @@ -3255,7 +3255,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, if (was_running && virDomainObjIsActive(vm)) { rc = qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_SAVE_CANCELED, - QEMU_ASYNC_JOB_SAVE); + QEMU_ASYNC_JOB_SAVE, false); if (rc < 0) { VIR_WARN("Unable to resume guest CPUs after save failure"); event = virDomainEventLifecycleNewFromObj(vm, @@ -3760,7 +3760,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, if (resume && virDomainObjIsActive(vm)) { if (qemuProcessStartCPUs(driver, vm, dom->conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_DUMP) < 0) { + QEMU_ASYNC_JOB_DUMP, false) < 0) { event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); @@ -3929,7 +3929,7 @@ static void processWatchdogEvent(virQEMUDriverPtr driver, virDomainObjPtr vm, in ret = qemuProcessStartCPUs(driver, vm, NULL, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_DUMP); + QEMU_ASYNC_JOB_DUMP, false); if (ret < 0) virReportError(VIR_ERR_OPERATION_FAILED, @@ -5644,7 +5644,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, if (header->was_running && !start_paused) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_RESTORED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, true) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to resume domain")); @@ -12554,7 +12554,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn, if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_SNAPSHOT) < 0) { + QEMU_ASYNC_JOB_SNAPSHOT, false) < 0) { event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); @@ -13418,7 +13418,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_SNAPSHOT) < 0) { + QEMU_ASYNC_JOB_SNAPSHOT, false) < 0) { event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); @@ -14349,7 +14349,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_NONE); + QEMU_ASYNC_JOB_NONE, true); if (rc < 0) goto endjob; virObjectUnref(event); @@ -15117,7 +15117,7 @@ qemuDomainBlockPivot(virConnectPtr conn, if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { virObjectEventPtr event = NULL; event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index 284cd5a..ea061b2 100644 --- i/src/qemu/qemu_migration.c +++ w/src/qemu/qemu_migration.c @@ -1314,7 +1314,7 @@ qemuMigrationRestoreDomainState(virConnectPtr conn, virDomainObjPtr vm) /* we got here through some sort of failure; start the domain again */ if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_MIGRATION_CANCELED, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) { + QEMU_ASYNC_JOB_MIGRATION_OUT, false) < 0) { /* Hm, we already know we are in error here. We don't want to * overwrite the previous error, though, so we just throw something * to the logs and hope for the best */ @@ -4919,7 +4919,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver, */ if (qemuProcessStartCPUs(driver, vm, dconn, VIR_DOMAIN_RUNNING_MIGRATED, - QEMU_ASYNC_JOB_MIGRATION_IN) < 0) { + QEMU_ASYNC_JOB_MIGRATION_IN, true) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c index 712a25e..d2f4d31 100644 --- i/src/qemu/qemu_process.c +++ w/src/qemu/qemu_process.c @@ -593,7 +593,7 @@ qemuProcessFakeReboot(void *opaque) if (qemuProcessStartCPUs(driver, vm, NULL, reason, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); @@ -2944,7 +2944,7 @@ qemuProcessPrepareMonitorChr(virQEMUDriverConfigPtr cfg, int qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, virConnectPtr conn, virDomainRunningReason reason, - qemuDomainAsyncJob asyncJob) + qemuDomainAsyncJob asyncJob, bool startDirectIfaces) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -2961,6 +2961,32 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, } VIR_FREE(priv->lockState); + if (startDirectIfaces) { + size_t i = 0; + + for (i = 0; i < vm->def->nnets; i++){ + virDomainNetDefPtr net = vm->def->nets[i]; + + if (virDomainNetGetActualType(net) != VIR_DOMAIN_NET_TYPE_DIRECT) + continue; + + if (virNetDevSetOnline(net->ifname, true) < 0) { + const char *netdev = virDomainNetGetActualDirectDev(net); + virNetDevVPortProfilePtr vport = + virDomainNetGetActualVirtPortProfile(net); + + ignore_value(virNetDevVPortProfileDisassociate(net->ifname, + vport, + &net->mac, + netdev, + -1, + net->vmOp)); + + return -1; + } + } + } + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto release; @@ -3145,7 +3171,7 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver, vm->def->name); if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, true) < 0) { VIR_WARN("Could not resume domain %s", vm->def->name); } break; @@ -3186,7 +3212,7 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver, reason == VIR_DOMAIN_PAUSED_UNKNOWN)) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { VIR_WARN("Could not resume domain %s", vm->def->name); } } @@ -3207,7 +3233,7 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver, reason == VIR_DOMAIN_PAUSED_UNKNOWN)) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { VIR_WARN("Could not resume domain %s", vm->def->name); } } @@ -3262,7 +3288,7 @@ qemuProcessRecoverJob(virQEMUDriverPtr driver, reason == VIR_DOMAIN_PAUSED_UNKNOWN)) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, false) < 0) { VIR_WARN("Could not resume domain '%s' after migration to file", vm->def->name); } @@ -4622,7 +4648,7 @@ int qemuProcessStart(virConnectPtr conn, /* Allow the CPUS to start executing */ if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_BOOTED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_NONE, true) < 0) { if (virGetLastError() == NULL) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("resume operation failed")); diff --git i/src/qemu/qemu_process.h w/src/qemu/qemu_process.h index 5948ea4..6fce0a8 100644 --- i/src/qemu/qemu_process.h +++ w/src/qemu/qemu_process.h @@ -33,7 +33,8 @@ int qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, virConnectPtr conn, virDomainRunningReason reason, - qemuDomainAsyncJob asyncJob); + qemuDomainAsyncJob asyncJob, + bool startDirectIfaces); int qemuProcessStopCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainPausedReason reason, diff --git i/src/util/virnetdevmacvlan.c w/src/util/virnetdevmacvlan.c index c83341c..3216b25 100644 --- i/src/util/virnetdevmacvlan.c +++ w/src/util/virnetdevmacvlan.c @@ -902,10 +902,9 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto link_del_exit; } - if (virNetDevSetOnline(cr_ifname, true) < 0) { - rc = -1; - goto disassociate_exit; - } + if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP && + (rc = virNetDevSetOnline(cr_ifname, true)) < 0) + goto disassociate_exit; if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10)) < 0) diff --git i/src/util/virnetdevmacvlan.h w/src/util/virnetdevmacvlan.h index 41aa4e2..41b4014 100644 --- i/src/util/virnetdevmacvlan.h +++ w/src/util/virnetdevmacvlan.h @@ -44,6 +44,8 @@ typedef enum { VIR_NETDEV_MACVLAN_CREATE_NONE = 0, /* Create with a tap device */ VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, + /* Bring the interface up */ + VIR_NETDEV_MACVLAN_CREATE_IFUP = 1 << 1, } virNetDevMacVLanCreateFlags; int virNetDevMacVLanCreate(const char *ifname, -- Martin
On 09/16/2014 04:50 PM, Matthew Rosato wrote:
Currently, MAC registration occurs during device creation, which is early enough that, during live migration, you end up with duplicate MAC addresses on still-running source and target devices, even though the target device isn't actually being used yet. This patch proposes to defer MAC registration until right before the guest can actually use the device -- In other words, right before starting guest CPUs.
I've probably led you on at least three wild goose chases related to this patch - the first was that I had thought that it would be better to unconditionally wait to ifup *all* network devices. It turns out, though, that regular tap devices which will be connected to a bridge should be ifup'ed and attached to the bridge as soon as possible, so that the forwarding delay timer of the bridge can start to count down. (However, at the same point that macvtap interfaces need to be ifup'ed (i.e. your new qemuInterfaceStartDevice()), tap devices connected to bridges need to have their FDB entries updated (if libvirt is managing the bridge fdb), so it is useful to have a function to call here, it just needs to do something different for different types of connections. Also, your addition of the call to qemuInterfaceStartDevice() during qemuDomainChangeNet() almost certainly came from me telling you that something was needed there. I had forgotten, however, that it isn't possible to change the tap or macvtap device of a network connection without completely removing it anyway, because qemu requires completely detaching both ends of the device in order to change the tap, and that means the device disappears from the guest, in other words it's impossible to *change* it in that way, you have to actually remove and re-add it (at one time I had code to do this, but when it failed to work I inquired and learned of this limitation). Finally, I think it was me that suggested it would be good to stow away the vmOp somewhere so it didn't have to be sent up and down the call stack. When I looked at what you're using it for though (calling virNetDevVPortProfileDisassociate() in case of a failed virNetDevSetOnline() in qemuInterfaceStartDevice()), I could see that if you modify qemuInterfaceStartDevice(s) to return an error on failure, the caller's cleanup will always automatically take care of doing the disassociate for you anyway (this is done in qemuProcessStop() as well as the failure cleanup of qemuDomainAttachNetDevice() - if there is a cleanup path where that isn't called, you've got bigger problems than a dangling 802.11qbh association!). So, we *really* don't need the vmOp :-)) (as aside - it turns out that even if we *did* need to do the disassociate ourselves, analysis of the code shows that there is only a single value of vmOp that would cause any change in behavior, and we're guaranteed that vmOp will *never* be set to that value in the cases where we would be called. I spent *a lot* of time figuring that out before I realized we didn't need to call disassociate *at all*)
Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com> ---
Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1081461
Changes for v3: * Some minor formatting fixes. * in qemuNetworkIfaceConnect, set VIR_NETDEV_TAP_CREATE_IFUP unconditionally. * in qemuDomainAttachNetDevice, call qemuInterfaceStartDevice only for VIR_DOMAIN_NET_TYPE_DIRECT, _BRIDGE and _NETWORK. * in qemuProcessStartCPUs, use 'reason' to determine whether or not qemuInterfaceStartDevices needs to be called. Basically, it needs to be called for any reason that the system would be initializing (or re-initializing).
src/Makefile.am | 3 +- src/conf/domain_conf.h | 2 ++ src/lxc/lxc_process.c | 4 ++- src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_hotplug.c | 8 +++++ src/qemu/qemu_interface.c | 78 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_interface.h | 32 ++++++++++++++++++ src/qemu/qemu_process.c | 7 ++++ src/util/virnetdevmacvlan.c | 8 +++-- src/util/virnetdevmacvlan.h | 2 ++ 10 files changed, 142 insertions(+), 5 deletions(-) create mode 100644 src/qemu/qemu_interface.c create mode 100644 src/qemu/qemu_interface.h
diff --git a/src/Makefile.am b/src/Makefile.am index fa741a8..035120e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_monitor_text.h \ qemu/qemu_monitor_json.c \ qemu/qemu_monitor_json.h \ - qemu/qemu_driver.c qemu/qemu_driver.h + qemu/qemu_driver.c qemu/qemu_driver.h \ + qemu/qemu_interface.c qemu/qemu_interface.h
XENAPI_DRIVER_SOURCES = \ xenapi/xenapi_driver.c xenapi/xenapi_driver.h \ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0862bd7..5f328cf 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -951,6 +951,8 @@ struct _virDomainNetDef { virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; int linkstate; + /* vmOp value saved if deferring interface start */ + virNetDevVPortProfileOp vmOp; };
I've removed this as unnecessary, per paragraph 3 above.
/* Used for prefix of ifname of any network name generated dynamically diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index ed30c37..b2256c0 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -300,6 +300,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, virNetDevBandwidthPtr bw; virNetDevVPortProfilePtr prof; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); + unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
/* XXX how todo bandwidth controls ? * Since the 'net-ifname' is about to be moved to a different @@ -336,7 +337,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, &res_ifname, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, cfg->stateDir, - virDomainNetGetActualBandwidth(net), 0) < 0) + virDomainNetGetActualBandwidth(net), + macvlan_create_flags) < 0)
This *could* be inferred from the other flags, but I like making it explicit as you have done (in the future the CREATE_WITH_TAP might *not* always be the opposite of CREATE_IFUP).
goto cleanup;
ret = res_ifname; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e5270bd..229dff4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def, net->ifname = res_ifname; }
+ /* Save vport profile op for later */ + net->vmOp = vmop; +
removed, as noted in paragraph 3 at the top.
virObjectUnref(cfg); return rc; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7bc19cd..530e6da 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -30,6 +30,7 @@ #include "qemu_domain.h" #include "qemu_command.h" #include "qemu_hostdev.h" +#include "qemu_interface.h" #include "domain_audit.h" #include "domain_nwfilter.h" #include "virlog.h" @@ -922,6 +923,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, priv->qemuCaps, tapfd, &tapfdSize) < 0) goto cleanup; iface_connected = true; + /* Set device online immediately */ + qemuInterfaceStartDevice(net);
For now calling this for a standard tap device (connected to a bridge) has no effect, but I'm leaving it in as it is going to be useful very soon (to update the fdb at the proper time).
if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { @@ -937,6 +940,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE)) < 0) goto cleanup; iface_connected = true; + /* Set device online immediately */ + qemuInterfaceStartDevice(net);
Well, actually what makes more sense is to put a single call to qemuInterfaceStartDevice() after all the if-elses. For all cases except DIRECT, BRIDGE, and NETWORK it is a NOP, and for those 3 the only difference will be that it gets called after qemuOpenVhostNet() rather than before, and that makes no practical difference (also sets us up better for having it take effect in the future if/when other interface types need special treatment during "start" phase).
if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, vhostfd, &vhostfdSize) < 0) goto cleanup; } else if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET) { @@ -2070,6 +2075,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }
+ /* Set device online immediately */ + qemuInterfaceStartDevice(newdev); +
Removed, as noted in paragraph 2 at the top.
newType = virDomainNetGetActualType(newdev);
if (newType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c new file mode 100644 index 0000000..dccfcc4 --- /dev/null +++ b/src/qemu/qemu_interface.c @@ -0,0 +1,78 @@ +/* + * qemu_interface.c: QEMU interface management + * + * Copyright IBM Corp. 2014 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#include <config.h> + +#include "qemu_interface.h" +#include "virnetdev.h" +#include "virnetdevtap.h" +#include "virnetdevmacvlan.h" +#include "virnetdevvportprofile.h" + +/** + * qemuInterfaceStartDevice: + * @net: net device to start + * + * Based upon the type of device provided, perform the appropriate + * work to set the device online. + */ +void +qemuInterfaceStartDevice(virDomainNetDefPtr net)
Both of these functions should return errors, since failure to ifup an interface was previously grounds for aborting whatever operation was happening.
+{ + switch (virDomainNetGetActualType(net)) {
I like the idea of typecasting the result here and putting in in all possible cases (even if empty) so that anyone adding a new type will be notified that they potentially need to add something.
+ case VIR_DOMAIN_NET_TYPE_BRIDGE:
The indentation is off here - the "case" should be at the same level as "switch", with contents of the case indented by 4.
+ case VIR_DOMAIN_NET_TYPE_NETWORK: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevTapDelete(net->ifname)); + }
Actually, in the case of tap devices connected to a bridge, they should be (and are) created IFF_UP (required to start the forwarding delay timers counting down as soon as possible), so doing this here is redundant. What *is* needed here is to update the bridge's fdb if macTableManager='libvirt'. I'm removing the existing call to virNetDevSetOnline() here, and will add in a call to update the fdb in an upcoming patch.
+ break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + if (virNetDevSetOnline(net->ifname, true) < 0) { + ignore_value(virNetDevVPortProfileDisassociate(net->ifname, + virDomainNetGetActualVirtPortProfile(net), + &net->mac, + virDomainNetGetActualDirectDev(net), + -1, + net->vmOp));
As noted in paragraph 4 at the top - if we properly return an error from this function (and qemuInterfaceStartDevices) then we don't need to call virNetDevVPortProfileDisassociate here - it will be handled in the cleanup of our caller. So I'm removing that bit.
+ } + break; + } +} + +/** + * qemuInterfaceStartDevices: + * @def: domain definition + * + * Set all ifaces associated with this domain to the online state. + */ +void +qemuInterfaceStartDevices(virDomainDefPtr def)
again - this should return errors rather than ignoring them.
+{ + size_t i; + + for (i = 0; i < def->nnets; i++) { + qemuInterfaceStartDevice(def->nets[i]); + }
Because I'm adding a "goto cleanup" on failure here, we *will* need the braces, but just for future reference if this code had stayed as is, you would need to remove the braces because the body is a single line.
+ + return; +} diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h new file mode 100644 index 0000000..5810cda --- /dev/null +++ b/src/qemu/qemu_interface.h @@ -0,0 +1,32 @@ +/* + * qemu_interface.h: QEMU interface management + * + * Copyright IBM Corp. 2014 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Matthew J. Rosato <mjrosato@linux.vnet.ibm.com> + */ + +#ifndef __QEMU_INTERFACE_H__ +# define __QEMU_INTERFACE_H__ + +# include "domain_conf.h" + +void qemuInterfaceStartDevice(virDomainNetDefPtr net); +void qemuInterfaceStartDevices(virDomainDefPtr def);
changed both to return int.
+ +#endif /* __QEMU_INTERFACE_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f391743..3fc50a0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -42,6 +42,7 @@ #include "qemu_hostdev.h" #include "qemu_hotplug.h" #include "qemu_migration.h" +#include "qemu_interface.h"
#include "cpu/cpu.h" #include "datatypes.h" @@ -2947,6 +2948,12 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ /* Bring up netdevs before starting CPUs */ + if (reason != VIR_DOMAIN_RUNNING_UNPAUSED && + reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED) { + qemuInterfaceStartDevices(vm->def); + } +
changed to goto cleanup if qemuInterfaceStartDevices() fails, which will guarantee that disassociate is called in the cleanup)
VIR_DEBUG("Using lock state '%s'", NULLSTR(priv->lockState)); if (virDomainLockProcessResume(driver->lockManager, cfg->uri, vm, priv->lockState) < 0) { diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index c83341c..1edf3ae 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -902,9 +902,11 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, goto link_del_exit; }
- if (virNetDevSetOnline(cr_ifname, true) < 0) { - rc = -1; - goto disassociate_exit; + if (flags & VIR_NETDEV_MACVLAN_CREATE_IFUP) { + if (virNetDevSetOnline(cr_ifname, true) < 0) { + rc = -1; + goto disassociate_exit; + } }
if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) { diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 41aa4e2..41b4014 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -44,6 +44,8 @@ typedef enum { VIR_NETDEV_MACVLAN_CREATE_NONE = 0, /* Create with a tap device */ VIR_NETDEV_MACVLAN_CREATE_WITH_TAP = 1 << 0, + /* Bring the interface up */ + VIR_NETDEV_MACVLAN_CREATE_IFUP = 1 << 1, } virNetDevMacVLanCreateFlags;
int virNetDevMacVLanCreate(const char *ifname,
ACK with the above modifications. I've made those (see attached diff) and am pushing this as a bugfix.
On 09/16/2014 04:50 PM, Matthew Rosato wrote:
#include "cpu/cpu.h" #include "datatypes.h" @@ -2947,6 +2948,12 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ /* Bring up netdevs before starting CPUs */ + if (reason != VIR_DOMAIN_RUNNING_UNPAUSED && + reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED) { + qemuInterfaceStartDevices(vm->def); + }
Matthew, I've already pushed your patch, but am trying to add to it for another related purpose and this bit is bothering me. What is the reason for not calling qemuInterfaceStartDevices() when reason is VIR_DOMAIN_RUNNING_UNPAUSED or VIR_DOMAIN_RUNNING_SAVE_CANCELED? Is it just to avoid setting IFF_UP on an interface that is already IFF_UP? If that's the only reason, I would prefer to have it *always* called when the CPUs are started (and a corresponding qemuInterfaceStopDevices() called when the CPUs are stopped). Otherwise, it looks to me like it is possible in some situations (migration error recovery perhaps? search for VIR_DOMAIN_RUNNING_UNPAUSED) to have the CPUs running, but the macvtap interfaces *not* IFF_UP. Do you see (or did you experience?) a problem calling qemuInterfaceStartDevices() for all reasons?
On 12/11/2014 01:35 PM, Laine Stump wrote:
On 09/16/2014 04:50 PM, Matthew Rosato wrote:
#include "cpu/cpu.h" #include "datatypes.h" @@ -2947,6 +2948,12 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ /* Bring up netdevs before starting CPUs */ + if (reason != VIR_DOMAIN_RUNNING_UNPAUSED && + reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED) { + qemuInterfaceStartDevices(vm->def); + }
Matthew,
I've already pushed your patch, but am trying to add to it for another related purpose and this bit is bothering me. What is the reason for not calling qemuInterfaceStartDevices() when reason is VIR_DOMAIN_RUNNING_UNPAUSED or VIR_DOMAIN_RUNNING_SAVE_CANCELED? Is it just to avoid setting IFF_UP on an interface that is already IFF_UP?
If that's the only reason, I would prefer to have it *always* called when the CPUs are started (and a corresponding qemuInterfaceStopDevices() called when the CPUs are stopped). Otherwise, it looks to me like it is possible in some situations (migration error recovery perhaps? search for VIR_DOMAIN_RUNNING_UNPAUSED) to have the CPUs running, but the macvtap interfaces *not* IFF_UP.
Do you see (or did you experience?) a problem calling qemuInterfaceStartDevices() for all reasons?
Hi Laine, I did not experience any problems calling qemuInterfaceStartDevices() unconditionally, that's actually how I originally wrote the code -- I added these conditional statements based on a review comment to avoid unnecessary IFF_UPs. I'd be fine with the call being unconditional again. Matt
On 12/11/2014 02:18 PM, Matthew Rosato wrote:
On 12/11/2014 01:35 PM, Laine Stump wrote:
On 09/16/2014 04:50 PM, Matthew Rosato wrote:
#include "cpu/cpu.h" #include "datatypes.h" @@ -2947,6 +2948,12 @@ qemuProcessStartCPUs(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ /* Bring up netdevs before starting CPUs */ + if (reason != VIR_DOMAIN_RUNNING_UNPAUSED && + reason != VIR_DOMAIN_RUNNING_SAVE_CANCELED) { + qemuInterfaceStartDevices(vm->def); + }
Matthew,
I've already pushed your patch, but am trying to add to it for another related purpose and this bit is bothering me. What is the reason for not calling qemuInterfaceStartDevices() when reason is VIR_DOMAIN_RUNNING_UNPAUSED or VIR_DOMAIN_RUNNING_SAVE_CANCELED? Is it just to avoid setting IFF_UP on an interface that is already IFF_UP?
If that's the only reason, I would prefer to have it *always* called when the CPUs are started (and a corresponding qemuInterfaceStopDevices() called when the CPUs are stopped). Otherwise, it looks to me like it is possible in some situations (migration error recovery perhaps? search for VIR_DOMAIN_RUNNING_UNPAUSED) to have the CPUs running, but the macvtap interfaces *not* IFF_UP.
Do you see (or did you experience?) a problem calling qemuInterfaceStartDevices() for all reasons?
Hi Laine,
I did not experience any problems calling qemuInterfaceStartDevices() unconditionally, that's actually how I originally wrote the code -- I added these conditional statements based on a review comment to avoid unnecessary IFF_UPs.
I'd be fine with the call being unconditional again.
Okay. I've gone back through the archives of the 3 versions of the patch and understand the origin. I've also tested out pausing/unpausing a guest with a macvtap interface when this extra condition is removed, and networking continues to work with no problems. Since there is no specific experienced reason for this restriction on when to call qemuInterface StartDevices() (but rather was just you making reviewers happy :-)), I'm going to send a patch to remove that conditional, then another patch to add qemuInterfaceStopDevices(), and finally one that adds/removes bridge fdb entries for tap devices when the network has macTableManager='libvirt'.
participants (3)
-
Laine Stump -
Martin Kletzander -
Matthew Rosato