Merge "Fix allocated_capacity_gb race on create volume"

cinder/tests/unit/volume/test_volume.py

@@ -233,7 +233,8 @@ class VolumeTestCase(base.BaseVolumeTestCase):
         # NOTE(dulek): Volume should be rescheduled as we passed request_spec
         # and filter_properties, assert that it wasn't counted in
         # allocated_capacity tracking.
-        self.assertEqual({}, self.volume.stats['pools'])
+        self.assertEqual({'_pool0': {'allocated_capacity_gb': 0}},
+                         self.volume.stats['pools'])
 
         # NOTE(dulek): As we've rescheduled, make sure delete_volume was
         # called.
@@ -260,7 +261,8 @@ class VolumeTestCase(base.BaseVolumeTestCase):
         # NOTE(dulek): Volume should be rescheduled as we passed request_spec
         # and filter_properties, assert that it wasn't counted in
         # allocated_capacity tracking.
-        self.assertEqual({}, self.volume.stats['pools'])
+        self.assertEqual({'_pool0': {'allocated_capacity_gb': 0}},
+                         self.volume.stats['pools'])
 
         db.volume_destroy(context.get_admin_context(), volume_id)
 
@@ -317,6 +319,8 @@ class VolumeTestCase(base.BaseVolumeTestCase):
         self.assert_notify_called(mock_notify,
                                   (['INFO', 'volume.create.start'],
                                    ['INFO', 'volume.create.end']))
+        self.assertEqual({'_pool0': {'allocated_capacity_gb': 1}},
+                         self.volume.stats['pools'])
 
         self.volume.delete_volume(self.context, volume)
         vol = db.volume_get(context.get_admin_context(read_deleted='yes'),
@@ -328,6 +332,8 @@ class VolumeTestCase(base.BaseVolumeTestCase):
                                    ['INFO', 'volume.create.end'],
                                    ['INFO', 'volume.delete.start'],
                                    ['INFO', 'volume.delete.end']))
+        self.assertEqual({'_pool0': {'allocated_capacity_gb': 0}},
+                         self.volume.stats['pools'])
 
         self.assertRaises(exception.NotFound,
                           db.volume_get,
@@ -2757,6 +2763,10 @@ class VolumeTestCase(base.BaseVolumeTestCase):
         self.assertTrue(mock_reschedule.called)
         volume = db.volume_get(context.get_admin_context(), test_vol_id)
         self.assertEqual('creating', volume['status'])
+        # We increase the stats on entering the create method, but we must
+        # have cleared them on reschedule.
+        self.assertEqual({'_pool0': {'allocated_capacity_gb': 0}},
+                         self.volume.stats['pools'])
 
     @mock.patch('cinder.volume.flows.manager.create_volume.'
                 'CreateVolumeFromSpecTask.execute')
@@ -2774,6 +2784,8 @@ class VolumeTestCase(base.BaseVolumeTestCase):
                           {'retry': {'num_attempts': 1, 'host': []}})
         volume = db.volume_get(context.get_admin_context(), test_vol_id)
         self.assertEqual('error', volume['status'])
+        self.assertEqual({'_pool0': {'allocated_capacity_gb': 1}},
+                         self.volume.stats['pools'])
 
     def test_cascade_delete_volume_with_snapshots(self):
         """Test volume deletion with dependent snapshots."""

cinder/volume/manager.py

@@ -584,6 +584,12 @@ class VolumeManager(manager.CleanableManager,
         # Make sure the host in the DB matches our own when clustered
         self._set_resource_host(volume)
 
+        # Update our allocated capacity counter early to minimize race
+        # conditions with the scheduler.
+        self._update_allocated_capacity(volume)
+        # We lose the host value if we reschedule, so keep it here
+        original_host = volume.host
+
         context_elevated = context.elevated()
         if filter_properties is None:
             filter_properties = {}
@@ -656,10 +662,12 @@ class VolumeManager(manager.CleanableManager,
                 except tfe.NotFound:
                     pass
 
-            if not rescheduled:
+            if rescheduled:
-                # NOTE(dulek): Volume wasn't rescheduled so we need to update
+                # NOTE(geguileo): Volume was rescheduled so we need to update
-                # volume stats as these are decremented on delete.
+                # volume stats because the volume wasn't created here.
-                self._update_allocated_capacity(volume)
+                # Volume.host is None now, so we pass the original host value.
+                self._update_allocated_capacity(volume, decrement=True,
+                                                host=original_host)
 
         shared_targets = (
             1
@@ -842,20 +850,7 @@ class VolumeManager(manager.CleanableManager,
             if reservations:
                 QUOTAS.commit(context, reservations, project_id=project_id)
 
-            pool = vol_utils.extract_host(volume.host, 'pool')
+            self._update_allocated_capacity(volume, decrement=True)
-            if pool is None:
-                # Legacy volume, put them into default pool
-                pool = self.driver.configuration.safe_get(
-                    'volume_backend_name') or vol_utils.extract_host(
-                        volume.host, 'pool', True)
-            size = volume.size
-
-            try:
-                self.stats['pools'][pool]['allocated_capacity_gb'] -= size
-            except KeyError:
-                self.stats['pools'][pool] = dict(
-                    allocated_capacity_gb=-size)
-
             self.publish_service_capabilities(context)
 
         msg = "Deleted volume successfully."
@@ -3263,21 +3258,22 @@ class VolumeManager(manager.CleanableManager,
 
         self.db.volume_update(context, vol['id'], update)
 
-    def _update_allocated_capacity(self, vol):
+    def _update_allocated_capacity(self, vol, decrement=False, host=None):
         # Update allocated capacity in volume stats
-        pool = vol_utils.extract_host(vol['host'], 'pool')
+        host = host or vol['host']
+        pool = vol_utils.extract_host(host, 'pool')
         if pool is None:
             # Legacy volume, put them into default pool
             pool = self.driver.configuration.safe_get(
-                'volume_backend_name') or vol_utils.extract_host(
+                'volume_backend_name') or vol_utils.extract_host(host, 'pool',
-                    vol['host'], 'pool', True)
+                                                                 True)
 
+        vol_size = -vol['size'] if decrement else vol['size']
         try:
-            self.stats['pools'][pool]['allocated_capacity_gb'] += (
+            self.stats['pools'][pool]['allocated_capacity_gb'] += vol_size
-                vol['size'])
         except KeyError:
             self.stats['pools'][pool] = dict(
-                allocated_capacity_gb=vol['size'])
+                allocated_capacity_gb=max(vol_size, 0))
 
     def delete_group(self, context, group):
         """Deletes group and the volumes in the group."""