Merge lp:~rconradharris/nova/lp742229 into lp:~hudson-openstack/nova/trunk

[Rick Harris]

disk_format is now an ImageService property. Adds tests to prevent regression.

[Josh Kearney (jk0)]

Big pimpin'.

review: Approve

[Jay Pipes (jaypipes)]

lgtm2

review: Approve

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-03-24 22:32:00 +0000
+++ nova/api/openstack/servers.py	2011-03-25 04:54:23 +0000
@@ -502,33 +502,41 @@
         return dict(actions=actions)
 
     def _get_kernel_ramdisk_from_image(self, req, image_id):
-        """Retrevies kernel and ramdisk IDs from Glance
-
-        Only 'machine' (ami) type use kernel and ramdisk outside of the
-        image.
-        """
-        # FIXME(sirp): Since we're retrieving the kernel_id from an
-        # image_property, this means only Glance is supported.
-        # The BaseImageService needs to expose a consistent way of accessing
-        # kernel_id and ramdisk_id
-        image = self._image_service.show(req.environ['nova.context'], image_id)
-
-        if image['status'] != 'active':
+        """Fetch an image from the ImageService, then if present, return the
+        associated kernel and ramdisk image IDs.
+        """
+        context = req.environ['nova.context']
+        image_meta = self._image_service.show(context, image_id)
+        # NOTE(sirp): extracted to a separate method to aid unit-testing, the
+        # new method doesn't need a request obj or an ImageService stub
+        kernel_id, ramdisk_id = self._do_get_kernel_ramdisk_from_image(
+            image_meta)
+        return kernel_id, ramdisk_id
+
+    @staticmethod
+    def  _do_get_kernel_ramdisk_from_image(image_meta):
+        """Given an ImageService image_meta, return kernel and ramdisk image
+        ids if present.
+
+        This is only valid for `ami` style images.
+        """
+        image_id = image_meta['id']
+        if image_meta['status'] != 'active':
             raise exception.Invalid(
                 _("Cannot build from image %(image_id)s, status not active") %
                   locals())
 
-        if image['disk_format'] != 'ami':
+        if image_meta['properties']['disk_format'] != 'ami':
             return None, None
 
         try:
-            kernel_id = image['properties']['kernel_id']
+            kernel_id = image_meta['properties']['kernel_id']
         except KeyError:
             raise exception.NotFound(
                 _("Kernel not found for image %(image_id)s") % locals())
 
         try:
-            ramdisk_id = image['properties']['ramdisk_id']
+            ramdisk_id = image_meta['properties']['ramdisk_id']
         except KeyError:
             raise exception.NotFound(
                 _("Ramdisk not found for image %(image_id)s") % locals())
 
=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-03-24 20:31:04 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-03-25 04:54:23 +0000
@@ -26,6 +26,7 @@
 
 from nova import context
 from nova import db
+from nova import exception
 from nova import flags
 from nova import test
 import nova.api.openstack
@@ -1260,3 +1261,57 @@
         server = dom.childNodes[0]
         self.assertEquals(server.nodeName, 'server')
         self.assertTrue(server.getAttribute('adminPass').startswith('fake'))
+
+
+class TestGetKernelRamdiskFromImage(test.TestCase):
+    """
+    If we're building from an AMI-style image, we need to be able to fetch the
+    kernel and ramdisk associated with the machine image. This information is
+    stored with the image metadata and return via the ImageService.
+
+    These tests ensure that we parse the metadata return the ImageService
+    correctly and that we handle failure modes appropriately.
+    """
+
+    def test_status_not_active(self):
+        """We should only allow fetching of kernel and ramdisk information if
+        we have a 'fully-formed' image, aka 'active'
+        """
+        image_meta = {'id': 1, 'status': 'queued'}
+        self.assertRaises(exception.Invalid, self._get_k_r, image_meta)
+
+    def test_not_ami(self):
+        """Anything other than ami should return no kernel and no ramdisk"""
+        image_meta = {'id': 1, 'status': 'active',
+                      'properties': {'disk_format': 'vhd'}}
+        kernel_id, ramdisk_id = self._get_k_r(image_meta)
+        self.assertEqual(kernel_id, None)
+        self.assertEqual(ramdisk_id, None)
+
+    def test_ami_no_kernel(self):
+        """If an ami is missing a kernel it should raise NotFound"""
+        image_meta = {'id': 1, 'status': 'active',
+                      'properties': {'disk_format': 'ami', 'ramdisk_id': 1}}
+        self.assertRaises(exception.NotFound, self._get_k_r, image_meta)
+
+    def test_ami_no_ramdisk(self):
+        """If an ami is missing a ramdisk it should raise NotFound"""
+        image_meta = {'id': 1, 'status': 'active',
+                      'properties': {'disk_format': 'ami', 'kernel_id': 1}}
+        self.assertRaises(exception.NotFound, self._get_k_r, image_meta)
+
+    def test_ami_kernel_ramdisk_present(self):
+        """Return IDs if both kernel and ramdisk are present"""
+        image_meta = {'id': 1, 'status': 'active',
+                      'properties': {'disk_format': 'ami', 'kernel_id': 1,
+                                     'ramdisk_id': 2}}
+        kernel_id, ramdisk_id = self._get_k_r(image_meta)
+        self.assertEqual(kernel_id, 1)
+        self.assertEqual(ramdisk_id, 2)
+
+    @staticmethod
+    def _get_k_r(image_meta):
+        """Rebinding function to a shorter name for convenience"""
+        kernel_id, ramdisk_id = \
+            servers.Controller._do_get_kernel_ramdisk_from_image(image_meta)
+        return kernel_id, ramdisk_id