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

On Fri, Jul 30, 2021 at 8:01 AM Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; wrote: > > 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. If I understand your suggestion, I don't think it solves any problems. It seems to me that you're saying that libvirt should ignore all mdev definitions from mdevctl if the parent does not (yet?) exist on the host. (At least that's my interpretation of your phrase "definitions which are not a usable host resource"). But it's still possible to have definitions with duplicate UUIDs for devices which have parents that *do* exist on the host but are simply not instantiated yet. If I'm misinterpreting your comment, feel free to clarify.