Current virtio-net drivers that support the failover feature match up
the virtio backup device with its corresponding hostdev device by
looking for an interface with a matching MAC address. Since libvirt
will assign a different random MAC address to each interface that
isn't given an explicit MAC address, this means that the configuration
for any failover pairs must include explicit matching MAC addresses.
To make life easier, we could have libvirt populate the XML config
with the same auto-generated MAC address for both interfaces when it
detects a failover pair that have no MAC addresses provided (a
failover pair can be detected by matching <alias name='blah'/> of the
virtio interface with <driver backupAlias='blah'/> of the hostdev
interface), but then we would be stuck with that behavior even if the
virtio guest driver later eliminated the requirement that mac
addresses match.
Additionally, some management software uses the MAC address as the
primary index for its list of network devices, and can't properly deal
with two interfaces having the same MAC address (oVirt). Even
libvirt's own virsh utility uses MAC address (combined with interface
type) to uniquely identify interfaces for the virsh detach-interface
command (in this case, fortunately the runtime interface type is used,
so one of the interfaces will always be of type='hostdev' and the
other type='something-else", so it doesn't currently cause any problem).
In order to remove the necessity of explicitly setting interface MAC
addresses, as well as permit the two interfaces of a failover pair to
each have a unique index while still fulfilling the current guest
driver requirement that the MAC addresses matchin the guest, this
patch adds a new attribute "useBackupMAC" that is set on the hostdev
interface of the pair. When useBackupMAC='yes', the setup for the
hostdev interface will find the virtio failover interface (using
backupAlias) and use that interface's MAC address to initialize the
MAC address of the hostdev interface; the MAC address in the hostdev
interface config remains unchanged, it just isnt used for device
initialization.
I made this patch to followup on
https://bugzilla.redhat.com/show_bug.cgi?id=1693587#c12 (where I
suggested this attribute as a possible remedy to oVirt's requirement
that each network device have a unique MAC address).
Truthfully, I'm not convinced that I want it though, as it seems
"a bit" hackish. In particular, I've thought for a long time that the
"hostdev manager" code in util/virhostdev.c should really live in the
node device driver and be called from the hypervisors via a public API
(so that there is one central place on the host that maintains the
list of in-use PCI devices and their status), but this patch adds an
obstacle to that goal by adding a virDomainDefPtr to more of the APIs in
that library - if this was turned into a public API, then entire
virDomainDef would need to be serialized and sent in the API call,
then parsed at the other end - yuck :-/. NB: there are already
functions in virhostdev.h that take a virDomainDefPtr, so maybe I'm
being too sensitive.
On the upside, it solves a problem, and default bevahior is unchanged.
So although we would have to live with the presence of this option
forever once added, at least it would never be used unknowingly - the
behavior it triggers would only occur if it was explicitly called out
in the config.
Signed-off-by: Laine Stump <laine@redhat.com>
---
docs/formatdomain.html.in | 6 ++++-
docs/schemas/domaincommon.rng | 5 ++++
src/conf/domain_conf.c | 41 ++++++++++++++++++++++++++++--
src/conf/domain_conf.h | 2 ++
src/libxl/libxl_driver.c | 2 +-
src/qemu/qemu_hostdev.c | 5 ++--
src/qemu/qemu_hostdev.h | 1 +
src/qemu/qemu_hotplug.c | 2 +-
src/util/virhostdev.c | 47 +++++++++++++++++++++++++++++------
src/util/virhostdev.h | 1 +
tests/virhostdevtest.c | 18 +++++++-------
11 files changed, 106 insertions(+), 24 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index e3ea89fe25..691c54da74 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -5906,7 +5906,11 @@
the virtio and hostdev NIC must match. Since that may not always
be a requirement, libvirt doesn't enforce this limitation - it
is up to the person/management application that is creating the
- configuration.
+ configuration. However, if the driver
+ attribute <code>useBackupMAC</code> is set to "yes", then the
+ hostdev NIC's own MAC address will be ignored, and libvirt will
+ initialize the hostdev NIC with the MAC address configured for
+ the virtio NIC (the backup).
</p>
<p>
NB3: Since the PCI addresses of the SRIOV VFs on the hosts that
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index e0977d28d3..cd19a7c3b4 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3007,6 +3007,11 @@
<ref name='aliasName'/>
</attribute>
</optional>
+ <optional>
+ <attribute name="useBackupMAC">
+ <ref name="virYesNo"/>
+ </attribute>
+ </optional>
<choice>
<group>
<attribute name="name">
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 89cccd22bc..ca49fb825b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6363,10 +6363,17 @@ virDomainHostdevDefValidate(const virDomainHostdevDef *hostdev)
"'unassigned' address type"));
return -1;
}
- if (hostdev->source.subsys.u.pci.backupAlias &&
+ if ((hostdev->source.subsys.u.pci.backupAlias ||
+ hostdev->source.subsys.u.pci.useBackupMAC == VIR_TRISTATE_BOOL_YES) &&
!hostdev->parentnet) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("backupAlias is not supported for plain <hostdev> - <interface type='hostdev'> is required"));
+ _("backupAlias and useBackupMAC are not supported for plain <hostdev> - <interface type='hostdev'> is required"));
+ return -1;
+ }
+ if (hostdev->source.subsys.u.pci.useBackupMAC == VIR_TRISTATE_BOOL_YES &&
+ !hostdev->source.subsys.u.pci.backupAlias) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("useBackupMAC requires backupAlias to also be set"));
return -1;
}
break;
@@ -8270,6 +8277,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
g_autofree char *model = NULL;
g_autofree char *display = NULL;
g_autofree char *ramfb = NULL;
+ g_autofree char *useBackupMAC = NULL;
/* @managed can be read from the xml document - it is always an
* attribute of the toplevel element, no matter what type of
@@ -8420,6 +8428,13 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
pcisrc->backend = backend;
pcisrc->backupAlias = virXPathString("string(./driver/@backupAlias)", ctxt);
+
+ if ((useBackupMAC = virXPathString("string(./driver/@useBackupMAC)", ctxt)) &&
+ (pcisrc->useBackupMAC = virTristateBoolTypeFromString(useBackupMAC)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unknown useBackupMAC value '%s'"), useBackupMAC);
+ return -1;
+ }
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
@@ -11565,6 +11580,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
g_autofree char *localport = NULL;
g_autofree char *model = NULL;
g_autofree char *backupAlias = NULL;
+ g_autofree char *useBackupMAC = NULL;
g_autofree char *backend = NULL;
g_autofree char *txmode = NULL;
g_autofree char *ioeventfd = NULL;
@@ -11738,6 +11754,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
model = virXMLPropString(cur, "type");
} else if (virXMLNodeNameEqual(cur, "driver")) {
backupAlias = virXMLPropString(cur, "backupAlias");
+ useBackupMAC = virXMLPropString(cur, "useBackupMAC");
backend = virXMLPropString(cur, "name");
txmode = virXMLPropString(cur, "txmode");
ioeventfd = virXMLPropString(cur, "ioeventfd");
@@ -12082,6 +12099,13 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
if (backupAlias)
def->driver.backupAlias = g_steal_pointer(&backupAlias);
+ if (useBackupMAC &&
+ (def->driver.useBackupMAC = virTristateBoolTypeFromString(useBackupMAC)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unknown useBackupMAC value '%s'"), useBackupMAC);
+ goto error;
+ }
+
if (virDomainNetIsVirtioModel(def)) {
if (backend != NULL) {
if ((val = virDomainNetBackendTypeFromString(backend)) < 0 ||
@@ -25073,6 +25097,11 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
virBufferEscapeString(buf, " backupAlias='%s'", pcisrc->backupAlias);
+ if (pcisrc->useBackupMAC) {
+ virBufferAsprintf(buf, " useBackupMAC='%s'",
+ virTristateBoolTypeToString(pcisrc->useBackupMAC));
+ }
+
virBufferAddLit(buf, "/>\n");
}
@@ -25464,6 +25493,11 @@ virDomainNetDriverAttributesFormat(char **outstr,
virBufferEscapeString(&buf, " backupAlias='%s'", def->driver.backupAlias);
+ if (def->driver.useBackupMAC) {
+ virBufferAsprintf(&buf, " useBackupMAC='%s'",
+ virTristateBoolTypeToString(def->driver.useBackupMAC));
+ }
+
*outstr = virBufferContentAndReset(&buf);
}
@@ -30686,6 +30720,9 @@ virDomainNetDefActualFromNetworkPort(virDomainNetDefPtr iface,
actual->data.hostdev.def.source.subsys.u.pci.addr = port->plug.hostdevpci.addr;
actual->data.hostdev.def.source.subsys.u.pci.backupAlias
= g_strdup(iface->driver.backupAlias);
+ actual->data.hostdev.def.source.subsys.u.pci.useBackupMAC
+ = iface->driver.useBackupMAC;
+
switch ((virNetworkForwardDriverNameType)port->plug.hostdevpci.driver) {
case VIR_NETWORK_FORWARD_DRIVER_NAME_DEFAULT:
actual->data.hostdev.def.source.subsys.u.pci.backend =
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2b6d9bab75..e505823a89 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -236,6 +236,7 @@ struct _virDomainHostdevSubsysPCI {
virPCIDeviceAddress addr; /* host address */
int backend; /* enum virDomainHostdevSubsysPCIBackendType */
char *backupAlias; /* alias id of backup virtio device for failover */
+ virTristateBool useBackupMAC; /* true to use MAC from backup virtio NIC */
};
struct _virDomainHostdevSubsysSCSIHost {
@@ -930,6 +931,7 @@ struct _virDomainNetDef {
char *modelstr;
struct {
char *backupAlias; /* alias id of backup virtio device for failover */
+ virTristateBool useBackupMAC; /* true to use MAC from backup virtio NIC */
union {
struct {
virDomainNetBackendType name; /* which driver backend to use */
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index f021ec9c5d..7e4266c534 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3119,7 +3119,7 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr driver,
if (virHostdevPreparePCIDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
vm->def->name, vm->def->uuid,
- &hostdev, 1, 0) < 0)
+ vm->def, &hostdev, 1, 0) < 0)
goto cleanup;
if (libxlMakePCI(hostdev, &pcidev) < 0)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 1774850640..7359f67b66 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -238,6 +238,7 @@ int
qemuHostdevPreparePCIDevices(virQEMUDriverPtr driver,
const char *name,
const unsigned char *uuid,
+ virDomainDefPtr def,
virDomainHostdevDefPtr *hostdevs,
int nhostdevs,
virQEMUCapsPtr qemuCaps,
@@ -249,7 +250,7 @@ qemuHostdevPreparePCIDevices(virQEMUDriverPtr driver,
return -1;
return virHostdevPreparePCIDevices(hostdev_mgr, QEMU_DRIVER_NAME,
- name, uuid, hostdevs,
+ name, uuid, def, hostdevs,
nhostdevs, flags);
}
@@ -354,7 +355,7 @@ qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver,
return -1;
if (qemuHostdevPreparePCIDevices(driver, def->name, def->uuid,
- def->hostdevs, def->nhostdevs,
+ def, def->hostdevs, def->nhostdevs,
qemuCaps, flags) < 0)
return -1;
diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
index 23df925529..01a7dd37c5 100644
--- a/src/qemu/qemu_hostdev.h
+++ b/src/qemu/qemu_hostdev.h
@@ -51,6 +51,7 @@ int qemuHostdevPrepareNVMeDisks(virQEMUDriverPtr driver,
int qemuHostdevPreparePCIDevices(virQEMUDriverPtr driver,
const char *name,
const unsigned char *uuid,
+ virDomainDefPtr def,
virDomainHostdevDefPtr *hostdevs,
int nhostdevs,
virQEMUCapsPtr qemuCaps,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 31d455505b..97fb859334 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1549,7 +1549,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
if (!cfg->relaxedACS)
flags |= VIR_HOSTDEV_STRICT_ACS_CHECK;
if (qemuHostdevPreparePCIDevices(driver, vm->def->name, vm->def->uuid,
- &hostdev, 1, priv->qemuCaps, flags) < 0)
+ vm->def, &hostdev, 1, priv->qemuCaps, flags) < 0)
return -1;
/* this could have been changed by qemuHostdevPreparePCIDevices */
diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 9b4ea30216..d42b5263d6 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -487,6 +487,7 @@ virHostdevSaveNetConfig(virDomainHostdevDefPtr hostdev,
/**
* virHostdevSetNetConfig:
* @hostdev: config object describing a hostdev device
+ * @domaindef: complete domain if available (otherwise NULL)
* @uuid: uuid of the domain
*
* If the given hostdev device is an SRIOV network VF, determine its
@@ -497,9 +498,11 @@ virHostdevSaveNetConfig(virDomainHostdevDefPtr hostdev,
*/
static int
virHostdevSetNetConfig(virDomainHostdevDefPtr hostdev,
+ virDomainDefPtr domaindef,
const unsigned char *uuid)
{
g_autofree char *linkdev = NULL;
+ const virMacAddr *mac;
const virNetDevVlan *vlan;
const virNetDevVPortProfile *virtPort;
int vf = -1;
@@ -511,6 +514,32 @@ virHostdevSetNetConfig(virDomainHostdevDefPtr hostdev,
if (virHostdevNetDevice(hostdev, -1, &linkdev, &vf) < 0)
return -1;
+ mac = &hostdev->parentnet->mac;
+ if (hostdev->source.subsys.u.pci.useBackupMAC == VIR_TRISTATE_BOOL_YES) {
+ size_t i;
+
+ if (!domaindef) {
+ /* This should never happen */
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to get backup device MAC because domaindef is NULL"));
+ return -1;
+ }
+
+ for (i = 0; i < domaindef->nnets; i++) {
+ if (STREQ_NULLABLE(domaindef->nets[i]->info.alias,
+ hostdev->source.subsys.u.pci.backupAlias)) {
+ mac = &domaindef->nets[i]->mac;
+ break;
+ }
+ }
+ if (i > domaindef->nnets) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("backup device with alias name='%s' was not found"),
+ hostdev->source.subsys.u.pci.backupAlias);
+ return -1;
+ }
+ }
+
vlan = virDomainNetGetActualVlan(hostdev->parentnet);
virtPort = virDomainNetGetActualVirtPortProfile(hostdev->parentnet);
if (virtPort) {
@@ -522,12 +551,10 @@ virHostdevSetNetConfig(virDomainHostdevDefPtr hostdev,
return -1;
}
if (virHostdevNetConfigVirtPortProfile(linkdev, vf, virtPort,
- &hostdev->parentnet->mac,
- uuid, port_profile_associate) < 0)
+ mac, uuid, port_profile_associate) < 0)
return -1;
} else {
- if (virNetDevSetNetConfig(linkdev, vf, &hostdev->parentnet->mac,
- vlan, NULL, true) < 0)
+ if (virNetDevSetNetConfig(linkdev, vf, mac, vlan, NULL, true) < 0)
return -1;
}
@@ -724,6 +751,7 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
const char *dom_name,
const unsigned char *uuid,
virPCIDeviceListPtr pcidevs,
+ virDomainDefPtr domaindef, /* only used if nhostdevs > 0 */
virDomainHostdevDefPtr *hostdevs,
int nhostdevs,
unsigned int flags)
@@ -872,7 +900,7 @@ virHostdevPreparePCIDevicesImpl(virHostdevManagerPtr mgr,
* the network device, set the new netdev config */
for (i = 0; i < nhostdevs; i++) {
- if (virHostdevSetNetConfig(hostdevs[i], uuid) < 0)
+ if (virHostdevSetNetConfig(hostdevs[i], domaindef, uuid) < 0)
goto resetvfnetconfig;
last_processed_hostdev_vf = i;
@@ -988,6 +1016,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
const char *drv_name,
const char *dom_name,
const unsigned char *uuid,
+ virDomainDefPtr domaindef,
virDomainHostdevDefPtr *hostdevs,
int nhostdevs,
unsigned int flags)
@@ -1001,7 +1030,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr,
return -1;
return virHostdevPreparePCIDevicesImpl(mgr, drv_name, dom_name, uuid,
- pcidevs, hostdevs, nhostdevs, flags);
+ pcidevs, domaindef, hostdevs,
+ nhostdevs, flags);
}
@@ -2151,6 +2181,7 @@ virHostdevPrepareDomainDevices(virHostdevManagerPtr mgr,
if (flags & VIR_HOSTDEV_SP_PCI) {
if (virHostdevPreparePCIDevices(mgr, driver,
def->name, def->uuid,
+ def,
def->hostdevs,
def->nhostdevs,
flags) < 0)
@@ -2358,8 +2389,8 @@ virHostdevPrepareOneNVMeDevice(virHostdevManagerPtr hostdev_mgr,
}
if (virHostdevPreparePCIDevicesImpl(hostdev_mgr,
- drv_name, dom_name, NULL,
- pciDevices, NULL, 0, pciFlags) < 0)
+ drv_name, dom_name, NULL, pciDevices,
+ NULL, NULL, 0, pciFlags) < 0)
goto rollback;
ret = 0;
diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h
index ae84ed3d20..1aba8b73a8 100644
--- a/src/util/virhostdev.h
+++ b/src/util/virhostdev.h
@@ -68,6 +68,7 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr hostdev_mgr,
const char *drv_name,
const char *dom_name,
const unsigned char *uuid,
+ virDomainDefPtr domaindef,
virDomainHostdevDefPtr *hostdevs,
int nhostdevs,
unsigned int flags)
diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c
index b6260bd9c1..f05ea73125 100644
--- a/tests/virhostdevtest.c
+++ b/tests/virhostdevtest.c
@@ -186,7 +186,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void)
/* Test normal functionality */
VIR_TEST_DEBUG("Test 0 hostdevs");
if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid,
- NULL, 0, 0) < 0)
+ NULL, NULL, 0, 0) < 0)
return -1;
CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count);
CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count);
@@ -194,7 +194,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void)
/* Test unmanaged hostdevs */
VIR_TEST_DEBUG("Test >=1 unmanaged hostdevs");
if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid,
- hostdevs, nhostdevs, 0) < 0)
+ NULL, hostdevs, nhostdevs, 0) < 0)
return -1;
CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs);
CHECK_PCI_LIST_COUNT(mgr->inactivePCIHostdevs, inactive_count - nhostdevs);
@@ -204,7 +204,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void)
inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs);
VIR_TEST_DEBUG("Test: prepare same hostdevs for same driver/domain again");
if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid,
- &hostdevs[0], 1, 0) == 0)
+ NULL, &hostdevs[0], 1, 0) == 0)
return -1;
virResetLastError();
CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count);
@@ -212,7 +212,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void)
VIR_TEST_DEBUG("Test: prepare same hostdevs for same driver, diff domain again");
if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid,
- &hostdevs[1], 1, 0) == 0)
+ NULL, &hostdevs[1], 1, 0) == 0)
return -1;
virResetLastError();
CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count);
@@ -220,7 +220,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(void)
VIR_TEST_DEBUG("Test: prepare same hostdevs for diff driver/domain again");
if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid,
- &hostdevs[2], 1, 0) == 0)
+ NULL, &hostdevs[2], 1, 0) == 0)
return -1;
virResetLastError();
CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count);
@@ -272,7 +272,7 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed)
/* Test normal functionality */
VIR_TEST_DEBUG("Test >=1 hostdevs");
if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid,
- hostdevs, nhostdevs, 0) < 0)
+ NULL, hostdevs, nhostdevs, 0) < 0)
return -1;
CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count + nhostdevs);
/* If testing a mixed roundtrip, devices are already in the inactive list
@@ -288,7 +288,7 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed)
inactive_count = virPCIDeviceListCount(mgr->inactivePCIHostdevs);
VIR_TEST_DEBUG("Test: prepare same hostdevs for same driver/domain again");
if (virHostdevPreparePCIDevices(mgr, drv_name, dom_name, uuid,
- &hostdevs[0], 1, 0) == 0)
+ NULL, &hostdevs[0], 1, 0) == 0)
return -1;
virResetLastError();
CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count);
@@ -296,7 +296,7 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed)
VIR_TEST_DEBUG("Test: prepare same hostdevs for same driver, diff domain again");
if (virHostdevPreparePCIDevices(mgr, drv_name, "test_domain1", uuid,
- &hostdevs[1], 1, 0) == 0)
+ NULL, &hostdevs[1], 1, 0) == 0)
return -1;
virResetLastError();
CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count);
@@ -304,7 +304,7 @@ testVirHostdevPreparePCIHostdevs_managed(bool mixed)
VIR_TEST_DEBUG("Test: prepare same hostdevs for diff driver/domain again");
if (virHostdevPreparePCIDevices(mgr, "test_driver1", dom_name, uuid,
- &hostdevs[2], 1, 0) == 0)
+ NULL, &hostdevs[2], 1, 0) == 0)
return -1;
virResetLastError();
CHECK_PCI_LIST_COUNT(mgr->activePCIHostdevs, active_count);
--
2.24.1
On Sun, Jan 19, 2020 at 10:24:19PM -0500, Laine Stump wrote: > Current virtio-net drivers that support the failover feature match up > the virtio backup device with its corresponding hostdev device by > looking for an interface with a matching MAC address. Since libvirt > will assign a different random MAC address to each interface that > isn't given an explicit MAC address, this means that the configuration > for any failover pairs must include explicit matching MAC addresses. > > To make life easier, we could have libvirt populate the XML config > with the same auto-generated MAC address for both interfaces when it > detects a failover pair that have no MAC addresses provided (a > failover pair can be detected by matching <alias name='blah'/> of the > virtio interface with <driver backupAlias='blah'/> of the hostdev > interface), but then we would be stuck with that behavior even if the > virtio guest driver later eliminated the requirement that mac > addresses match. > > Additionally, some management software uses the MAC address as the > primary index for its list of network devices, and can't properly deal > with two interfaces having the same MAC address (oVirt). Even > libvirt's own virsh utility uses MAC address (combined with interface > type) to uniquely identify interfaces for the virsh detach-interface > command (in this case, fortunately the runtime interface type is used, > so one of the interfaces will always be of type='hostdev' and the > other type='something-else", so it doesn't currently cause any problem). > > In order to remove the necessity of explicitly setting interface MAC > addresses, as well as permit the two interfaces of a failover pair to > each have a unique index while still fulfilling the current guest > driver requirement that the MAC addresses matchin the guest, this > patch adds a new attribute "useBackupMAC" that is set on the hostdev > interface of the pair. When useBackupMAC='yes', the setup for the > hostdev interface will find the virtio failover interface (using > backupAlias) and use that interface's MAC address to initialize the > MAC address of the hostdev interface; the MAC address in the hostdev > interface config remains unchanged, it just isnt used for device > initialization. > > I made this patch to followup on > https://bugzilla.redhat.com/show_bug.cgi?id=1693587#c12 (where I > suggested this attribute as a possible remedy to oVirt's requirement > that each network device have a unique MAC address). > > Truthfully, I'm not convinced that I want it though, as it seems > "a bit" hackish. In particular, I've thought for a long time that the > "hostdev manager" code in util/virhostdev.c should really live in the > node device driver and be called from the hypervisors via a public API > (so that there is one central place on the host that maintains the > list of in-use PCI devices and their status), but this patch adds an > obstacle to that goal by adding a virDomainDefPtr to more of the APIs in > that library - if this was turned into a public API, then entire > virDomainDef would need to be serialized and sent in the API call, > then parsed at the other end - yuck :-/. NB: there are already > functions in virhostdev.h that take a virDomainDefPtr, so maybe I'm > being too sensitive. > > On the upside, it solves a problem, and default bevahior is unchanged. I don't believe it does solve a real problem. If a mgmt app is capable of setting useBackupMAC=yes when writing the XML doc, then I don't see why it cannot just as easily set the matching MAC address when wring the XML doc. It can still treat both NICs as having a different MAC address in its own internal code. All it has to do is use the matching MAC address when writing out the XML config it gives to libvirt. I know oVirt has a facility for hook scripts that are add-ons which can arbitrarily munge the XML that VDSM creates. So AFAICT this doesn't even need to involve changes to VDSM itself. There can be a hook script which looks for the config indicating a failover pair, and rewrites the XML to have the matching MAC addr. Such a workaround then only needs to exist for as long as the mgmt app has this problematic limitation without impacting libvirt's maint. So I don't want to take this application specific hack in libvirt. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Jan 20, 2020 at 8:33 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Sun, Jan 19, 2020 at 10:24:19PM -0500, Laine Stump wrote: > > Current virtio-net drivers that support the failover feature match up > > the virtio backup device with its corresponding hostdev device by > > looking for an interface with a matching MAC address. Since libvirt > > will assign a different random MAC address to each interface that > > isn't given an explicit MAC address, this means that the configuration > > for any failover pairs must include explicit matching MAC addresses. > > > > To make life easier, we could have libvirt populate the XML config > > with the same auto-generated MAC address for both interfaces when it > > detects a failover pair that have no MAC addresses provided (a > > failover pair can be detected by matching <alias name='blah'/> of the > > virtio interface with <driver backupAlias='blah'/> of the hostdev > > interface), but then we would be stuck with that behavior even if the > > virtio guest driver later eliminated the requirement that mac > > addresses match. > > > > Additionally, some management software uses the MAC address as the > > primary index for its list of network devices, and can't properly deal > > with two interfaces having the same MAC address (oVirt). Even > > libvirt's own virsh utility uses MAC address (combined with interface > > type) to uniquely identify interfaces for the virsh detach-interface > > command (in this case, fortunately the runtime interface type is used, > > so one of the interfaces will always be of type='hostdev' and the > > other type='something-else", so it doesn't currently cause any problem). > > > > In order to remove the necessity of explicitly setting interface MAC > > addresses, as well as permit the two interfaces of a failover pair to > > each have a unique index while still fulfilling the current guest > > driver requirement that the MAC addresses matchin the guest, this > > patch adds a new attribute "useBackupMAC" that is set on the hostdev > > interface of the pair. When useBackupMAC='yes', the setup for the > > hostdev interface will find the virtio failover interface (using > > backupAlias) and use that interface's MAC address to initialize the > > MAC address of the hostdev interface; the MAC address in the hostdev > > interface config remains unchanged, it just isnt used for device > > initialization. > > > > I made this patch to followup on > > https://bugzilla.redhat.com/show_bug.cgi?id=1693587#c12 (where I > > suggested this attribute as a possible remedy to oVirt's requirement > > that each network device have a unique MAC address). > > > > Truthfully, I'm not convinced that I want it though, as it seems > > "a bit" hackish. In particular, I've thought for a long time that the > > "hostdev manager" code in util/virhostdev.c should really live in the > > node device driver and be called from the hypervisors via a public API > > (so that there is one central place on the host that maintains the > > list of in-use PCI devices and their status), but this patch adds an > > obstacle to that goal by adding a virDomainDefPtr to more of the APIs in > > that library - if this was turned into a public API, then entire > > virDomainDef would need to be serialized and sent in the API call, > > then parsed at the other end - yuck :-/. NB: there are already > > functions in virhostdev.h that take a virDomainDefPtr, so maybe I'm > > being too sensitive. > > > > On the upside, it solves a problem, and default bevahior is unchanged. > > I don't believe it does solve a real problem. > > If a mgmt app is capable of setting useBackupMAC=yes when writing the > XML doc, then I don't see why it cannot just as easily set the matching > MAC address when wring the XML doc. > > It can still treat both NICs as having a different MAC address in its > own internal code. All it has to do is use the matching MAC address > when writing out the XML config it gives to libvirt. > > I know oVirt has a facility for hook scripts that are add-ons which > can arbitrarily munge the XML that VDSM creates. So AFAICT this doesn't > even need to involve changes to VDSM itself. There can be a hook > script which looks for the config indicating a failover pair, and > rewrites the XML to have the matching MAC addr. > > Such a workaround then only needs to exist for as long as the mgmt > app has this problematic limitation without impacting libvirt's > maint. > > So I don't want to take this application specific hack in libvirt. I see why you can see it as a hack that should not exist in libvirt. However I would try to put out my case for it. This feature creates something very similar to an in-guest bond between an sriov interface to a virtio one. Currently, we ask end users to configure the bond, set the sriov interface as the primary interface, and allow mac-spoofing on the virio interface. To me, the purpose of this feature is to remove the need for end-user intervention. The bond device no longer need to be created in the guest, it can be configured by management on the host side. Defining bonds in Linux is an established procedure. You select pre-existing interfaces, each with its different mac address, pick up a bonding mode, pick up a master interface, pick up the active mac address of the bond and start using it. I'd like to have the same experience when I configure this new type of bonding via libvirt. It just feels right to have a couple of independent interfaces, bonded together, with one selected as "master". Regards, Dan. > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Tue, Jan 21, 2020 at 12:46:38PM +0200, Dan Kenigsberg wrote: > On Mon, Jan 20, 2020 at 8:33 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Sun, Jan 19, 2020 at 10:24:19PM -0500, Laine Stump wrote: > > > Current virtio-net drivers that support the failover feature match up > > > the virtio backup device with its corresponding hostdev device by > > > looking for an interface with a matching MAC address. Since libvirt > > > will assign a different random MAC address to each interface that > > > isn't given an explicit MAC address, this means that the configuration > > > for any failover pairs must include explicit matching MAC addresses. > > > > > > To make life easier, we could have libvirt populate the XML config > > > with the same auto-generated MAC address for both interfaces when it > > > detects a failover pair that have no MAC addresses provided (a > > > failover pair can be detected by matching <alias name='blah'/> of the > > > virtio interface with <driver backupAlias='blah'/> of the hostdev > > > interface), but then we would be stuck with that behavior even if the > > > virtio guest driver later eliminated the requirement that mac > > > addresses match. > > > > > > Additionally, some management software uses the MAC address as the > > > primary index for its list of network devices, and can't properly deal > > > with two interfaces having the same MAC address (oVirt). Even > > > libvirt's own virsh utility uses MAC address (combined with interface > > > type) to uniquely identify interfaces for the virsh detach-interface > > > command (in this case, fortunately the runtime interface type is used, > > > so one of the interfaces will always be of type='hostdev' and the > > > other type='something-else", so it doesn't currently cause any problem). > > > > > > In order to remove the necessity of explicitly setting interface MAC > > > addresses, as well as permit the two interfaces of a failover pair to > > > each have a unique index while still fulfilling the current guest > > > driver requirement that the MAC addresses matchin the guest, this > > > patch adds a new attribute "useBackupMAC" that is set on the hostdev > > > interface of the pair. When useBackupMAC='yes', the setup for the > > > hostdev interface will find the virtio failover interface (using > > > backupAlias) and use that interface's MAC address to initialize the > > > MAC address of the hostdev interface; the MAC address in the hostdev > > > interface config remains unchanged, it just isnt used for device > > > initialization. > > > > > > I made this patch to followup on > > > https://bugzilla.redhat.com/show_bug.cgi?id=1693587#c12 (where I > > > suggested this attribute as a possible remedy to oVirt's requirement > > > that each network device have a unique MAC address). > > > > > > Truthfully, I'm not convinced that I want it though, as it seems > > > "a bit" hackish. In particular, I've thought for a long time that the > > > "hostdev manager" code in util/virhostdev.c should really live in the > > > node device driver and be called from the hypervisors via a public API > > > (so that there is one central place on the host that maintains the > > > list of in-use PCI devices and their status), but this patch adds an > > > obstacle to that goal by adding a virDomainDefPtr to more of the APIs in > > > that library - if this was turned into a public API, then entire > > > virDomainDef would need to be serialized and sent in the API call, > > > then parsed at the other end - yuck :-/. NB: there are already > > > functions in virhostdev.h that take a virDomainDefPtr, so maybe I'm > > > being too sensitive. > > > > > > On the upside, it solves a problem, and default bevahior is unchanged. > > > > I don't believe it does solve a real problem. > > > > If a mgmt app is capable of setting useBackupMAC=yes when writing the > > XML doc, then I don't see why it cannot just as easily set the matching > > MAC address when wring the XML doc. > > > > It can still treat both NICs as having a different MAC address in its > > own internal code. All it has to do is use the matching MAC address > > when writing out the XML config it gives to libvirt. > > > > I know oVirt has a facility for hook scripts that are add-ons which > > can arbitrarily munge the XML that VDSM creates. So AFAICT this doesn't > > even need to involve changes to VDSM itself. There can be a hook > > script which looks for the config indicating a failover pair, and > > rewrites the XML to have the matching MAC addr. > > > > Such a workaround then only needs to exist for as long as the mgmt > > app has this problematic limitation without impacting libvirt's > > maint. > > > > So I don't want to take this application specific hack in libvirt. > > I see why you can see it as a hack that should not exist in libvirt. > However I would try to put out my case for it. This feature creates > something very similar to an in-guest bond between an sriov interface > to a virtio one. Currently, we ask end users to configure the bond, > set the sriov interface as the primary interface, and allow > mac-spoofing on the virio interface. To me, the purpose of this > feature is to remove the need for end-user intervention. The bond > device no longer need to be created in the guest, it can be configured > by management on the host side. Defining bonds in Linux is an > established procedure. You select pre-existing interfaces, each with > its different mac address, pick up a bonding mode, pick up a master > interface, pick up the active mac address of the bond and start using > it. I'd like to have the same experience when I configure this new > type of bonding via libvirt. It just feels right to have a couple of > independent interfaces, bonded together, with one selected as > "master". This is comparing apples & oranges IMHO. It is comparing what is done as an end user in the guest with what is done as an internal config option in libvirt. If you want to compare the user experiance, then the comparison is between the guest setup and the RHEV user interface experiance. AFAICT, we can achieve the desired user experiance in RHEV and there's no functional reason to need this patch. It is just adding a policy control knob that doesn't have any impact on what it is possible to configure for the guest, while adding to maint burden of libvirt. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Jan 21, 2020 at 4:47 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Jan 21, 2020 at 12:46:38PM +0200, Dan Kenigsberg wrote:
> > On Mon, Jan 20, 2020 at 8:33 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Sun, Jan 19, 2020 at 10:24:19PM -0500, Laine Stump wrote:
> > > > Current virtio-net drivers that support the failover feature match up
> > > > the virtio backup device with its corresponding hostdev device by
> > > > looking for an interface with a matching MAC address. Since libvirt
> > > > will assign a different random MAC address to each interface that
> > > > isn't given an explicit MAC address, this means that the configuration
> > > > for any failover pairs must include explicit matching MAC addresses.
> > > >
> > > > To make life easier, we could have libvirt populate the XML config
> > > > with the same auto-generated MAC address for both interfaces when it
> > > > detects a failover pair that have no MAC addresses provided (a
> > > > failover pair can be detected by matching <alias name='blah'/> of the
> > > > virtio interface with <driver backupAlias='blah'/> of the hostdev
> > > > interface), but then we would be stuck with that behavior even if the
> > > > virtio guest driver later eliminated the requirement that mac
> > > > addresses match.
> > > >
> > > > Additionally, some management software uses the MAC address as the
> > > > primary index for its list of network devices, and can't properly deal
> > > > with two interfaces having the same MAC address (oVirt). Even
> > > > libvirt's own virsh utility uses MAC address (combined with interface
> > > > type) to uniquely identify interfaces for the virsh detach-interface
> > > > command (in this case, fortunately the runtime interface type is used,
> > > > so one of the interfaces will always be of type='hostdev' and the
> > > > other type='something-else", so it doesn't currently cause any problem).
> > > >
> > > > In order to remove the necessity of explicitly setting interface MAC
> > > > addresses, as well as permit the two interfaces of a failover pair to
> > > > each have a unique index while still fulfilling the current guest
> > > > driver requirement that the MAC addresses matchin the guest, this
> > > > patch adds a new attribute "useBackupMAC" that is set on the hostdev
> > > > interface of the pair. When useBackupMAC='yes', the setup for the
> > > > hostdev interface will find the virtio failover interface (using
> > > > backupAlias) and use that interface's MAC address to initialize the
> > > > MAC address of the hostdev interface; the MAC address in the hostdev
> > > > interface config remains unchanged, it just isnt used for device
> > > > initialization.
> > > >
> > > > I made this patch to followup on
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1693587#c12 (where I
> > > > suggested this attribute as a possible remedy to oVirt's requirement
> > > > that each network device have a unique MAC address).
> > > >
> > > > Truthfully, I'm not convinced that I want it though, as it seems
> > > > "a bit" hackish. In particular, I've thought for a long time that the
> > > > "hostdev manager" code in util/virhostdev.c should really live in the
> > > > node device driver and be called from the hypervisors via a public API
> > > > (so that there is one central place on the host that maintains the
> > > > list of in-use PCI devices and their status), but this patch adds an
> > > > obstacle to that goal by adding a virDomainDefPtr to more of the APIs in
> > > > that library - if this was turned into a public API, then entire
> > > > virDomainDef would need to be serialized and sent in the API call,
> > > > then parsed at the other end - yuck :-/. NB: there are already
> > > > functions in virhostdev.h that take a virDomainDefPtr, so maybe I'm
> > > > being too sensitive.
> > > >
> > > > On the upside, it solves a problem, and default bevahior is unchanged.
> > >
> > > I don't believe it does solve a real problem.
> > >
> > > If a mgmt app is capable of setting useBackupMAC=yes when writing the
> > > XML doc, then I don't see why it cannot just as easily set the matching
> > > MAC address when wring the XML doc.
> > >
> > > It can still treat both NICs as having a different MAC address in its
> > > own internal code. All it has to do is use the matching MAC address
> > > when writing out the XML config it gives to libvirt.
> > >
> > > I know oVirt has a facility for hook scripts that are add-ons which
> > > can arbitrarily munge the XML that VDSM creates. So AFAICT this doesn't
> > > even need to involve changes to VDSM itself. There can be a hook
> > > script which looks for the config indicating a failover pair, and
> > > rewrites the XML to have the matching MAC addr.
> > >
> > > Such a workaround then only needs to exist for as long as the mgmt
> > > app has this problematic limitation without impacting libvirt's
> > > maint.
> > >
> > > So I don't want to take this application specific hack in libvirt.
> >
> > I see why you can see it as a hack that should not exist in libvirt.
> > However I would try to put out my case for it. This feature creates
> > something very similar to an in-guest bond between an sriov interface
> > to a virtio one. Currently, we ask end users to configure the bond,
> > set the sriov interface as the primary interface, and allow
> > mac-spoofing on the virio interface. To me, the purpose of this
> > feature is to remove the need for end-user intervention. The bond
> > device no longer need to be created in the guest, it can be configured
> > by management on the host side. Defining bonds in Linux is an
> > established procedure. You select pre-existing interfaces, each with
> > its different mac address, pick up a bonding mode, pick up a master
> > interface, pick up the active mac address of the bond and start using
> > it. I'd like to have the same experience when I configure this new
> > type of bonding via libvirt. It just feels right to have a couple of
> > independent interfaces, bonded together, with one selected as
> > "master".
>
> This is comparing apples & oranges IMHO. It is comparing what is done
> as an end user in the guest with what is done as an internal config
> option in libvirt. If you want to compare the user experiance, then
> the comparison is between the guest setup and the RHEV user interface
> experiance. AFAICT, we can achieve the desired user experiance in RHEV
> and there's no functional reason to need this patch. It is just adding
> a policy control knob that doesn't have any impact on what it is possible
> to configure for the guest, while adding to maint burden of libvirt.
IMHO there are three layers of apples here.
In oVirt or KubeVirt, maybe also in OpenStack, a VM owner would like
to define to virtual interfaces. One that is primary, based on SR-IOV.
The other is secondary, connected via virtio to a logical network to
their choosing. They would to specify the second interface as a backup
for the first one. They don't really care about the mac address of the
interfaces.
In the guest, oVirt users can currently do just this using normal Linux bonding.
IMHO a user of virsh or virt-manager would like to do that, too. He or
she should not bother setting the mac address of the two interfaces;
no human like to do that. They rather state which interface is the
backup of another one.
I think that what you see as a maintenance burden stems from a design
mistake^Wdecision in virtio, which expresses the "x is the backup of
y" as "x and y have the same mac address and x is virtio". I
(egotistically?) think of libvirt as the human-accessible API for
virtualization, that let me express what I want, with little leakage
of implementation issues.
I'll try to express the user experience I envisage in KubeVirt terms:
kind: VM
spec:
domain:
devices:
interfaces:
- name: fast
sriov: {}
- name: backup
bridge: {}
backupOf: fast # <---- how I'd like to express the
relation between the two nics
networks:
- name: fast
multus:
networkName: sriov-vlan-100
- name: backup
multus:
networkName: ovs-vlan-100
When this is fed today to a cluster with kubemacpool [1] enabled,
kubemacpool would allocate a different mac to each of the interfaces.
KubeVirt would be able to ignore the mac of the backup interface and
feed the mac of the fast mac instead. I suppose that with enough work,
oVirt would be able to do the same. Humans using virt-manager would
curse us every time they copy a pseudorandom mac address from one nic
to another.
I think that it would be better to address this once, here in libvirt,
rather than repeat it thrice higher up the management stack.
Regards,
Dan.
[1] https://github.com/k8snetworkplumbingwg/kubemacpool
On Wed, Jan 22, 2020 at 02:59:36PM +0200, Dan Kenigsberg wrote:
> On Tue, Jan 21, 2020 at 4:47 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Tue, Jan 21, 2020 at 12:46:38PM +0200, Dan Kenigsberg wrote:
> > > On Mon, Jan 20, 2020 at 8:33 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >
> > > > On Sun, Jan 19, 2020 at 10:24:19PM -0500, Laine Stump wrote:
> > > > > Current virtio-net drivers that support the failover feature match up
> > > > > the virtio backup device with its corresponding hostdev device by
> > > > > looking for an interface with a matching MAC address. Since libvirt
> > > > > will assign a different random MAC address to each interface that
> > > > > isn't given an explicit MAC address, this means that the configuration
> > > > > for any failover pairs must include explicit matching MAC addresses.
> > > > >
> > > > > To make life easier, we could have libvirt populate the XML config
> > > > > with the same auto-generated MAC address for both interfaces when it
> > > > > detects a failover pair that have no MAC addresses provided (a
> > > > > failover pair can be detected by matching <alias name='blah'/> of the
> > > > > virtio interface with <driver backupAlias='blah'/> of the hostdev
> > > > > interface), but then we would be stuck with that behavior even if the
> > > > > virtio guest driver later eliminated the requirement that mac
> > > > > addresses match.
> > > > >
> > > > > Additionally, some management software uses the MAC address as the
> > > > > primary index for its list of network devices, and can't properly deal
> > > > > with two interfaces having the same MAC address (oVirt). Even
> > > > > libvirt's own virsh utility uses MAC address (combined with interface
> > > > > type) to uniquely identify interfaces for the virsh detach-interface
> > > > > command (in this case, fortunately the runtime interface type is used,
> > > > > so one of the interfaces will always be of type='hostdev' and the
> > > > > other type='something-else", so it doesn't currently cause any problem).
> > > > >
> > > > > In order to remove the necessity of explicitly setting interface MAC
> > > > > addresses, as well as permit the two interfaces of a failover pair to
> > > > > each have a unique index while still fulfilling the current guest
> > > > > driver requirement that the MAC addresses matchin the guest, this
> > > > > patch adds a new attribute "useBackupMAC" that is set on the hostdev
> > > > > interface of the pair. When useBackupMAC='yes', the setup for the
> > > > > hostdev interface will find the virtio failover interface (using
> > > > > backupAlias) and use that interface's MAC address to initialize the
> > > > > MAC address of the hostdev interface; the MAC address in the hostdev
> > > > > interface config remains unchanged, it just isnt used for device
> > > > > initialization.
> > > > >
> > > > > I made this patch to followup on
> > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1693587#c12 (where I
> > > > > suggested this attribute as a possible remedy to oVirt's requirement
> > > > > that each network device have a unique MAC address).
> > > > >
> > > > > Truthfully, I'm not convinced that I want it though, as it seems
> > > > > "a bit" hackish. In particular, I've thought for a long time that the
> > > > > "hostdev manager" code in util/virhostdev.c should really live in the
> > > > > node device driver and be called from the hypervisors via a public API
> > > > > (so that there is one central place on the host that maintains the
> > > > > list of in-use PCI devices and their status), but this patch adds an
> > > > > obstacle to that goal by adding a virDomainDefPtr to more of the APIs in
> > > > > that library - if this was turned into a public API, then entire
> > > > > virDomainDef would need to be serialized and sent in the API call,
> > > > > then parsed at the other end - yuck :-/. NB: there are already
> > > > > functions in virhostdev.h that take a virDomainDefPtr, so maybe I'm
> > > > > being too sensitive.
> > > > >
> > > > > On the upside, it solves a problem, and default bevahior is unchanged.
> > > >
> > > > I don't believe it does solve a real problem.
> > > >
> > > > If a mgmt app is capable of setting useBackupMAC=yes when writing the
> > > > XML doc, then I don't see why it cannot just as easily set the matching
> > > > MAC address when wring the XML doc.
> > > >
> > > > It can still treat both NICs as having a different MAC address in its
> > > > own internal code. All it has to do is use the matching MAC address
> > > > when writing out the XML config it gives to libvirt.
> > > >
> > > > I know oVirt has a facility for hook scripts that are add-ons which
> > > > can arbitrarily munge the XML that VDSM creates. So AFAICT this doesn't
> > > > even need to involve changes to VDSM itself. There can be a hook
> > > > script which looks for the config indicating a failover pair, and
> > > > rewrites the XML to have the matching MAC addr.
> > > >
> > > > Such a workaround then only needs to exist for as long as the mgmt
> > > > app has this problematic limitation without impacting libvirt's
> > > > maint.
> > > >
> > > > So I don't want to take this application specific hack in libvirt.
> > >
> > > I see why you can see it as a hack that should not exist in libvirt.
> > > However I would try to put out my case for it. This feature creates
> > > something very similar to an in-guest bond between an sriov interface
> > > to a virtio one. Currently, we ask end users to configure the bond,
> > > set the sriov interface as the primary interface, and allow
> > > mac-spoofing on the virio interface. To me, the purpose of this
> > > feature is to remove the need for end-user intervention. The bond
> > > device no longer need to be created in the guest, it can be configured
> > > by management on the host side. Defining bonds in Linux is an
> > > established procedure. You select pre-existing interfaces, each with
> > > its different mac address, pick up a bonding mode, pick up a master
> > > interface, pick up the active mac address of the bond and start using
> > > it. I'd like to have the same experience when I configure this new
> > > type of bonding via libvirt. It just feels right to have a couple of
> > > independent interfaces, bonded together, with one selected as
> > > "master".
> >
> > This is comparing apples & oranges IMHO. It is comparing what is done
> > as an end user in the guest with what is done as an internal config
> > option in libvirt. If you want to compare the user experiance, then
> > the comparison is between the guest setup and the RHEV user interface
> > experiance. AFAICT, we can achieve the desired user experiance in RHEV
> > and there's no functional reason to need this patch. It is just adding
> > a policy control knob that doesn't have any impact on what it is possible
> > to configure for the guest, while adding to maint burden of libvirt.
>
> IMHO there are three layers of apples here.
> In oVirt or KubeVirt, maybe also in OpenStack, a VM owner would like
> to define to virtual interfaces. One that is primary, based on SR-IOV.
> The other is secondary, connected via virtio to a logical network to
> their choosing. They would to specify the second interface as a backup
> for the first one. They don't really care about the mac address of the
> interfaces.
> In the guest, oVirt users can currently do just this using normal Linux bonding.
> IMHO a user of virsh or virt-manager would like to do that, too. He or
> she should not bother setting the mac address of the two interfaces;
> no human like to do that. They rather state which interface is the
> backup of another one.
>
> I think that what you see as a maintenance burden stems from a design
> mistake^Wdecision in virtio, which expresses the "x is the backup of
> y" as "x and y have the same mac address and x is virtio". I
> (egotistically?) think of libvirt as the human-accessible API for
> virtualization, that let me express what I want, with little leakage
> of implementation issues.
I think this is where the disconnect is. The libvirt API is not aiming
to provide human targetted convenience. It is intending to provide a
machine targetted functional mechanism with clear semantics, on which
applications can construct human targetted virtualization solutions.
Human convenience entails making a large number of usage policy
decisions based on criteria that are appropriate for the application's
use cases & knowledge of the guest OS needs.
> I'll try to express the user experience I envisage in KubeVirt terms:
>
> kind: VM
> spec:
> domain:
> devices:
> interfaces:
> - name: fast
> sriov: {}
> - name: backup
> bridge: {}
> backupOf: fast # <---- how I'd like to express the
> relation between the two nics
> networks:
> - name: fast
> multus:
> networkName: sriov-vlan-100
> - name: backup
> multus:
> networkName: ovs-vlan-100
Yes, this is a perfectly sane way to expose teaming in KubeVirt.
What you're showing here is the KubeVirt config though, and this
will be converted into a libvirt guest XML config, at which time
KubeVirt can choose to provide a matching MAC address for the two
NICs it has requested.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2026 Red Hat, Inc.