Re: [libvirt PATCH 6/7] nodedev: Handle inactive mdevs with the same UUID

On 7/30/21 9:48 AM, Michal Prívozník wrote: > On 7/29/21 9:27 PM, Jonathon Jongsma wrote: >> On Thu, Jul 29, 2021 at 1:35 PM Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; wrote: >>> >>> On 7/27/21 4:09 PM, Jonathon Jongsma wrote: >>>> On Tue, Jul 27, 2021 at 3:02 AM Michal Prívozník <mprivozn(a)redhat.com&gt; wrote: >>>>> >>>>> On 7/27/21 12:08 AM, Jonathon Jongsma wrote: >>>>>> On Mon, Jul 26, 2021 at 9:47 AM Michal Prívozník <mprivozn(a)redhat.com&gt; wrote: >>>>>>> >>>>>>> On 7/23/21 6:40 PM, Jonathon Jongsma wrote: >>>>>>>> Unfortunately, mdevctl supports defining more than one mdev with the >>>>>>>> same UUID as long as they have different parent devices. (Only one of >>>>>>>> these devices can be active at any given time). >>>>>>>> >>>>>>>> This means that we can't use the UUID alone as a way to uniquely >>>>>>>> identify mdev node devices. Append the parent address to ensure >>>>>>>> uniqueness. For example: >>>>>>>> >>>>>>>> Before: mdev_88a6b868_46bd_4015_8e5b_26107f82da38 >>>>>>>> After: mdev_88a6b868_46bd_4015_8e5b_26107f82da38_0000_00_02_0 >>>>>>>> >>>>>>>> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1979440 >>>>>>>> >>>>>>>> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com&gt; >>>>>>>> --- >>>>>>>> src/node_device/node_device_driver.c | 3 ++- >>>>>>>> src/node_device/node_device_udev.c | 2 +- >>>>>>>> tests/nodedevmdevctldata/mdevctl-list-multiple.out.xml | 8 ++++---- >>>>>>>> 3 files changed, 7 insertions(+), 6 deletions(-) >>>>>>> >>>>>>> The patch looks good, but for some reason it leaves API breakage >>>>>>> aftertaste. But I guess we don't document anywhere what MDEV <name/> >>>>>>> looks like, do we? IOW - we are free to change it if needed. >>>>>> >>>>>> >>>>>> That is true -- it does have a bit of a bad aftertaste. As far as I >>>>>> know, we haven't documented or promised any specific naming format for >>>>>> mdevs. But it could certainly cause some pain for people that are >>>>>> using mdevs already, which might be reason enough to reject or adjust >>>>>> this patch. If a person already has assigned a mdev device to their vm >>>>>> with the old name, the name would change when upgrading libvirt. That >>>>>> could make the domain definition become invalid. >>>>>> >>>>>> But, we have to deal with this situation somehow. We don't have a >>>>>> choice but to handle the case where mdevctl might return multiple >>>>>> defined devices with the same UUID. I considered a couple of other >>>>>> approaches to handling this that I rejected, such as >>>>>> - only add a suffix to the second such device with the same UUID >>>>>> - rejected because the name of a given device depends on the order >>>>>> in which it was observed and the presence of other devices. For >>>>>> example, if we have two devices: mdev_$UUID and mdev_$UUID_$PARENT, >>>>>> and the first device was undefined, then when libvirt was restarted, >>>>>> mdev_$UUID_$PARENT would change its name to mdev_$UUID because it is >>>>>> now the only device with that UUID >>>>> >>>>> Agreed, this would not make situation any better, in fact worse. >>>>> >>>>>> - name all *active* mdevs mdev_$UUID and use a different scheme for >>>>>> inactive, defined mdevs >>>>>> - rejected because the device would change names when it changed >>>>>> from inactive to active. >>>>> >>>>> Yeah, this is equally horrible. >>>>> >>>>>> - ignore this situation because probably almost nobody uses multiple >>>>>> devices with the same UUID >>>>>> - but do they? >>>>> >>>>> Frankly, I don't have enough experience with MDEVs, but since it's >>>>> possible to define an MDEV with an existing UUID, it is possible to >>>>> define it also for the same parent? I mean, if I'd have an MDEV capable >>>>> graphics card and I'd define two MDEVs with the same UUID they would >>>>> both have the same parent, wouldn't they? Because in that case, >>>>> appending PCI address to libvirt's name is not enough. >>>> >>>> >>>> Nope, it's the opposite in fact. mdevctl allows you to define two or >>>> more mdevs with the same UUID, but only if they have different >>>> parents. Only a single mdev with a given UUID can be activated at the >>>> same time. So among *active* devices, the UUID is indeed unique. But >>>> not among persistent device definitions. For defined devices, the UUID >>>> is guaranteed unique per parent. >>>> >>>> I discussed this with Alex a while ago and he thought that the reason >>>> that mdevctl supported this was to allow people to assign an mdev with >>>> a specific UUID to a vm, but that UUID could represent one of several >>>> (equivalent?) devices, depending on which parent device was present in >>>> the host or which device was instantiated. That feature doesn't seem >>>> very compelling to me, and Alex wasn't sure that anybody was actually >>>> using it that way. But the fact remains that mdevctl does allow people >>>> to configure things this way and it seems like we need to be prepared >>>> to deal with it. >>> >>> >>> I am questioning how a user is supposed to correlate the nodedev >>> representation with mdev_{uuid}_{parent} to a domain using the mdev like >>> <devices> >>> <hostdev mode='subsystem' type='mdev' model='vfio-ccw'> >>> <source> >>> <address uuid='{uuid}'/> >>> </source> >>> </hostdev> >>> </devices> >> >> Sorry for potentially confusing the impact of this change with my >> previous comments. I had said that domains with mdevs assigned might >> become invalid if the nodedev name changes. But that's not actually >> true. The naming that we're discussing is only related to the name of >> the device within the libvirt nodedev driver. When adding a mediated >> device to a domain, you don't use this nodedev name, you use the UUID >> directly. This is similar to the way you add a physical passthrough >> PCI device to a domain using its PCI address rather than its >> 'pci_$domain_$bus_$slot_$fn' nodedev name. >> >>> Reading uuid it tells me its universal unique >>> (https://en.wikipedia.org/wiki/Universally_unique_identifier) but now >>> this is not true for definitions which breaks the term universal in my >>> small universe since for the defined state of an mdev it becomes >>> "parental" unique... >> >> Yes, that's true. Which is why I think this is a questionable feature. But: >> A) defined devices are only *potential* devices.This is not all that >> different from the fact that a user can manually instantiate a mdev on >> parent A with a specific UUID and then later instantiate a totally >> different mdev on parent B with the exact same UUID. One UUID refers >> to different devices at different times. >> B) the question we're discussing is not whether mdevctl should allow >> duplicate UUIDs or not. mdevctl *already* allows it, so we need to >> figure out how to deal with it in libvirt. >> >> I suppose one potential possibility might be to change upstream >> mdevctl to disallow duplicate UUIDs and then require this new mdevctl >> version for libvirt. But that's a discussion we'd have to have in >> upstream mdevctl first. > > Maybe we can start the discussion, but even in libvirt you are allowed > to have two guests with the same name and UUID if they are in different > drivers (e.g. one in QEMU and one in LXC). > > Meanwhile, I think these can be pushed as we don't have much options anyway. > > Michal > I think that we have at least one option. We could rethink the approach to create nodedev objects based on mdevctl mdev definitions which actually are not a usable host resource for domains.