Fwd: Re: [PATCH virt-install v1 3/3] virtxml: prevent defining same hostdev again

Fri May 28 20:05:42 UTC 2021

On 5/28/21 8:06 AM, Shalini Chellathurai Saroja wrote:
> 
> On 5/19/21 12:49 AM, Cole Robinson wrote:
>> On 4/14/21 11:18 AM, Shalini Chellathurai Saroja wrote:
>>> Currently, it is possible to add same device multiple times, when the
>>> guest domain is in shut-off state. This patch prevents this unexpected
>>> behavior for all hostdev devices, including mdev devices.
>>>
>> If this is an invalid config, these types of validation checks should
>> live in libvirt so all clients benefit from them, and libvirt is the one
>> source of truth for what's supported or not.
> 
> Hello Cole,
> 
> Thank you for the review.
> 
> This validation check is available in libvirt, for this to work, the
> flag "VIR_DOMAIN_AFFECT_CONFIG" has to be set, when the virtual server
> is not in running state.
> 

It's kind of complicated.

AFFECT_CONFIG for an offline VM (or the offline XML of a running VM)
should in theory be identical to the process we currently use: dumpxml,
modify XML, redefine. Anything else is a bug IMO. My reasoning is that
if AFFECT_CONFIG does anything special besides hitting the XML define
code path, then there's logic locked into the AFFECT_CONFIG code path
which isn't available to the many other ways of editing the XML.

So for example, if there is a validation check on coldplugging mdev
devices in libvirt, and it's only accessible through the attachDevice
API, that's a libvirt bug IMO. If the check is worth doing at coldplug
time, it's also worth doing at XML define time, and should be part of
the generic virDomainDefValidate infrastructure. If it's not worth doing
at XML validate time for some reasons, there should be no argument for
why it makes sense at coldplug time.

That's my theory anyways. I think other libvirt devs will agree but I'm
not sure.

AFFECT_CONFIG in general is not a good fit for virt-install, for the
reasons you show below: we can't rely on it being implemented
consistently. It's not implemented in the test driver, and it's not
implemented for all devices even in the qemu driver. So we would have to
either punt the option to the user by explicitly exposing the flag, or
try to keep track internally which cases it works for and what cases it
doesn't, neither of which ideas I like.

So I think if for this case, libvirt already has the validation you
want, but it's only in the attachDevice code path, that's an opportunity
to improve libvirt. Move that check to
domain_validate.c:virDomainHostdevDefValidate, or possibly to
virDomainDefValidateInternal , do something like what
virDomainDefDuplicateDiskInfoValidate does

Hopefully that makes sense

Thanks,
Cole

> With the below changes, libvirt restricts addition of same device
> multiple times.
> 
> diff --git a/virtinst/virtxml.py b/virtinst/virtxml.py
> index 0c8da37e..6de66488 100644
> --- a/virtinst/virtxml.py
> +++ b/virtinst/virtxml.py
> @@ -222,12 +222,24 @@ def setup_device(dev):
>      dev.build_storage(cli.get_meter())
> 
> 
> -def define_changes(conn, inactive_xmlobj, devs, action, confirm):
> +def define_changes(domain, conn, inactive_xmlobj, devs, action, confirm):
>      if confirm:
>          if not prompt_yes_or_no(
>                  _("Define '%s' with the changed XML?") %
> inactive_xmlobj.name):
>              return False
> 
> +    if action == "coldplug":
> +        msg_fail = _("Error attempting device coldplug: %(error)s")
> +
> +        for dev in devs:
> +            xml = dev.get_xml()
> +            setup_device(dev)
> +
> +            try:
> +                domain.attachDeviceFlags(xml,
> libvirt.VIR_DOMAIN_AFFECT_CONFIG)
> +            except libvirt.libvirtError as e:
> +                fail(msg_fail % {"error": e})
> +
>      if action == "hotplug":
>          for dev in devs:
>              setup_device(dev)
> @@ -324,7 +336,10 @@ def prepare_changes(xmlobj, options, parserclass):
> 
>      elif options.add_device:
>          devs = action_add_device(xmlobj, options, parserclass)
> -        action = "hotplug"
> +        if options.update:
> +            action = "hotplug"
> +        else:
> +            action = "coldplug"
> 
>      elif options.remove_device:
>          devs = action_remove_device(xmlobj, options, parserclass)
> @@ -524,7 +539,7 @@ def main(conn=None):
>                                     action, options.confirm)
>          return 0
> 
> -    dom = define_changes(conn, inactive_xmlobj,
> +    dom = define_changes(domain, conn, inactive_xmlobj,
>                           devs, action, options.confirm)
>      if not dom:
>          # --confirm user said 'no'
> 
> However, some tests fail as shown below because the flag is not
> supported by the test driver.
> 
> ERROR    Error attempting device coldplug: this function is not
> supported by the connection driver: virDomainAttachDeviceFlags
> 
> TESTSUITE: Expected command to pass, but it didn't.
> 
> 
> ============================= short test summary info
> =============================
> FAILED tests/test_cli.py::testCLI0455virt_xml - AssertionError: Command
> was: ./v...
> FAILED tests/test_cli.py::testCLI0458virt_xml_add_host_device -
> AssertionError: ...
> FAILED tests/test_cli.py::testCLI0459virt_xml_add_sound -
> AssertionError: Comman...
> FAILED tests/test_cli.py::testCLI0460virt_xml_add_disk_basic -
> AssertionError: C...
> FAILED tests/test_cli.py::testCLI0461virt_xml_add_disk_notarget -
> AssertionError...
> FAILED tests/test_cli.py::testCLI0462virt_xml_add_disk_create_storage -
> Assertio...
> FAILED tests/test_cli.py::testCLI0463virt_xml_add_disk_default_storage -
> Asserti...
> FAILED tests/test_cli.py::testCLI0470virt_xml_add_hostdev_mdev -
> AssertionError:...
> FAILED tests/test_cli.py::testCLI0474virt_xml_add_host_device_start -
> AssertionE...
> FAILED tests/test_cli.py::testCLI0479virt_xml_kvm_add_disk_os_from_xml -
> Asserti...
> FAILED
> tests/test_cli.py::testCLI0480virt_xml_kvm_add_disk_os_from_cmdline - Ass...
> FAILED
> tests/test_cli.py::testCLI0481virt_xml_kvm_add_network_os_from_xml - Asse...
> FAILED
> tests/test_cli.py::testCLI0482virt_xml_kvm_add_network_os_from_cmdline - ...
> =================== 13 failed, 582 passed, 2 skipped in 14.01s
> ====================
> 
> Is it ok to add these tests as invalid like the below example?
> 
> c.add_invalid("test-for-virtxml --add-device --host-device 0x04b3:0x4485
> --update --confirm", input_text="yes")  # test driver doesn't support
> attachdevice...
> 
> Please let me know your comments.
> 
>>
>> We make some exceptions in virt-install/virt-xml to validate common
>> configuration errors but I don't think this is one of them.
>>
>> Thanks,
>> Cole
>>
> -- 
> Kind regards
> Shalini Chellathurai Saroja
> Linux on Z and Virtualization Development
> Vorsitzende des Aufsichtsrats: Gregor Pillen
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
> 

- Cole