On Mon, Jul 15, 2013 at 02:54:58PM +0200, Martin Kletzander wrote:
> On 07/15/2013 02:24 PM, Daniel P. Berrange wrote:
> > On Mon, Jul 15, 2013 at 09:50:45AM +0200, Martin Kletzander wrote:
> >> When user does not specify any model for scsi controller, or worse, no
> >> controller at all, but libvirt automatically adds scsi controller with
> >> no model, we are not searching for virtio-scsi and thus this can fail
> >> for example on qemu which doesn't support lsi logic adapter.
> >>
> >> This means that when qemu on x86 doesn't support lsi53c895a and the
> >> user adds the following to an XML without any scsi controller:
> >>
> >> <disk ...>
> >> ...
> >> <target dev='sda'>
> >> </disk>
> >>
> >> libvirt fails like this:
> >> # virsh define asdf.xml
> >> error: Failed to define domain from asdf.xml
> >> error: internal error Unable to determine model for scsi controller
> >>
> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=974943
> >> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> >> ---
> >> src/qemu/qemu_command.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >> index 0e517f2..aff1768 100644
> >> --- a/src/qemu/qemu_command.c
> >> +++ b/src/qemu/qemu_command.c
> >> @@ -703,6 +703,8 @@ qemuSetScsiControllerModel(virDomainDefPtr def,
> >> *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
> >> } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) {
> >> *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
> >> + } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) {
> >> + *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
> >> } else {
> >> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> _("Unable to determine model for scsi controller"));
> >
> > I'm not really convinced this is a good idea.
> >
> > The code as it stands is saying on PPC uses IBMVSCSI, otherwise
> > uses LSI SCSI. This is reasonable behaviour for an application to
> > rely on since it is predictable from the outside - ie the app knows
> > what architecture / machine type the VM is, so it knows exactly what
> > SCSI controller will be chosen by libvirt & will get an error if not
> > available.
> >
> > You're adding in a layer of unpredictability there, because now there
> > are multiple choices of SCSI controller for all architectures.
> > Furthermore, there is no parity of guest OS support between these
> > different SCSI controllers. Thus if an application has a guest that
> > supports LSI Logic SCSI, it is not reasonable to assume that falling
> > back to VirtIO SCSI is going to be helpful behaviour.
> >
>
> Since this information is visible from the code only, I don't think we
> need to guarantee that. If an application wants a SCSI controller of a
> particular type, it should specify that. Mgmt application IMHO can't
> rely on the fact that due to historical reasons there is controller
> model left as a default. We wouldn't be able to change anything then
> and whenever would QEMU decided to drop the model (like now with all
> "lsi*" controllers), we wouldn't be able to do anything automatically.
>
> The main problem now is that if only disk gets inserted into the XML, we
> will automatically add invalid controller (without any model), so the
> automatic addition of controllers is now broken due to this reason as well.
>
> > IMHO it is better to keep an error as we have now, and require that an
> > application specify an explicit model name in the XML for anything
> > else.
> >
>
> Can we at least fallback to the 'virtio-scsi' model for controllers that
> were added automatically as a requisite of another device?
>
> > We could improve the error message here though, to something like
> >
> > "LSI Logic SCSI controller not available on this host"
> >
>
> OK, at least a message like "No valid default SCSI controller available
> on host" (so it is sensible even on ppc64) would help a bit with other
> cases.
Hmm, I had forgotten that we auto-added controllers as a side
effect of adding disks. If we were doing hotplug today, we
would not do the auto-add of controllers, but we have no choice
due to back compat issues.
So I guess, reluctantly, we have to do what you suggest in your
patch to cope with distros disabling other SCSI controllers.
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|