On 01/24/2019 09:48 AM, Andrea Bolognani wrote:
> On Wed, 2019-01-23 at 13:30 -0500, Cole Robinson wrote:
>> On 01/22/2019 12:32 PM, Cole Robinson wrote:
>>> On 01/21/2019 11:20 AM, Andrea Bolognani wrote:
>>>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
>>>>> @@ -917,6 +917,15 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>>>>> case VIR_DOMAIN_DEVICE_INPUT:
>>>>> switch ((virDomainInputBus) dev->data.input->bus) {
>>>>> case VIR_DOMAIN_INPUT_BUS_VIRTIO:
>>>>> + switch ((virDomainInputModel) dev->data.input->model) {
>>>>> + case VIR_DOMAIN_INPUT_MODEL_VIRTIO_TRANSITIONAL:
>>>>> + return pciFlags;
>>>>> + case VIR_DOMAIN_INPUT_MODEL_VIRTIO:
>>>>> + case VIR_DOMAIN_INPUT_MODEL_VIRTIO_NON_TRANSITIONAL:
>>>>> + case VIR_DOMAIN_INPUT_MODEL_DEFAULT:
>>>>> + case VIR_DOMAIN_INPUT_MODEL_LAST:
>>>>> + break;
>>>>> + }
>>>>> return virtioFlags;
>>>>
>>>> VIR_DOMAIN_INPUT_MODEL_DEFAULT and VIR_DOMAIN_INPUT_MODEL_LAST
>>>> should result in 0 rather than virtioFlags being returned.
>>>
>>> Oops good catch
>>
>> Actually this missed a case: at least DEFAULT (meaning no model=
>> specified) should return virtioFlags, since this block is already
>> conditional on bus='virtio'. I'll have DEFAULT return virtioFlags and
>> LAST return 0;
>
> Now that you mention it, for devices such as <input> and <disk>,
> where we're introducing the model attribute just now, we should make
> sure that we expand the default correctly, such that eg.
>
> <disk type='file' device='disk'>
> <target dev='vda' bus='virtio'/>
> </disk>
>
> is formatted back as
>
> <disk type='file' device='disk' model='virtio'>
> <target dev='vda' bus='virtio'/>
> </disk>
>
> We might have to be careful when formatting the XML for migration
> and scrub model='virtio' in that case in order not to break
> backwards migration, as we already do for plenty of other cases.
>
> Once that's done, then you'll be guaranteed the model above is
> never going to be DEFAULT.
>
IMO extending the XML we generate by default should only be done for
very good reasons, the migration compat issue is not a trivial extension
and comes with issues of its own.
For example, say we turn model=DEFAULT into model=VIRTIO in the qemu
driver. In generic domain format code, we would do:
if (!MIGRATABLE || disk->model != VIRTIO)
<format disk->model in the XML>
But the MIGRATABLE checking is very crude. It doesn't consider version
or capabilities of the remote libvirt. We could be migrating to an
identical qemu and libvirt version, but we still drop the model=VIRTIO
from the XML. Basically for the indefinite future, disk->model=VIRTIO in
the XML is never going to be accounted for in migration. This can cause
theoretical future issues if non-qemu drivers (or other qemu configs)
support model=VIRTIO but it's not their default. For those non-qemu
drivers, if disk->model=VIRTIO is present in the XML, the user
explicitly requested it, but generic formatting code will never send the
value during migration.
Let's say we skip the MIGRATABLE check. In fact for most of the cases in
this patch series (except maybe for <controller> devs), this would work
fine. If we are migrating to same or newer libvirt+qemu, all is good. If
we migrate to older libvirt, it won't know to check disk->model at all
so won't complain, maybe migration would fail due to qemu compat issues
but that's unavoidable.
However, the NEXT time we make a similar change, let's say filling in
disk->model=FOO for bus=ide, or more likely, input->model=QEMUTABLET for
input->bus=usb + input->type=tablet. Now we have to blacklist
model=QEMUTABLET from the migratable XML for the indefinite future,
because older libvirt _may_ know enough to parse input->model, but
doesn't have QEMUTABLET in the enum, and parsing fails. Okay, this is
what the MIGRATABLE flag is all about. But it's exposing us to the issue
with other drivers mentioned above.
And this model=QEMUTABLET MIGRATABLE issue is theoretical but it's an
exact case that has happened before, with controller models at least.
Basically once you open that door of adding new default output to the
XML for common cases, you can't ever close it again for that enum property.
Plus there's other downsides for outputting new properties by default:
* massive test suite churn: every single virtio disk in the XML2XML
tests will now spit out model='virtio', plus all the other devices. This
makes backporting suck
* libvirt downgrade issues. Previous times we added new output, like
input type=keyboard, there were lots of complaints like: I upgraded
libvirt via virt-preview to see if it fixed a bug, type=keyboard was
added by default, I downgraded libvirt back to regular distro version,
my VM disappeared, because the XML parser choked on unknown
type=keyboard. Again in this particular case it likely won't cause those
issues because model= is new everywhere, but the pattern basically can't
be extended forward beyond this one instance.
Here's the benefits I see of outputing model=virtio by default
* Reports more accurate XML. Minor IMO.
* Better chance of maintaining qemu compat across libvirt/qemu upgrades.
Encoding model=virtio means we will always try that. Hypothetically 5
years from now libvirt starts defaulting to
model=virtio-non-transitional, you might upgrade libvirt and your
windows virtio VM doesn't boot. Yeah that's not nice, but it's fixable
(user sets model=virtio in the XML). Same reasoning basically applies to
migration compat as well
I don't think the benefits outweigh the downsides. Of course I may be
missing plenty of things so please correct me if I'm wrong
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
