Merge lp:~tamura-yoshiaki/nova/bug-805312 into lp:~hudson-openstack/nova/trunk

[Yoshiaki Tamura]

Fix boot from volume failure for network block devices.

This patch looks up the device_path and swithes between 'block' and 'network' when creating libvirt.xml.

[Devin Carlen (devcamcar)]

This looks good but it looks like there is some logic is being duplicated:

35 + if vol['device_path'].startswith('/dev/'):
36 + vol['type'] = 'block'
37 + elif ':' in vol['device_path']:
38 + (protocol, name) = vol['device_path'].split(':')
39 + vol['type'] = 'network'
40 + vol['protocol'] = protocol
41 + vol['device_path'] = name
42 + else:
43 + raise exception.InvalidDevicePath(path=vol['device_path'])

There is very similar logic in the attach_volume method. I recommend fixing this so that there is no repetition. Maybe something like:

def _get_volume_device_type(self, device_path):
    if device_path.startswith('/dev/'):
        return 'block'
    elif ':' in device_path:
        return 'network'
    else:
        raise exception.InvalidDevicePath(path=device_path)

And then re-use that.

review: Needs Fixing

[Yoshiaki Tamura (tamura-yoshiaki)]

> This looks good but it looks like there is some logic is being duplicated:
>
> 35 + if vol['device_path'].startswith('/dev/'):
> 36 + vol['type'] = 'block'
> 37 + elif ':' in vol['device_path']:
> 38 + (protocol, name) = vol['device_path'].split(':')
> 39 + vol['type'] = 'network'
> 40 + vol['protocol'] = protocol
> 41 + vol['device_path'] = name
> 42 + else:
> 43 + raise
> exception.InvalidDevicePath(path=vol['device_path'])
>
> There is very similar logic in the attach_volume method. I recommend fixing
> this so that there is no repetition. Maybe something like:
>
> def _get_volume_device_type(self, device_path):
> if device_path.startswith('/dev/'):
> return 'block'
> elif ':' in device_path:
> return 'network'
> else:
> raise exception.InvalidDevicePath(path=device_path)
>
> And then re-use that.

Sorry for my slow response. Applied your comments.

[Devin Carlen (devcamcar)]

great, thanks!

review: Approve

[Devin Carlen (devcamcar)]

Don't forget to set it to "Ready for review" again.

[Christopher MacGown (0x44)]

review: Approve

=== modified file 'nova/virt/libvirt.xml.template'
--- nova/virt/libvirt.xml.template	2011-06-24 12:01:51 +0000
+++ nova/virt/libvirt.xml.template	2011-07-20 07:23:37 +0000
@@ -82,9 +82,13 @@
             </disk>
         #end if
         #for $vol in $volumes
-            <disk type='block'>
+            <disk type='${vol.type}'>
                 <driver type='raw'/>
+                #if $vol.type == 'network'
+                <source protocol='${vol.protocol}' name='${vol.name}'/>
+                #else
                 <source dev='${vol.device_path}'/>
+                #end if
                 <target dev='${vol.mount_device}' bus='${disk_bus}'/>
             </disk>
         #end for

=== modified file 'nova/virt/libvirt/connection.py'
--- nova/virt/libvirt/connection.py	2011-07-13 18:19:32 +0000
+++ nova/virt/libvirt/connection.py	2011-07-20 07:23:37 +0000
@@ -335,21 +335,20 @@
     def attach_volume(self, instance_name, device_path, mountpoint):
         virt_dom = self._lookup_by_name(instance_name)
         mount_device = mountpoint.rpartition("/")[2]
-        if device_path.startswith('/dev/'):
+        (type, protocol, name) = \
+            self._get_volume_device_info(vol['device_path'])
+        if type == 'block':
             xml = """<disk type='block'>
                          <driver name='qemu' type='raw'/>
                          <source dev='%s'/>
                          <target dev='%s' bus='virtio'/>
                      </disk>""" % (device_path, mount_device)
-        elif ':' in device_path:
-            (protocol, name) = device_path.split(':')
+        elif type == 'network':
             xml = """<disk type='network'>
                          <driver name='qemu' type='raw'/>
                          <source protocol='%s' name='%s'/>
                          <target dev='%s' bus='virtio'/>
-                     </disk>""" % (protocol,
-                                   name,
-                                   mount_device)
+                     </disk>""" % (protocol, name, mount_device)
         else:
             raise exception.InvalidDevicePath(path=device_path)
 
@@ -971,6 +970,16 @@
                 return True
         return False
 
+    @exception.wrap_exception
+    def _get_volume_device_info(self, device_path):
+        if device_path.startswith('/dev/'):
+            return ('block', None, None)
+        elif ':' in device_path:
+            (protocol, name) = device_path.split(':')
+            return ('network', protocol, name)
+        else:
+            raise exception.InvalidDevicePath(path=device_path)
+
     def _prepare_xml_info(self, instance, rescue=False, network_info=None,
                           block_device_mapping=None):
         block_device_mapping = block_device_mapping or []
@@ -993,6 +1002,9 @@
 
         for vol in block_device_mapping:
             vol['mount_device'] = _strip_dev(vol['mount_device'])
+            (vol['type'], vol['protocol'], vol['name']) = \
+                self._get_volume_device_info(vol['device_path'])
+
         ebs_root = self._volume_in_mapping(self.root_mount_device,
                                            block_device_mapping)
         if self._volume_in_mapping(self.local_mount_device,