On 2/6/19 12:03 PM, Daniel P. Berrangé wrote:
> On Thu, Jan 24, 2019 at 12:05:11PM -0500, Cole Robinson wrote:
>> 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
>
> Realistically we can never change the defaults for anything in libvirt,
> it has to be done at a layer above.
>
> Previously when we've added elements to the XML that were not previously
> present, we have done this to capture some aspect of the guest ABI that
> was not previously recorded by libvirt. I see value in that.
>
> I don't think that is actually the case this time though. The model=virtio
> addition is just duplcating what is already perfectly well expressed in
> the existing bus=virtio attribute. There's no aspect of the guest ABI
> that was missing that we've added here.
>
> Only model=virtio-transitional / model=virtio-non-transitional add new
> information wrt the ABI. model=virtio is merely a redundant convenience
> we've added for sake of symmetry.
>
> So on this basis, I think there's not a compelling reason to output it
> in the XML if the user didn't explicitly set it themselves.
>
Okay thanks for the response, I'll skip this for my series then. FWIW if
we change our minds later I don't think there's any special downsides to
doing it in a follow up series.
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
