Fixes bug 890206

Add a check when creating the volume definition for the libvirt manager
to use the apporpiate driver for the disk, according to [1].

[1] http://libvirt.org/formatdomain.html#elementsDisks

Change-Id: I89cf3bb308cff28194de0beb36d69f0e32224cbe

4 files changed, 63 insertions, 8 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 0f3e0ae3d..aa2d08c64 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1522,7 +1522,7 @@ class API(base.Base):
 
     def attach_volume(self, context, instance, volume_id, device):
         """Attach an existing volume to an existing instance."""
-        if not re.match("^/dev/[a-z]d[a-z]+$", device):
+        if not re.match("^/dev/x{0,1}[a-z]d[a-z]+$", device):
             raise exception.ApiError(_("Invalid device specified: %s. "
                                      "Example device: /dev/vdb") % device)
         self.volume_api.check_attach(context, volume_id=volume_id)
diff --git a/nova/tests/test_compute.py b/nova/tests/test_compute.py
index 6b74a14f0..543f6823c 100644
--- a/nova/tests/test_compute.py
+++ b/nova/tests/test_compute.py
@@ -1995,6 +1995,47 @@ class ComputeAPITestCase(BaseTestCase):
         self.compute_api.add_fixed_ip(self.context, instance, '1')
         self.compute_api.remove_fixed_ip(self.context, instance, '192.168.1.1')
 
+    def test_attach_volume_invalid(self):
+        self.assertRaises(exception.ApiError,
+                self.compute_api.attach_volume,
+                None,
+                None,
+                None,
+                '/dev/invalid')
+
+    def test_attach_volume(self):
+        instance_id = 1
+        volume_id = 1
+
+        for device in ('/dev/sda', '/dev/xvda'):
+            # creating mocks
+            self.mox.StubOutWithMock(self.compute_api.volume_api,
+                    'check_attach')
+            self.mox.StubOutWithMock(self.compute_api, 'get')
+            self.mox.StubOutWithMock(rpc, 'cast')
+
+            rpc.cast(
+                    mox.IgnoreArg(),
+                    mox.IgnoreArg(), {"method": "attach_volume",
+                        "args": {'volume_id': volume_id,
+                                 'instance_id': instance_id,
+                                 'mountpoint': device}})
+
+            self.compute_api.volume_api.check_attach(
+                    mox.IgnoreArg(),
+                    volume_id=volume_id).AndReturn(
+                            {'id': volume_id, 'status': 'available',
+                                'attach_status': 'detached'})
+
+            self.compute_api.get(
+                    mox.IgnoreArg(),
+                    mox.IgnoreArg()).AndReturn({'id': instance_id,
+                        'host': 'fake'})
+
+            self.mox.ReplayAll()
+            self.compute_api.attach_volume(None, None, volume_id, device)
+            self.mox.UnsetStubs()
+
     def test_vnc_console(self):
         """Make sure we can a vnc console for an instance."""
         def vnc_rpc_call_wrapper(*args, **kwargs):
diff --git a/nova/tests/test_libvirt.py b/nova/tests/test_libvirt.py
index 6420aa01c..61ba21692 100644
--- a/nova/tests/test_libvirt.py
+++ b/nova/tests/test_libvirt.py
@@ -109,11 +109,19 @@ class LibvirtVolumeTestCase(test.TestCase):
         super(LibvirtVolumeTestCase, self).setUp()
         self.stubs.Set(utils, 'execute', self.fake_execute)
 
+        class FakeLibvirtConnection(object):
+            def __init__(self, hyperv="QEMU"):
+                self.hyperv = hyperv
+
+            def get_hypervisor_type(self):
+                return self.hyperv
+        self.fake_conn = FakeLibvirtConnection("Xen")
+
     def test_libvirt_iscsi_driver(self):
         # NOTE(vish) exists is to make driver assume connecting worked
         self.stubs.Set(os.path, 'exists', lambda x: True)
         vol_driver = volume_driver.ISCSIDriver()
-        libvirt_driver = volume.LibvirtISCSIVolumeDriver('fake')
+        libvirt_driver = volume.LibvirtISCSIVolumeDriver(self.fake_conn)
         name = 'volume-00000001'
         vol = {'id': 1,
                'name': name,
@@ -133,7 +141,7 @@ class LibvirtVolumeTestCase(test.TestCase):
 
     def test_libvirt_sheepdog_driver(self):
         vol_driver = volume_driver.SheepdogDriver()
-        libvirt_driver = volume.LibvirtNetVolumeDriver('fake')
+        libvirt_driver = volume.LibvirtNetVolumeDriver(self.fake_conn)
         name = 'volume-00000001'
         vol = {'id': 1, 'name': name}
         address = '127.0.0.1'
@@ -148,7 +156,7 @@ class LibvirtVolumeTestCase(test.TestCase):
 
     def test_libvirt_rbd_driver(self):
         vol_driver = volume_driver.RBDDriver()
-        libvirt_driver = volume.LibvirtNetVolumeDriver('fake')
+        libvirt_driver = volume.LibvirtNetVolumeDriver(self.fake_conn)
         name = 'volume-00000001'
         vol = {'id': 1, 'name': name}
         address = '127.0.0.1'
diff --git a/nova/virt/libvirt/volume.py b/nova/virt/libvirt/volume.py
index caa1ec48a..a890dadad 100644
--- a/nova/virt/libvirt/volume.py
+++ b/nova/virt/libvirt/volume.py
@@ -35,14 +35,19 @@ class LibvirtVolumeDriver(object):
     def __init__(self, connection):
         self.connection = connection
 
+    def _pick_volume_driver(self):
+        hypervisor_type = self.connection.get_hypervisor_type().lower()
+        return "phy" if hypervisor_type == "xen" else "qemu"
+
     def connect_volume(self, connection_info, mount_device):
         """Connect the volume. Returns xml for libvirt."""
+        driver = self._pick_volume_driver()
         device_path = connection_info['data']['device_path']
         xml = """<disk type='block'>
-                     <driver name='qemu' type='raw'/>
+                     <driver name='%s' type='raw'/>
                      <source dev='%s'/>
                      <target dev='%s' bus='virtio'/>
-                 </disk>""" % (device_path, mount_device)
+                 </disk>""" % (driver, device_path, mount_device)
         return xml
 
     def disconnect_volume(self, connection_info, mount_device):
@@ -54,13 +59,14 @@ class LibvirtNetVolumeDriver(LibvirtVolumeDriver):
     """Driver to attach Network volumes to libvirt."""
 
     def connect_volume(self, connection_info, mount_device):
+        driver = self._pick_volume_driver()
         protocol = connection_info['driver_volume_type']
         name = connection_info['data']['name']
         xml = """<disk type='network'>
-                     <driver name='qemu' type='raw'/>
+                     <driver name='%s' type='raw'/>
                      <source protocol='%s' name='%s'/>
                      <target dev='%s' bus='virtio'/>
-                 </disk>""" % (protocol, name, mount_device)
+                 </disk>""" % (driver, protocol, name, mount_device)
         return xml