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

[Boris Fiuczynski]

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 at linux.ibm.com> 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 at redhat.com> 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 at redhat.com> 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 at redhat.com>
>>>>>>>> ---
>>>>>>>>    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.

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294