[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Add virtio-scsi to fallback models of scsi controller



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 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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]