Re: [libvirt PATCH 12/12] conf/qemu: new <driver> attribute "useBackupMAC"

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(a)redhat.com&gt; 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.