Merge lp:~jk0/nova/lp824034 into lp:~hudson-openstack/nova/trunk

[Josh Kearney]

Validate the size of VHD files in OVF containers.

[Dan Prince (dan-prince)]

Hi Josh,

The new rev of the v1.1 OSAPI has image attributes for minDisk and minRam. Is it possible we might be able to use these to predetermine if an image/instance-type combination is invalid? This would probably require an update to Glance as well but I thought it worth mentioning.

--

Shouldn't 'physical_utilisation' be 'physical_utilization'?

--

Lastly, I get a conflict when merging trunk:

nova/virt/xenapi/vm_utils.py

review: Needs Fixing

[Josh Kearney (jk0)]

> The new rev of the v1.1 OSAPI has image attributes for minDisk and minRam. Is
> it possible we might be able to use these to predetermine if an image
> /instance-type combination is invalid? This would probably require an update
> to Glance as well but I thought it worth mentioning.

To the best of my knowledge, those are used in cases where there is a minimum amount of RAM/disk space required to boot an instance (for example, Windows requires at least 512MB of RAM) -- so a Windows image might have a minRam of 512 and minDisk of 10GB, but that doesn't imply the size of the image is actually 10GB.

> Shouldn't 'physical_utilisation' be 'physical_utilization'?

I believe this was written by Citrix employees in the UK, so it's actually 'physical_utilisation'.

> Lastly, I get a conflict when merging trunk:
>
> nova/virt/xenapi/vm_utils.py

Thanks, I'll merge trunk and re-push.

[lp:~jk0/nova/lp824034]

1425. By Josh Kearney on 2011-08-15

Merged trunk.

1426. By Josh Kearney on 2011-08-15

Fixed merge conflict.

1427. By Josh Kearney on 2011-08-15

Merged trunk.

1428. By Josh Kearney on 2011-08-15

Corrected names in TODO/FIXME.

[Rick Harris (rconradharris)]

LGTM

review: Approve

[Matt Dietz (cerberus)]

Is bueno

review: Approve

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-08-09 09:54:51 +0000
+++ nova/compute/manager.py	2011-08-15 18:37:26 +0000
@@ -321,10 +321,63 @@
 
     def _run_instance(self, context, instance_id, **kwargs):
         """Launch a new instance with specified options."""
+        def _check_image_size():
+            """Ensure image is smaller than the maximum size allowed by the
+            instance_type.
+
+            The image stored in Glance is potentially compressed, so we use two
+            checks to ensure that the size isn't exceeded:
+
+                1) This one - checks compressed size, this a quick check to
+                   eliminate any images which are obviously too large
+
+                2) Check uncompressed size in nova.virt.xenapi.vm_utils. This
+                   is a slower check since it requires uncompressing the entire
+                   image, but is accurate because it reflects the image's
+                   actual size.
+            """
+            # NOTE(jk0): image_ref is defined in the DB model, image_href is
+            # used by the image service. This should be refactored to be
+            # consistent.
+            image_href = instance['image_ref']
+            image_service, image_id = nova.image.get_image_service(image_href)
+            image_meta = image_service.show(context, image_id)
+
+            try:
+                size_bytes = image_meta['size']
+            except KeyError:
+                # Size is not a required field in the image service (yet), so
+                # we are unable to rely on it being there even though it's in
+                # glance.
+
+                # TODO(jk0): Should size be required in the image service?
+                return
+
+            instance_type_id = instance['instance_type_id']
+            instance_type = self.db.instance_type_get(context,
+                    instance_type_id)
+            allowed_size_gb = instance_type['local_gb']
+            allowed_size_bytes = allowed_size_gb * 1024 * 1024 * 1024
+
+            LOG.debug(_("image_id=%(image_id)d, image_size_bytes="
+                        "%(size_bytes)d, allowed_size_bytes="
+                        "%(allowed_size_bytes)d") % locals())
+
+            if size_bytes > allowed_size_bytes:
+                LOG.info(_("Image '%(image_id)d' size %(size_bytes)d exceeded"
+                           " instance_type allowed size "
+                           "%(allowed_size_bytes)d")
+                           % locals())
+                raise exception.ImageTooLarge()
+
         context = context.elevated()
         instance = self.db.instance_get(context, instance_id)
+
         if instance['name'] in self.driver.list_instances():
             raise exception.Error(_("Instance has already been created"))
+
+        _check_image_size()
+
         LOG.audit(_("instance %s: starting..."), instance_id,
                   context=context)
         updates = {}

=== modified file 'nova/exception.py'
--- nova/exception.py	2011-08-09 23:59:51 +0000
+++ nova/exception.py	2011-08-15 18:37:26 +0000
@@ -721,3 +721,7 @@
 
 class CannotResizeToSmallerSize(NovaException):
     message = _("Resizing to a smaller size is not supported.")
+
+
+class ImageTooLarge(NovaException):
+    message = _("Image is larger than instance type allows")

=== modified file 'nova/tests/scheduler/test_scheduler.py'
--- nova/tests/scheduler/test_scheduler.py	2011-08-11 23:26:26 +0000
+++ nova/tests/scheduler/test_scheduler.py	2011-08-15 18:37:26 +0000
@@ -257,7 +257,9 @@
     def _create_instance(self, **kwargs):
         """Create a test instance"""
         inst = {}
-        inst['image_id'] = 1
+        # NOTE(jk0): If an integer is passed as the image_ref, the image
+        # service will use the default image service (in this case, the fake).
+        inst['image_ref'] = '1'
         inst['reservation_id'] = 'r-fakeres'
         inst['user_id'] = self.user_id
         inst['project_id'] = self.project_id

=== modified file 'nova/tests/xenapi/stubs.py'
--- nova/tests/xenapi/stubs.py	2011-08-09 22:46:57 +0000
+++ nova/tests/xenapi/stubs.py	2011-08-15 18:37:26 +0000
@@ -28,10 +28,10 @@
 
 def stubout_instance_snapshot(stubs):
     @classmethod
-    def fake_fetch_image(cls, context, session, instance_id, image, user,
+    def fake_fetch_image(cls, context, session, instance, image, user,
                          project, type):
         from nova.virt.xenapi.fake import create_vdi
-        name_label = "instance-%s" % instance_id
+        name_label = "instance-%s" % instance.id
         #TODO: create fake SR record
         sr_ref = "fakesr"
         vdi_ref = create_vdi(name_label=name_label, read_only=False,

=== modified file 'nova/virt/xenapi/fake.py'
--- nova/virt/xenapi/fake.py	2011-06-28 11:33:12 +0000
+++ nova/virt/xenapi/fake.py	2011-08-15 18:37:26 +0000
@@ -140,6 +140,7 @@
                            'location': '',
                            'xenstore_data': '',
                            'sm_config': {},
+                           'physical_utilisation': '123',
                            'VBDs': {}})
 
 

=== modified file 'nova/virt/xenapi/vm_utils.py'
--- nova/virt/xenapi/vm_utils.py	2011-08-14 04:25:46 +0000
+++ nova/virt/xenapi/vm_utils.py	2011-08-15 18:37:26 +0000
@@ -31,6 +31,7 @@
 from xml.dom import minidom
 
 import glance.client
+from nova import db
 from nova import exception
 from nova import flags
 import nova.image
@@ -413,7 +414,7 @@
         return vdi_ref
 
     @classmethod
-    def fetch_image(cls, context, session, instance_id, image, user_id,
+    def fetch_image(cls, context, session, instance, image, user_id,
                     project_id, image_type):
         """Fetch image from glance based on image type.
 
@@ -422,18 +423,19 @@
         """
         if image_type == ImageType.DISK_VHD:
             return cls._fetch_image_glance_vhd(context,
-                session, instance_id, image, image_type)
+                session, instance, image, image_type)
         else:
             return cls._fetch_image_glance_disk(context,
-                session, instance_id, image, image_type)
+                session, instance, image, image_type)
 
     @classmethod
-    def _fetch_image_glance_vhd(cls, context, session, instance_id, image,
+    def _fetch_image_glance_vhd(cls, context, session, instance, image,
                                 image_type):
         """Tell glance to download an image and put the VHDs into the SR
 
         Returns: A list of dictionaries that describe VDIs
         """
+        instance_id = instance.id
         LOG.debug(_("Asking xapi to fetch vhd image %(image)s")
                     % locals())
         sr_ref = safe_find_sr(session)
@@ -467,17 +469,58 @@
 
         cls.scan_sr(session, instance_id, sr_ref)
 
-        # Pull out the UUID of the first VDI
-        vdi_uuid = vdis[0]['vdi_uuid']
+        # Pull out the UUID of the first VDI (which is the os VDI)
+        os_vdi_uuid = vdis[0]['vdi_uuid']
+
         # Set the name-label to ease debugging
-        vdi_ref = session.get_xenapi().VDI.get_by_uuid(vdi_uuid)
+        vdi_ref = session.get_xenapi().VDI.get_by_uuid(os_vdi_uuid)
         primary_name_label = get_name_label_for_image(image)
         session.get_xenapi().VDI.set_name_label(vdi_ref, primary_name_label)
 
+        cls._check_vdi_size(context, session, instance, os_vdi_uuid)
         return vdis
 
     @classmethod
-    def _fetch_image_glance_disk(cls, context, session, instance_id, image,
+    def _get_vdi_chain_size(cls, context, session, vdi_uuid):
+        """Compute the total size of a VDI chain, starting with the specified
+        VDI UUID.
+
+        This will walk the VDI chain to the root, add the size of each VDI into
+        the total.
+        """
+        size_bytes = 0
+        for vdi_rec in walk_vdi_chain(session, vdi_uuid):
+            cur_vdi_uuid = vdi_rec['uuid']
+            vdi_size_bytes = int(vdi_rec['physical_utilisation'])
+            LOG.debug(_('vdi_uuid=%(cur_vdi_uuid)s vdi_size_bytes='
+                        '%(vdi_size_bytes)d' % locals()))
+            size_bytes += vdi_size_bytes
+        return size_bytes
+
+    @classmethod
+    def _check_vdi_size(cls, context, session, instance, vdi_uuid):
+        size_bytes = cls._get_vdi_chain_size(context, session, vdi_uuid)
+
+        # FIXME(jk0): this was copied directly from compute.manager.py, let's
+        # refactor this to a common area
+        instance_type_id = instance['instance_type_id']
+        instance_type = db.instance_type_get(context,
+                instance_type_id)
+        allowed_size_gb = instance_type['local_gb']
+        allowed_size_bytes = allowed_size_gb * 1024 * 1024 * 1024
+
+        LOG.debug(_("image_size_bytes=%(size_bytes)d, allowed_size_bytes="
+                    "%(allowed_size_bytes)d") % locals())
+
+        if size_bytes > allowed_size_bytes:
+            LOG.info(_("Image size %(size_bytes)d exceeded"
+                       " instance_type allowed size "
+                       "%(allowed_size_bytes)d")
+                       % locals())
+            raise exception.ImageTooLarge()
+
+    @classmethod
+    def _fetch_image_glance_disk(cls, context, session, instance, image,
                                  image_type):
         """Fetch the image from Glance
 
@@ -489,6 +532,7 @@
         Returns: A single filename if image_type is KERNEL_RAMDISK
                  A list of dictionaries that describe VDIs, otherwise
         """
+        instance_id = instance.id
         # FIXME(sirp): Since the Glance plugin seems to be required for the
         # VHD disk, it may be worth using the plugin for both VHD and RAW and
         # DISK restores
@@ -807,6 +851,21 @@
         return None
 
 
+def walk_vdi_chain(session, vdi_uuid):
+    """Yield vdi_recs for each element in a VDI chain"""
+    # TODO(jk0): perhaps make get_vhd_parent use this
+    while True:
+        vdi_ref = session.get_xenapi().VDI.get_by_uuid(vdi_uuid)
+        vdi_rec = session.get_xenapi().VDI.get_record(vdi_ref)
+        yield vdi_rec
+
+        parent_uuid = vdi_rec['sm_config'].get('vhd-parent')
+        if parent_uuid:
+            vdi_uuid = parent_uuid
+        else:
+            break
+
+
 def wait_for_vhd_coalesce(session, instance_id, sr_ref, vdi_ref,
                           original_parent_uuid):
     """ Spin until the parent VHD is coalesced into its parent VHD

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-08-14 04:25:46 +0000
+++ nova/virt/xenapi/vmops.py	2011-08-15 18:37:26 +0000
@@ -137,7 +137,7 @@
     def _create_disks(self, context, instance):
         disk_image_type = VMHelper.determine_disk_image_type(instance)
         vdis = VMHelper.fetch_image(context, self._session,
-                instance.id, instance.image_ref,
+                instance, instance.image_ref,
                 instance.user_id, instance.project_id,
                 disk_image_type)
         return vdis
@@ -182,11 +182,11 @@
         try:
             if instance.kernel_id:
                 kernel = VMHelper.fetch_image(context, self._session,
-                        instance.id, instance.kernel_id, instance.user_id,
+                        instance, instance.kernel_id, instance.user_id,
                         instance.project_id, ImageType.KERNEL)[0]
             if instance.ramdisk_id:
                 ramdisk = VMHelper.fetch_image(context, self._session,
-                        instance.id, instance.ramdisk_id, instance.user_id,
+                        instance, instance.ramdisk_id, instance.user_id,
                         instance.project_id, ImageType.RAMDISK)[0]
             # Create the VM ref and attach the first disk
             first_vdi_ref = self._session.call_xenapi('VDI.get_by_uuid',