Bug 1581414

(In reply to Eduardo Habkost from comment #0)
> We plan to change the default machine-type to "q35" in qemu-kvm-rhev, but we
> want to ensure no management system will break when we do that.  To do this,
> we need to ensure there's no code in OpenStack that assumes an omitted
> machine-type means libvirt+QEMU will always default to "pc".
> 
> If you believe OpenStack won't break when we change the default machine-type
> to "q35", please mark this bug as TestOnly.

I think OpenStack Nova still needs work in this area.

Currently, the support for Q35 is "bare bones"; as in: if you explicitly configure a Nova Compute node to use Q35 machine type, then all instances launched from that point on that Compute node will use it.

But I learnt (from libvirt developer Andrea Bolognani) that something like the above will will probably quickly break down as soon as you try to hot-plug devices, etc. And that Q35 is just different enough from 'i440fx' that Nova has to account for both types at some point.

Note that if Nova still doesn't work well with q35, you are free to choose "pc" as the default on Nova.  The scope of this BZ is to make Nova not assume that "pc" is the default on libvirt+QEMU (which would be a bug on Nova).

You don't need to make Nova work well with q35 out of the box (which would be a feature, not a bug fix), or make q35 the default on Nova (which would also be a new feature).

Nova has no default machine type for libvirt. We allow it to be configured explicitly, but if it is not specified we have no hard-coded default.

(In reply to Matthew Booth from comment #3)
> Nova has no default machine type for libvirt. We allow it to be configured
> explicitly, but if it is not specified we have no hard-coded default.
tl;dr: From the past few hours of research, it _looks_ like Nova doesn't
any work to handle QEMU changing machine type default to 'q35' from
'pc'.  However, I was told that to make 'q35' work *correctly* in
context of PCIe, there may need to some more work.

Long
----

While what you say is correct (Nova today allows configuring machine
type either via per-image metadata property or via setting per-Compute
node config), but what Eduardo is:

    If / when QEMU switches default machine type to 'Q35', is something
    likely to break in Nova?

Let's try to answer that below.

Nova by default does *not* hard-code any machine type; it just uses
whatever libvirt gives it on default.  All it does is allow configuring
machine type in two ways: 

  (1) Disk image metadata property, so that when you boot a guest from
      that disk image, it gets the configured machine type.

  (2) Per-Compute host configuration file, so that _all_ guests launched
      on that host gets the configured machine type.

    * * *

From a discussion with Andrea Bolognani from libvirt on IRC, management
applications (like Nova) are likely to break _only_ if we have a code
pattern like:

   if (q35_explicitly_requested) {do_q35_thing();} else {do_pc_thing();}

But -- Nova doesn't have any such.  

Auditing the Nova code[+], the only place where we explicitly take 'q35'
into consideration is during PCIe ports (from
nova/virt/libvirt/driver.py), from _get_guest_config() function:

        [...]   
        # Add PCIe root port controllers for PCI Express machines
        # but only if their amount is configured
        if (CONF.libvirt.num_pcie_ports and
                ((caps.host.cpu.arch == fields.Architecture.AARCH64 and
                guest.os_mach_type.startswith('virt')) or
                (caps.host.cpu.arch == fields.Architecture.X86_64 and
                guest.os_mach_type is not None and
                'q35' in guest.os_mach_type))):
            self._guest_add_pcie_root_ports(guest)
        [...]   

The above code is fine because it is looking directly at the guest
configuration, so it will work correctly regardless of whether the guest
is 'q35', because the user explicitly asked for it or because it's the
default machine type.

Further, Nova's (also from libvirt/driver.py) _get_machine_type() is
called only from one other function, _configure_guest_by_virt_type(),
during guest definition time:

    [...]
    guest.os_mach_type = self._get_machine_type(image_meta, caps)
    [...]

Here too, _get_machine_type() is called *only* at guest definition time.
So long as we are calling  _get_machine_type() *only* at guest
definition time, we seemt to be good with respect to handling QEMU
changing default from 'pc' to 'q35'.

Correct me if I am wrong.

[*] http://git.openstack.org/cgit/openstack/nova/commit/?id=a234bbf8 --
    Allow to configure amount of PCIe ports

The amount of detail in this discussion is huge, so I will try to limit the scope of this BZ, first:

1) The original BZ mentioned deprecating "pc", but that's not being considered anymore.

2) The default in qemu-kvm-rhev is going to be changed from "pc" to "q35".  If this is going to break something for OpenStack users (anything), this needs to be addressed in OpenStack somehow.

That's all.  This doesn't mean OpenStack needs to work with q35 out of the box, but this means OpenStack might need to explicitly ask for "pc" if you know "q35" will break something for users.

Things get more complex where "break something for users" might be caused by subtle incompatibilities elsewhere: e.g. users importing existing guest images that work only with "pc", and now they won't work out of the box anymore.

It's up for you to decide how to handle this.  Maybe the easiest solution is to keep "pc" as default until we have better mechanisms for automatically choosing a smarter default after looking at a disk image.


I'll try to be answer more specific points below:

(In reply to Kashyap Chamarthy from comment #6)
> (In reply to Matthew Booth from comment #3)
> > Nova has no default machine type for libvirt. We allow it to be configured
> > explicitly, but if it is not specified we have no hard-coded default.
> tl;dr: From the past few hours of research, it _looks_ like Nova doesn't
> any work to handle QEMU changing machine type default to 'q35' from
> 'pc'.  However, I was told that to make 'q35' work *correctly* in
> context of PCIe, there may need to some more work.

This seems to mean "yes, something is going to break if the default is changed to q35".  Doesn't it?

> 
> Long
> ----
> 
> While what you say is correct (Nova today allows configuring machine
> type either via per-image metadata property or via setting per-Compute
> node config), but what Eduardo is:
> 
>     If / when QEMU switches default machine type to 'Q35', is something
>     likely to break in Nova?

Note that "something breaking in Nova" is much more specific than "something breaking for an OpenStack user".  Even if Nova is 100% bug free, users might still have problems caused by the change to q35.


> 
> Let's try to answer that below.
> 
> Nova by default does *not* hard-code any machine type; it just uses
> whatever libvirt gives it on default.  All it does is allow configuring
> machine type in two ways: 
> 
>   (1) Disk image metadata property, so that when you boot a guest from
>       that disk image, it gets the configured machine type.
> 
>   (2) Per-Compute host configuration file, so that _all_ guests launched
>       on that host gets the configured machine type.
> 
>     * * *
> 
> From a discussion with Andrea Bolognani from libvirt on IRC, management
> applications (like Nova) are likely to break _only_ if we have a code
> pattern like:
> 
>    if (q35_explicitly_requested) {do_q35_thing();} else {do_pc_thing();}
> 
> But -- Nova doesn't have any such.  

What about existing guest images that will break if booted using q35?  What about having pc-specific code that isn't even marked as pc-specific but breaks if the machine type is q35?  Finding these cases might be difficult.


> 
[...]
> Here too, _get_machine_type() is called *only* at guest definition time.
> So long as we are calling  _get_machine_type() *only* at guest
> definition time, we seemt to be good with respect to handling QEMU
> changing default from 'pc' to 'q35'.

I don't think we can be sure about that without extensive testing.  Are you sure there isn't pc-specific code not documented as pc-specific anywhere else?

But, anyway, even if OpenStack is considered ready to use q35 by default, probably the main issue (and most complex one) is breaking things for users having pc-specific disk images that worked out of the box.

[Meta comment: Given Eduardo's comment above, removing 'Triaged' and 'TestOnly' keywords, and moving the bug back to 'NEW', to not let it drop off our radar.  Matt, feel free to override this, if you think no work needs doing given comment#7.]

Changing bug summary to make the intent clearer.

I believe the best option for OpenStack is to have its own default, and keep it as "pc" initially.

Changing OpenStack's default "q35" would be a larger task that OpenStack developers should  probably plan for, but this is not in the scope of this BZ.

To further clarify what Eduardo says, the specific code pattern we want to avoid is something like

   if guest machine type == q35:
       ... do something q35 related...
   else
       ... do something pc related...

As this code pattern is assuming that not setting a machine type results in pc.

I can see exactly this bug pattern in current Nova code:

        # Add PCIe root port controllers for PCI Express machines
        # but only if their amount is configured
        if (CONF.libvirt.num_pcie_ports and
                ((caps.host.cpu.arch == fields.Architecture.AARCH64 and
                guest.os_mach_type.startswith('virt')) or
                (caps.host.cpu.arch == fields.Architecture.X86_64 and
                guest.os_mach_type is not None and
                'q35' in guest.os_mach_type))):
            self._guest_add_pcie_root_ports(guest)


This is assuming that when  guest.os_mach_type == None, then you have 'pc' machine type, which is not going to be valid in future.

Currently guest.os_mach_type is only set if the image / flavour metadata requests a non-default machine type, or if nova.conf does so.

To fix this Nova needs to make sure guest.os_mach_type is always set.

The "easy" way todo this is to just hardcode it to "pc" on x86, or other platform specific machine type.

The better way todo this is to query the libvirt capabilities XML to extract the default machine type.

(In reply to Daniel Berrange from comment #11)
> To further clarify what Eduardo says, the specific code pattern we want to
> avoid is something like
> 
>    if guest machine type == q35:
>        ... do something q35 related...
>    else
>        ... do something pc related...
> 
> As this code pattern is assuming that not setting a machine type results in
> pc.
> 
> I can see exactly this bug pattern in current Nova code:
> 
>         # Add PCIe root port controllers for PCI Express machines
>         # but only if their amount is configured
>         if (CONF.libvirt.num_pcie_ports and
>                 ((caps.host.cpu.arch == fields.Architecture.AARCH64 and
>                 guest.os_mach_type.startswith('virt')) or
>                 (caps.host.cpu.arch == fields.Architecture.X86_64 and
>                 guest.os_mach_type is not None and
>                 'q35' in guest.os_mach_type))):
>             self._guest_add_pcie_root_ports(guest)
> 
> 
> This is assuming that when  guest.os_mach_type == None, then you have 'pc'
> machine type, which is not going to be valid in future.
> 
> Currently guest.os_mach_type is only set if the image / flavour metadata
> requests a non-default machine type, or if nova.conf does so.
> 
> To fix this Nova needs to make sure guest.os_mach_type is always set.
> 
> The "easy" way todo this is to just hardcode it to "pc" on x86, or other
> platform specific machine type.
> 
> The better way todo this is to query the libvirt capabilities XML to extract
> the default machine type.

Thanks for the analysis Dan.  (I should point out that I was wrong in comment#6 on my analysis that the above code snippet is okay; it's not -- for the reasons Dan outlined.)

As discussed on IRC, Nova should aim to go with the better approach: and since Nova already supplies host capabilities into _get_machine_type(), so that we won't return 'None'.

I have opened bug 1602889 against libosinfo, and I'm adding it as a dependency of this bug.

If you plan to address this bug without libosinfo help first, I recommend opening a separate BZ against this component to ensure the component will eventually use libosinfo to choose the machine-type.  Please let me know if a separate BZ will be needed.

Important update: I have just noticed that even when using q35, the default NIC and sound card model in libvirt are legacy PCI devices that end up requiring a legacy PCI bridge to be added to the VM: rtl8139 and ich6-hda-intel.

New action items for libosinfo and all libvirt users:
* libosinfo also needs to provide a recommendation of sound hardware
* In addition to prefer Q35 when possible, management software should prefer the 'ich9-hda-intel' sound card and 'e1000e' NIC when using Q35.


In other words, the recommendations are:

- If there's no user input about machine-type:
-- If guest OS is known, use libosinfo
-- If guest OS is unknown, prefer Q35

- If using Q35:
-- If there's no user input about NIC model:
--- If guest OS is known, use libosinfo
--- If guest OS is unknown, prefer e1000e
-- If there's no user input about sound card model:
--- If guest OS is known, use libosinfo
--- If guest OS is unknown or libosinfo has no sound card recommendation, prefer ich9

We have one additional issue with RHEL-6 guests: they don't have drivers for virtio-1, so they need virtio devices to run in legacy mode.  So I'm extending the scope of this BZ (and related ones) again.

Full list of action items, for reference:
On libosinfo:
* libosinfo should provide a recommendation of machine-type
* libosinfo should provide a recommendation of sound hardware
* libosinfo should tell if the guest OS supports only legacy virtio and/or modern virtio

On management software:
* Management software should use libosinfo to pick default machine-type, and prefer Q35 when possible
* Management software should prefer the 'ich9-hda-intel' sound card and 'e1000e' NIC when using Q35.
* Management software should do what's necessary if guest supports only legacy virtio. Options to ensure that:
** Use "pc" machine type
** Use a libvirt flag that will force the virtio device to be in legacy mode (not available yet, I will open a BZ for this)
** Manually plug the virtio device to a legacy PCI bus (probably not feasible as it would require duplicating the libvirt PCI address allocation logic)

I'm not opening separate BZs for each item above because it's up to the maintainer of each component to decide how to track these tasks.

(In reply to Eduardo Habkost from comment #16)
> We have one additional issue with RHEL-6 guests: they don't have drivers for
> virtio-1, so they need virtio devices to run in legacy mode.  So I'm
> extending the scope of this BZ (and related ones) again.
> 
> Full list of action items, for reference:
> On libosinfo:
> * libosinfo should provide a recommendation of machine-type
> * libosinfo should provide a recommendation of sound hardware
> * libosinfo should tell if the guest OS supports only legacy virtio and/or
> modern virtio
> 
> On management software:
> * Management software should use libosinfo to pick default machine-type, and
> prefer Q35 when possible
> * Management software should prefer the 'ich9-hda-intel' sound card and
> 'e1000e' NIC when using Q35.
> * Management software should do what's necessary if guest supports only
> legacy virtio. Options to ensure that:
> ** Use "pc" machine type
> ** Use a libvirt flag that will force the virtio device to be in legacy mode
> (not available yet, I will open a BZ for this)
> ** Manually plug the virtio device to a legacy PCI bus (probably not
> feasible as it would require duplicating the libvirt PCI address allocation
> logic)
> 
> I'm not opening separate BZs for each item above because it's up to the
> maintainer of each component to decide how to track these tasks.

Meta comment: From what I understand, and from my discussion with Dan Berrangé (refer: https://bugzilla.redhat.com/show_bug.cgi?id=1585798#c2), this item involving libosinfo in comment#16 and your comment#15 should be done as separate work items as part of: https://bugzilla.redhat.com/show_bug.cgi?id=1585798

I am closing this bug as it has not been addressed for a very long time. Please feel free to reopen if it is still relevant.

Closing this as WONTFIX is incorrect; re-opening it.  As 'pc' / 'i440fx' machine type is slated for deprecation, and Nova needs to be ready to handle it.

I don't think this is being tracked anywhere else in Red Hat bugzilla.

There is an upstream bug: 

    https://bugs.launchpad.net/nova/+bug/1780138
    Don't assume the guest machine type to be of 'pc' 

And a WIP Nova specification (started in Jan 2019):

    https://review.opendev.org/#/c/631154/
    WIP: Gracefully handle QEMU machine types for guests

This RFE was not marked as MVP for OSP 17.0 and so will be re targeted for consideration for verification and docs in OSP 17.1. If Tech Preview for OSP 17.0 is required please clone bug and contact rhos-trac with details.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (Release of components for Red Hat OpenStack Platform 17.1 (Wallaby)), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2023:4577