Merge "Check quota before creating volume snapshots" into stable/queens

3 files changed, 92 insertions, 30 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 6dfa6baad8..fe8c7af802 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -2855,6 +2855,40 @@ class API(base.Base):
         if instance.root_device_name:
             properties['root_device_name'] = instance.root_device_name
 
+        bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
+                context, instance.uuid)
+
+        mapping = []  # list of BDM dicts that can go into the image properties
+        # Do some up-front filtering of the list of BDMs from
+        # which we are going to create snapshots.
+        volume_bdms = []
+        for bdm in bdms:
+            if bdm.no_device:
+                continue
+            if bdm.is_volume:
+                # These will be handled below.
+                volume_bdms.append(bdm)
+            else:
+                mapping.append(bdm.get_image_mapping())
+
+        # Check limits in Cinder before creating snapshots to avoid going over
+        # quota in the middle of a list of volumes. This is a best-effort check
+        # but concurrently running snapshot requests from the same project
+        # could still fail to create volume snapshots if they go over limit.
+        if volume_bdms:
+            limits = self.volume_api.get_absolute_limits(context)
+            total_snapshots_used = limits['totalSnapshotsUsed']
+            max_snapshots = limits['maxTotalSnapshots']
+            # -1 means there is unlimited quota for snapshots
+            if (max_snapshots > -1 and
+                    len(volume_bdms) + total_snapshots_used > max_snapshots):
+                LOG.debug('Unable to create volume snapshots for instance. '
+                          'Currently has %s snapshots, requesting %s new '
+                          'snapshots, with a limit of %s.',
+                          total_snapshots_used, len(volume_bdms),
+                          max_snapshots, instance=instance)
+                raise exception.OverQuota(overs='snapshots')
+
         quiesced = False
         if instance.vm_state == vm_states.ACTIVE:
             try:
@@ -2873,35 +2907,24 @@ class API(base.Base):
                              {'reason': err},
                              instance=instance)
 
-        bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
-                context, instance.uuid)
-
         @wrap_instance_event(prefix='api')
         def snapshot_instance(self, context, instance, bdms):
-            mapping = []
             try:
-                for bdm in bdms:
-                    if bdm.no_device:
-                        continue
-
-                    if bdm.is_volume:
-                        # create snapshot based on volume_id
-                        volume = self.volume_api.get(context, bdm.volume_id)
-                        # NOTE(yamahata): Should we wait for snapshot creation?
-                        # Linux LVM snapshot creation completes in short time,
-                        # it doesn't matter for now.
-                        name = _('snapshot for %s') % image_meta['name']
-                        LOG.debug('Creating snapshot from volume %s.',
-                                  volume['id'], instance=instance)
-                        snapshot = self.volume_api.create_snapshot_force(
-                            context, volume['id'],
-                            name, volume['display_description'])
-                        mapping_dict = block_device.snapshot_from_bdm(
-                            snapshot['id'], bdm)
-                        mapping_dict = mapping_dict.get_image_mapping()
-                    else:
-                        mapping_dict = bdm.get_image_mapping()
-
+                for bdm in volume_bdms:
+                    # create snapshot based on volume_id
+                    volume = self.volume_api.get(context, bdm.volume_id)
+                    # NOTE(yamahata): Should we wait for snapshot creation?
+                    #                 Linux LVM snapshot creation completes in
+                    #                 short time, it doesn't matter for now.
+                    name = _('snapshot for %s') % image_meta['name']
+                    LOG.debug('Creating snapshot from volume %s.',
+                              volume['id'], instance=instance)
+                    snapshot = self.volume_api.create_snapshot_force(
+                        context, volume['id'],
+                        name, volume['display_description'])
+                    mapping_dict = block_device.snapshot_from_bdm(
+                        snapshot['id'], bdm)
+                    mapping_dict = mapping_dict.get_image_mapping()
                     mapping.append(mapping_dict)
                 return mapping
             # NOTE(tasker): No error handling is done in the above for loop.
diff --git a/nova/tests/unit/api/openstack/compute/test_server_actions.py b/nova/tests/unit/api/openstack/compute/test_server_actions.py
index dd590a5023..5b846ca12e 100644
--- a/nova/tests/unit/api/openstack/compute/test_server_actions.py
+++ b/nova/tests/unit/api/openstack/compute/test_server_actions.py
@@ -982,6 +982,10 @@ class ServerActionsControllerTestV21(test.TestCase):
         snapshot = dict(id=_fake_id('d'))
 
         with test.nested(
+            mock.patch.object(
+                self.controller.compute_api.volume_api, 'get_absolute_limits',
+                return_value={'totalSnapshotsUsed': 0,
+                              'maxTotalSnapshots': 10}),
             mock.patch.object(self.controller.compute_api.compute_rpcapi,
                 'quiesce_instance',
                 side_effect=exception.InstanceQuiesceNotSupported(
@@ -991,7 +995,7 @@ class ServerActionsControllerTestV21(test.TestCase):
             mock.patch.object(self.controller.compute_api.volume_api,
                               'create_snapshot_force',
                               return_value=snapshot),
-        ) as (mock_quiesce, mock_vol_get, mock_vol_create):
+        ) as (mock_get_limits, mock_quiesce, mock_vol_get, mock_vol_create):
 
             if mock_vol_create_side_effect:
                 mock_vol_create.side_effect = mock_vol_create_side_effect
@@ -1086,6 +1090,10 @@ class ServerActionsControllerTestV21(test.TestCase):
         snapshot = dict(id=_fake_id('d'))
 
         with test.nested(
+            mock.patch.object(
+                self.controller.compute_api.volume_api, 'get_absolute_limits',
+                return_value={'totalSnapshotsUsed': 0,
+                              'maxTotalSnapshots': 10}),
             mock.patch.object(self.controller.compute_api.compute_rpcapi,
                 'quiesce_instance',
                 side_effect=exception.InstanceQuiesceNotSupported(
@@ -1095,7 +1103,7 @@ class ServerActionsControllerTestV21(test.TestCase):
             mock.patch.object(self.controller.compute_api.volume_api,
                               'create_snapshot_force',
                               return_value=snapshot),
-        ) as (mock_quiesce, mock_vol_get, mock_vol_create):
+        ) as (mock_get_limits, mock_quiesce, mock_vol_get, mock_vol_create):
 
             response = self.controller._action_create_image(self.req,
                 FAKE_UUID, body=body)
diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py
index 9c67da19fe..ae6ff253f9 100644
--- a/nova/tests/unit/compute/test_compute_api.py
+++ b/nova/tests/unit/compute/test_compute_api.py
@@ -3044,7 +3044,7 @@ class _ComputeAPIUnitTestMixIn(object):
 
     def _test_snapshot_volume_backed(self, quiesce_required, quiesce_fails,
                                      vm_state=vm_states.ACTIVE,
-                                     snapshot_fails=False):
+                                     snapshot_fails=False, limits=None):
         fake_sys_meta = {'image_min_ram': '11',
                          'image_min_disk': '22',
                          'image_container_format': 'ami',
@@ -3103,6 +3103,11 @@ class _ComputeAPIUnitTestMixIn(object):
         def fake_unquiesce_instance(context, instance, mapping=None):
             quiesced[1] = True
 
+        def fake_get_absolute_limits(context):
+            if limits is not None:
+                return limits
+            return {"totalSnapshotsUsed": 0, "maxTotalSnapshots": 10}
+
         self.stub_out('nova.objects.BlockDeviceMappingList'
                       '.get_by_instance_uuid',
                       fake_bdm_list_get_by_instance_uuid)
@@ -3151,6 +3156,12 @@ class _ComputeAPIUnitTestMixIn(object):
              'destination_type': 'volume', 'delete_on_termination': False,
              'tag': None})
 
+        limits_patcher = mock.patch.object(
+            self.compute_api.volume_api, 'get_absolute_limits',
+            side_effect=fake_get_absolute_limits)
+        limits_patcher.start()
+        self.addCleanup(limits_patcher.stop)
+
         with test.nested(
                 mock.patch.object(compute_api.API, '_record_action_start'),
                 mock.patch.object(compute_utils, 'EventReporter')) as (
@@ -3195,7 +3206,9 @@ class _ComputeAPIUnitTestMixIn(object):
                  'guest_format': 'swap', 'delete_on_termination': True,
                  'tag': None})
         instance_bdms.append(bdm)
-        expect_meta['properties']['block_device_mapping'].append(
+        # The non-volume image mapping will go at the front of the list
+        # because the volume BDMs are processed separately.
+        expect_meta['properties']['block_device_mapping'].insert(0,
             {'guest_format': 'swap', 'boot_index': -1, 'no_device': False,
              'image_id': None, 'volume_id': None, 'disk_bus': None,
              'volume_size': None, 'source_type': 'blank',
@@ -3241,6 +3254,24 @@ class _ComputeAPIUnitTestMixIn(object):
                                           quiesce_fails=False,
                                           snapshot_fails=True)
 
+    def test_snapshot_volume_backed_unlimited_quota(self):
+        """Tests that there is unlimited quota on volume snapshots so we
+        don't perform a quota check.
+        """
+        limits = {'maxTotalSnapshots': -1, 'totalSnapshotsUsed': 0}
+        self._test_snapshot_volume_backed(
+            quiesce_required=False, quiesce_fails=False, limits=limits)
+
+    def test_snapshot_volume_backed_over_quota_before_snapshot(self):
+        """Tests that the up-front check on quota fails before actually
+        attempting to snapshot any volumes.
+        """
+        limits = {'maxTotalSnapshots': 1, 'totalSnapshotsUsed': 1}
+        self.assertRaises(exception.OverQuota,
+                          self._test_snapshot_volume_backed,
+                          quiesce_required=False, quiesce_fails=False,
+                          limits=limits)
+
     def test_snapshot_volume_backed_with_quiesce_skipped(self):
         self._test_snapshot_volume_backed(False, True)