Merge lp:~billy-olsen/nova/kilo+lp1457517 into lp:~ubuntu-server-dev/nova/kilo

[Billy Olsen]

This is a resubmission of Liang's branch for including the upstream patch for not considering a flavor's volume size when booting from volume. His original branch is located at lp:~cbjchen/nova/kilo-sru-lp1457517 but he's currently unavailable.

=== modified file 'debian/changelog'
--- debian/changelog	2015-08-18 21:07:07 +0000
+++ debian/changelog	2015-09-08 23:22:49 +0000
@@ -1,7 +1,12 @@
 nova (1:2015.1.1-0ubuntu2) UNRELEASED; urgency=medium
 
+  [ Corey Bryant ]
   * d/rules: Prevent dh_python2 from guessing dependencies.
 
+  [ Liang Chen ]
+  * d/p/not-check-disk-size.patch: Fix booting from volume error
+    when flavor disk too small (LP: #1457517)
+
  -- Corey Bryant <[e-mail]>  Thu, 13 Aug 2015 15:13:43 -0400
 
 nova (1:2015.1.1-0ubuntu1) vivid; urgency=medium

=== added file 'debian/patches/not-check-disk-size.patch'
--- debian/patches/not-check-disk-size.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/not-check-disk-size.patch	2015-09-08 23:22:49 +0000
@@ -0,0 +1,494 @@
+From 8794b938dcb983b7c918718807c2396cb255b4ce Mon Sep 17 00:00:00 2001
+From: Matthew Booth <[e-mail]>
+Date: Wed, 22 Jul 2015 14:56:52 +0100
+Subject: [PATCH 1/1] Don't check flavor disk size when booting from volume
+
+When creating a volume from an image, cinder copies the image metadata
+into volume properties. When booting from the volume, we read this
+metadata from the volume and use it as image metadata once again.
+
+While fixing the check against min_ram,
+change I861a78b5c7efa71e4bf7206d388b8d0d8048c78e introduced a
+regression which prevents a user from booting a volume which is larger
+than the flavor's disk. As we are not creating this disk, this check
+does not make sense. Similarly, it checks the image metadata's
+min_disk against the flavor disk size, which is not being used.
+
+This change leaves the image metadata check unaltered when creating a
+flavor disk. When booting from a volume, we check min_disk from image
+metadata against the actual size of the volume. We don't check the
+volume size at all. The check against min_ram is retained unaltered.
+
+Closes-Bug: #1457517
+Closes-Bug: #1459491
+Closes-Bug: #1466305
+Change-Id: I264493172da20b664df571e32876030246c2a87c
+(cherry picked from commit 642c986f0636d52a9ba279c87e25082b4aa9b3b8)
+---
+ nova/compute/api.py                           | 101 +++++++++++++++-----
+ nova/tests/unit/compute/test_compute.py       | 127 +++++++++++++++++++++++---
+ nova/tests/unit/compute/test_compute_api.py   |  11 ++-
+ nova/tests/unit/compute/test_compute_cells.py |   8 +-
+ 4 files changed, 203 insertions(+), 44 deletions(-)
+
+diff --git a/nova/compute/api.py b/nova/compute/api.py
+index 6634918..5937895 100644
+--- a/nova/compute/api.py
++++ b/nova/compute/api.py
+@@ -651,7 +651,8 @@ class API(base.Base):
+         # reason, we rely on the DB to cast True to a String.
+         return True if bool_val else ''
+ 
+-    def _check_requested_image(self, context, image_id, image, instance_type):
++    def _check_requested_image(self, context, image_id, image,
++                               instance_type, root_bdm):
+         if not image:
+             return
+ 
+@@ -668,15 +669,63 @@ class API(base.Base):
+         if instance_type['memory_mb'] < int(image.get('min_ram') or 0):
+             raise exception.FlavorMemoryTooSmall()
+ 
+-        # NOTE(johannes): root_gb is allowed to be 0 for legacy reasons
+-        # since libvirt interpreted the value differently than other
+-        # drivers. A value of 0 means don't check size.
+-        root_gb = instance_type['root_gb']
+-        if root_gb:
+-            if int(image.get('size') or 0) > root_gb * (1024 ** 3):
+-                raise exception.FlavorDiskTooSmall()
++        # Image min_disk is in gb, size is in bytes. For sanity, have them both
++        # in bytes.
++        image_min_disk = int(image.get('min_disk') or 0) * units.Gi
++        image_size = int(image.get('size') or 0)
++
++        # Target disk is a volume. Don't check flavor disk size because it
++        # doesn't make sense, and check min_disk against the volume size.
++        if (root_bdm is not None and root_bdm.is_volume):
++            # There are 2 possibilities here: either the target volume already
++            # exists, or it doesn't, in which case the bdm will contain the
++            # intended volume size.
++            #
++            # Cinder does its own check against min_disk, so if the target
++            # volume already exists this has already been done and we don't
++            # need to check it again here. In this case, volume_size may not be
++            # set on the bdm.
++            #
++            # If we're going to create the volume, the bdm will contain
++            # volume_size. Therefore we should check it if it exists. This will
++            # still be checked again by cinder when the volume is created, but
++            # that will not happen until the request reaches a host. By
++            # checking it here, the user gets an immediate and useful failure
++            # indication.
++            #
++            # The third possibility is that we have failed to consider
++            # something, and there are actually more than 2 possibilities. In
++            # this case cinder will still do the check at volume creation time.
++            # The behaviour will still be correct, but the user will not get an
++            # immediate failure from the api, and will instead have to
++            # determine why the instance is in an error state with a task of
++            # block_device_mapping.
++            #
++            # We could reasonably refactor this check into _validate_bdm at
++            # some future date, as the various size logic is already split out
++            # in there.
++            dest_size = root_bdm.volume_size
++            if dest_size is not None:
++                dest_size *= units.Gi
++
++                if image_min_disk > dest_size:
++                    # TODO(mdbooth) Raise a more descriptive exception here.
++                    # This is the exception which calling code expects, but
++                    # it's potentially misleading to the user.
++                    raise exception.FlavorDiskTooSmall()
++
++        # Target disk is a local disk whose size is taken from the flavor
++        else:
++            dest_size = instance_type['root_gb'] * units.Gi
++
++            # NOTE(johannes): root_gb is allowed to be 0 for legacy reasons
++            # since libvirt interpreted the value differently than other
++            # drivers. A value of 0 means don't check size.
++            if dest_size != 0:
++                if image_size > dest_size:
++                    raise exception.FlavorDiskTooSmall()
+ 
+-            if int(image.get('min_disk') or 0) > root_gb:
++                if image_min_disk > dest_size:
+                     raise exception.FlavorDiskTooSmall()
+ 
+     def _get_image_defined_bdms(self, base_options, instance_type, image_meta,
+@@ -767,10 +816,11 @@ class API(base.Base):
+ 
+     def _checks_for_create_and_rebuild(self, context, image_id, image,
+                                        instance_type, metadata,
+-                                       files_to_inject):
++                                       files_to_inject, root_bdm):
+         self._check_metadata_properties_quota(context, metadata)
+         self._check_injected_file_quota(context, files_to_inject)
+-        self._check_requested_image(context, image_id, image, instance_type)
++        self._check_requested_image(context, image_id, image,
++                                    instance_type, root_bdm)
+ 
+     def _validate_and_build_base_options(self, context, instance_type,
+                                          boot_meta, image_href, image_id,
+@@ -778,7 +828,7 @@ class API(base.Base):
+                                          display_description, key_name,
+                                          key_data, security_groups,
+                                          availability_zone, forced_host,
+-                                         user_data, metadata, injected_files,
++                                         user_data, metadata,
+                                          access_ip_v4, access_ip_v6,
+                                          requested_networks, config_drive,
+                                          auto_disk_config, reservation_id,
+@@ -810,9 +860,6 @@ class API(base.Base):
+             except base64.binascii.Error:
+                 raise exception.InstanceUserDataMalformed()
+ 
+-        self._checks_for_create_and_rebuild(context, image_id, boot_meta,
+-                instance_type, metadata, injected_files)
+-
+         self._check_requested_secgroups(context, security_groups)
+ 
+         # Note:  max_count is the number of instances requested by the user,
+@@ -1095,7 +1142,7 @@ class API(base.Base):
+                 instance_type, boot_meta, image_href, image_id, kernel_id,
+                 ramdisk_id, display_name, display_description,
+                 key_name, key_data, security_groups, availability_zone,
+-                forced_host, user_data, metadata, injected_files, access_ip_v4,
++                forced_host, user_data, metadata, access_ip_v4,
+                 access_ip_v6, requested_networks, config_drive,
+                 auto_disk_config, reservation_id, max_count)
+ 
+@@ -1115,6 +1162,12 @@ class API(base.Base):
+             base_options, instance_type, boot_meta, min_count, max_count,
+             block_device_mapping, legacy_bdm)
+ 
++        # We can't do this check earlier because we need bdms from all sources
++        # to have been merged in order to get the root bdm.
++        self._checks_for_create_and_rebuild(context, image_id, boot_meta,
++                instance_type, metadata, injected_files,
++                block_device_mapping.root_bdm())
++
+         instance_group = self._get_requested_instance_group(context,
+                                    scheduler_hints, check_server_group_quota)
+ 
+@@ -2333,8 +2386,9 @@ class API(base.Base):
+         self._check_auto_disk_config(image=image, **kwargs)
+ 
+         flavor = instance.get_flavor()
++        root_bdm = self._get_root_bdm(context, instance)
+         self._checks_for_create_and_rebuild(context, image_id, image,
+-                flavor, metadata, files_to_inject)
++                flavor, metadata, files_to_inject, root_bdm)
+ 
+         kernel_id, ramdisk_id = self._handle_kernel_and_ramdisk(
+                 context, None, None, image)
+@@ -3201,15 +3255,18 @@ class API(base.Base):
+         uuids = [instance.uuid for instance in instances]
+         return self.db.instance_fault_get_by_instance_uuids(context, uuids)
+ 
+-    def is_volume_backed_instance(self, context, instance, bdms=None):
+-        if not instance.image_ref:
+-            return True
+-
++    def _get_root_bdm(self, context, instance, bdms=None):
+         if bdms is None:
+             bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
+                     context, instance.uuid)
+ 
+-        root_bdm = bdms.root_bdm()
++        return bdms.root_bdm()
++
++    def is_volume_backed_instance(self, context, instance, bdms=None):
++        if not instance.image_ref:
++            return True
++
++        root_bdm = self._get_root_bdm(context, instance, bdms)
+         if not root_bdm:
+             return False
+         return root_bdm.is_volume
+diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py
+index bf36960..b18c1e5 100644
+--- a/nova/tests/unit/compute/test_compute.py
++++ b/nova/tests/unit/compute/test_compute.py
+@@ -11356,6 +11356,8 @@ class ComputeInactiveImageTestCase(BaseTestCase):
+             return {'id': id, 'min_disk': None, 'min_ram': None,
+                     'name': 'fake_name',
+                     'status': 'deleted',
++                    'min_ram': 0,
++                    'min_disk': 0,
+                     'properties': {'kernel_id': 'fake_kernel_id',
+                                    'ramdisk_id': 'fake_ramdisk_id',
+                                    'something_else': 'meow'}}
+@@ -11708,78 +11710,175 @@ class CheckRequestedImageTestCase(test.TestCase):
+ 
+     def test_no_image_specified(self):
+         self.compute_api._check_requested_image(self.context, None, None,
+-                self.instance_type)
++                self.instance_type, None)
+ 
+     def test_image_status_must_be_active(self):
+         image = dict(id='123', status='foo')
+ 
+         self.assertRaises(exception.ImageNotActive,
+                 self.compute_api._check_requested_image, self.context,
+-                image['id'], image, self.instance_type)
++                image['id'], image, self.instance_type, None)
+ 
+         image['status'] = 'active'
+         self.compute_api._check_requested_image(self.context, image['id'],
+-                image, self.instance_type)
++                image, self.instance_type, None)
+ 
+     def test_image_min_ram_check(self):
+         image = dict(id='123', status='active', min_ram='65')
+ 
+         self.assertRaises(exception.FlavorMemoryTooSmall,
+                 self.compute_api._check_requested_image, self.context,
+-                image['id'], image, self.instance_type)
++                image['id'], image, self.instance_type, None)
+ 
+         image['min_ram'] = '64'
+         self.compute_api._check_requested_image(self.context, image['id'],
+-                image, self.instance_type)
++                image, self.instance_type, None)
+ 
+     def test_image_min_disk_check(self):
+         image = dict(id='123', status='active', min_disk='2')
+ 
+         self.assertRaises(exception.FlavorDiskTooSmall,
+                 self.compute_api._check_requested_image, self.context,
+-                image['id'], image, self.instance_type)
++                image['id'], image, self.instance_type, None)
+ 
+         image['min_disk'] = '1'
+         self.compute_api._check_requested_image(self.context, image['id'],
+-                image, self.instance_type)
++                image, self.instance_type, None)
+ 
+     def test_image_too_large(self):
+         image = dict(id='123', status='active', size='1073741825')
+ 
+         self.assertRaises(exception.FlavorDiskTooSmall,
+                 self.compute_api._check_requested_image, self.context,
+-                image['id'], image, self.instance_type)
++                image['id'], image, self.instance_type, None)
+ 
+         image['size'] = '1073741824'
+         self.compute_api._check_requested_image(self.context, image['id'],
+-                image, self.instance_type)
++                image, self.instance_type, None)
+ 
+     def test_root_gb_zero_disables_size_check(self):
+         self.instance_type['root_gb'] = 0
+         image = dict(id='123', status='active', size='1073741825')
+ 
+         self.compute_api._check_requested_image(self.context, image['id'],
+-                image, self.instance_type)
++                image, self.instance_type, None)
+ 
+     def test_root_gb_zero_disables_min_disk(self):
+         self.instance_type['root_gb'] = 0
+         image = dict(id='123', status='active', min_disk='2')
+ 
+         self.compute_api._check_requested_image(self.context, image['id'],
+-                image, self.instance_type)
++                image, self.instance_type, None)
+ 
+     def test_config_drive_option(self):
+         image = {'id': 1, 'status': 'active'}
+         image['properties'] = {'img_config_drive': 'optional'}
+         self.compute_api._check_requested_image(self.context, image['id'],
+-                image, self.instance_type)
++                image, self.instance_type, None)
+         image['properties'] = {'img_config_drive': 'mandatory'}
+         self.compute_api._check_requested_image(self.context, image['id'],
+-                image, self.instance_type)
++                image, self.instance_type, None)
+         image['properties'] = {'img_config_drive': 'bar'}
+         self.assertRaises(exception.InvalidImageConfigDrive,
+                           self.compute_api._check_requested_image,
+-                          self.context, image['id'], image, self.instance_type)
++                          self.context, image['id'], image, self.instance_type,
++                          None)
++
++    def test_volume_blockdevicemapping(self):
++        # We should allow a root volume which is larger than the flavor root
++        # disk.
++        # We should allow a root volume created from an image whose min_disk is
++        # larger than the flavor root disk.
++        image_uuid = str(uuid.uuid4())
++        image = dict(id=image_uuid, status='active',
++                     size=self.instance_type.root_gb * units.Gi,
++                     min_disk=self.instance_type.root_gb + 1)
++
++        volume_uuid = str(uuid.uuid4())
++        root_bdm = block_device_obj.BlockDeviceMapping(
++            source_type='volume', destination_type='volume',
++            volume_id=volume_uuid, volume_size=self.instance_type.root_gb + 1)
++
++        self.compute_api._check_requested_image(self.context, image['id'],
++                image, self.instance_type, root_bdm)
++
++    def test_volume_blockdevicemapping_min_disk(self):
++        # A bdm object volume smaller than the image's min_disk should not be
++        # allowed
++        image_uuid = str(uuid.uuid4())
++        image = dict(id=image_uuid, status='active',
++                     size=self.instance_type.root_gb * units.Gi,
++                     min_disk=self.instance_type.root_gb + 1)
++
++        volume_uuid = str(uuid.uuid4())
++        root_bdm = block_device_obj.BlockDeviceMapping(
++            source_type='image', destination_type='volume',
++            image_id=image_uuid, volume_id=volume_uuid,
++            volume_size=self.instance_type.root_gb)
++
++        self.assertRaises(exception.FlavorDiskTooSmall,
++                          self.compute_api._check_requested_image,
++                          self.context, image_uuid, image, self.instance_type,
++                          root_bdm)
++
++    def test_volume_blockdevicemapping_min_disk_no_size(self):
++        # We should allow a root volume whose size is not given
++        image_uuid = str(uuid.uuid4())
++        image = dict(id=image_uuid, status='active',
++                     size=self.instance_type.root_gb * units.Gi,
++                     min_disk=self.instance_type.root_gb)
++
++        volume_uuid = str(uuid.uuid4())
++        root_bdm = block_device_obj.BlockDeviceMapping(
++            source_type='volume', destination_type='volume',
++            volume_id=volume_uuid, volume_size=None)
++
++        self.compute_api._check_requested_image(self.context, image['id'],
++                image, self.instance_type, root_bdm)
++
++    def test_image_blockdevicemapping(self):
++        # Test that we can succeed when passing bdms, and the root bdm isn't a
++        # volume
++        image_uuid = str(uuid.uuid4())
++        image = dict(id=image_uuid, status='active',
++                     size=self.instance_type.root_gb * units.Gi, min_disk=0)
++
++        root_bdm = block_device_obj.BlockDeviceMapping(
++            source_type='image', destination_type='local', image_id=image_uuid)
++
++        self.compute_api._check_requested_image(self.context, image['id'],
++                image, self.instance_type, root_bdm)
++
++    def test_image_blockdevicemapping_too_big(self):
++        # We should do a size check against flavor if we were passed bdms but
++        # the root bdm isn't a volume
++        image_uuid = str(uuid.uuid4())
++        image = dict(id=image_uuid, status='active',
++                     size=(self.instance_type.root_gb + 1) * units.Gi,
++                     min_disk=0)
++
++        root_bdm = block_device_obj.BlockDeviceMapping(
++            source_type='image', destination_type='local', image_id=image_uuid)
++
++        self.assertRaises(exception.FlavorDiskTooSmall,
++                          self.compute_api._check_requested_image,
++                          self.context, image['id'],
++                          image, self.instance_type, root_bdm)
++
++    def test_image_blockdevicemapping_min_disk(self):
++        # We should do a min_disk check against flavor if we were passed bdms
++        # but the root bdm isn't a volume
++        image_uuid = str(uuid.uuid4())
++        image = dict(id=image_uuid, status='active',
++                     size=0, min_disk=self.instance_type.root_gb + 1)
++
++        root_bdm = block_device_obj.BlockDeviceMapping(
++            source_type='image', destination_type='local', image_id=image_uuid)
++
++        self.assertRaises(exception.FlavorDiskTooSmall,
++                          self.compute_api._check_requested_image,
++                          self.context, image['id'],
++                          image, self.instance_type, root_bdm)
+ 
+ 
+ class ComputeHooksTestCase(test.BaseHookTestCase):
+diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py
+index 47848a9..5b4c2b3 100644
+--- a/nova/tests/unit/compute/test_compute_api.py
++++ b/nova/tests/unit/compute/test_compute_api.py
+@@ -2257,15 +2257,16 @@ class _ComputeAPIUnitTestMixIn(object):
+                 vm_state=vm_states.ACTIVE, cell_name='fake-cell',
+                 launched_at=timeutils.utcnow(),
+                 system_metadata=orig_system_metadata,
++                image_ref='foo',
+                 expected_attrs=['system_metadata'])
+         get_flavor.return_value = test_flavor.fake_flavor
+         flavor = instance.get_flavor()
+-        image_href = ''
++        image_href = 'foo'
+         image = {"min_ram": 10, "min_disk": 1,
+                  "properties": {'architecture': arch.X86_64}}
+         admin_pass = ''
+         files_to_inject = []
+-        bdms = []
++        bdms = objects.BlockDeviceMappingList()
+ 
+         _get_image.return_value = (None, image)
+         bdm_get_by_instance_uuid.return_value = bdms
+@@ -2284,7 +2285,7 @@ class _ComputeAPIUnitTestMixIn(object):
+ 
+         _check_auto_disk_config.assert_called_once_with(image=image)
+         _checks_for_create_and_rebuild.assert_called_once_with(self.context,
+-                None, image, flavor, {}, [])
++                None, image, flavor, {}, [], None)
+         self.assertNotEqual(orig_system_metadata, instance.system_metadata)
+ 
+     @mock.patch.object(objects.Instance, 'save')
+@@ -2309,7 +2310,7 @@ class _ComputeAPIUnitTestMixIn(object):
+                                     'vm_mode': 'xen'}}
+         admin_pass = ''
+         files_to_inject = []
+-        bdms = []
++        bdms = objects.BlockDeviceMappingList()
+ 
+         instance = fake_instance.fake_instance_obj(self.context,
+                 vm_state=vm_states.ACTIVE, cell_name='fake-cell',
+@@ -2342,7 +2343,7 @@ class _ComputeAPIUnitTestMixIn(object):
+ 
+         _check_auto_disk_config.assert_called_once_with(image=new_image)
+         _checks_for_create_and_rebuild.assert_called_once_with(self.context,
+-                None, new_image, flavor, {}, [])
++                None, new_image, flavor, {}, [], None)
+         self.assertEqual(vm_mode.XEN, instance.vm_mode)
+ 
+     def _test_check_injected_file_quota_onset_file_limit_exceeded(self,
+diff --git a/nova/tests/unit/compute/test_compute_cells.py b/nova/tests/unit/compute/test_compute_cells.py
+index a769e1d..77d3dd7 100644
+--- a/nova/tests/unit/compute/test_compute_cells.py
++++ b/nova/tests/unit/compute/test_compute_cells.py
+@@ -236,11 +236,13 @@ class CellsConductorAPIRPCRedirect(test.NoDBTestCase):
+     @mock.patch.object(compute_api.API, '_check_and_transform_bdm')
+     @mock.patch.object(compute_api.API, '_get_image')
+     @mock.patch.object(compute_api.API, '_validate_and_build_base_options')
+-    def test_build_instances(self, _validate, _get_image, _check_bdm,
++    @mock.patch.object(compute_api.API, '_checks_for_create_and_rebuild')
++    def test_build_instances(self, _checks_for_create_and_rebuild,
++                             _validate, _get_image, _check_bdm,
+                              _provision, _record_action_start):
+         _get_image.return_value = (None, 'fake-image')
+         _validate.return_value = ({}, 1)
+-        _check_bdm.return_value = 'bdms'
++        _check_bdm.return_value = objects.BlockDeviceMappingList()
+         _provision.return_value = 'instances'
+ 
+         self.compute_api.create(self.context, 'fake-flavor', 'fake-image')
+@@ -309,7 +311,7 @@ class CellsConductorAPIRPCRedirect(test.NoDBTestCase):
+                  "properties": {'architecture': 'x86_64'}}
+         admin_pass = ''
+         files_to_inject = []
+-        bdms = []
++        bdms = objects.BlockDeviceMappingList()
+ 
+         _get_image.return_value = (None, image)
+         bdm_get_by_instance_uuid.return_value = bdms
+-- 
+2.1.4
+

=== modified file 'debian/patches/series'
--- debian/patches/series	2015-08-03 20:04:14 +0000
+++ debian/patches/series	2015-09-08 23:22:49 +0000
@@ -7,3 +7,4 @@
 #rate-limit-power-syncs.patch
 skip-ubuntu-tests.patch
 skip-proxy-test.patch
+not-check-disk-size.patch