Remove object to dictionary translation logic

If cinder manages hundred of volumes, the cinder
volume service is taking a long time to start up.

Most of time is spent at object to dictionary
conversion in list_of_dicts_to_dict().

  dict(d, index=d[key])

We should remove this logic to avoid performance
degradation, and then use simple iteration for
referring volume object same as snapshot.

Also snapshot_update() in _sync_provider_info()
would be better to use 'update' instead of 'updt'.

Change-Id: Idde635b9c488e40e53377e0f3b48c8478df97a84
Closes-Bug: #1582505

cinder/tests/unit/test_utils.py

@@ -352,15 +352,6 @@ class GenericUtilsTestCase(test.TestCase):
         self.assertEqual('sudo cinder-rootwrap /path/to/conf',
                          utils.get_root_helper())
 
-    def test_list_of_dicts_to_dict(self):
-        a = {'id': '1', 'color': 'orange'}
-        b = {'id': '2', 'color': 'blue'}
-        c = {'id': '3', 'color': 'green'}
-        lst = [a, b, c]
-
-        resp = utils.list_of_dicts_to_dict(lst, 'id')
-        self.assertEqual(c['id'], resp['3']['id'])
-
 
 class TemporaryChownTestCase(test.TestCase):
     @mock.patch('os.stat')

cinder/tests/unit/test_volume.py

@@ -441,6 +441,69 @@ class VolumeTestCase(BaseVolumeTestCase):
         self.volume.delete_volume(self.context, vol3['id'])
         self.volume.delete_volume(self.context, vol4['id'])
 
+    @mock.patch.object(driver.BaseVD, "update_provider_info")
+    def test_init_host_sync_provider_info(self, mock_update):
+        vol0 = tests_utils.create_volume(
+            self.context, size=1, host=CONF.host)
+        vol1 = tests_utils.create_volume(
+            self.context, size=1, host=CONF.host)
+        snap0 = tests_utils.create_snapshot(self.context, vol0.id)
+        snap1 = tests_utils.create_snapshot(self.context, vol1.id)
+        # Return values for update_provider_info
+        volumes = [{'id': vol0.id, 'provider_id': '1 2 xxxx'},
+                   {'id': vol1.id, 'provider_id': '3 4 yyyy'}]
+        snapshots = [{'id': snap0.id, 'provider_id': '5 6 xxxx'},
+                     {'id': snap1.id, 'provider_id': '7 8 yyyy'}]
+        mock_update.return_value = (volumes, snapshots)
+        # initialize
+        self.volume.init_host()
+        # Grab volume and snapshot objects
+        vol0_obj = objects.Volume.get_by_id(context.get_admin_context(),
+                                            vol0.id)
+        vol1_obj = objects.Volume.get_by_id(context.get_admin_context(),
+                                            vol1.id)
+        snap0_obj = objects.Snapshot.get_by_id(self.context, snap0.id)
+        snap1_obj = objects.Snapshot.get_by_id(self.context, snap1.id)
+        # Check updated provider ids
+        self.assertEqual('1 2 xxxx', vol0_obj.provider_id)
+        self.assertEqual('3 4 yyyy', vol1_obj.provider_id)
+        self.assertEqual('5 6 xxxx', snap0_obj.provider_id)
+        self.assertEqual('7 8 yyyy', snap1_obj.provider_id)
+        # Clean up
+        self.volume.delete_snapshot(self.context, snap0_obj)
+        self.volume.delete_snapshot(self.context, snap1_obj)
+        self.volume.delete_volume(self.context, vol0.id)
+        self.volume.delete_volume(self.context, vol1.id)
+
+    @mock.patch.object(driver.BaseVD, "update_provider_info")
+    def test_init_host_sync_provider_info_no_update(self, mock_update):
+        vol0 = tests_utils.create_volume(
+            self.context, size=1, host=CONF.host)
+        vol1 = tests_utils.create_volume(
+            self.context, size=1, host=CONF.host)
+        snap0 = tests_utils.create_snapshot(self.context, vol0.id)
+        snap1 = tests_utils.create_snapshot(self.context, vol1.id)
+        mock_update.return_value = ([], [])
+        # initialize
+        self.volume.init_host()
+        # Grab volume and snapshot objects
+        vol0_obj = objects.Volume.get_by_id(context.get_admin_context(),
+                                            vol0.id)
+        vol1_obj = objects.Volume.get_by_id(context.get_admin_context(),
+                                            vol1.id)
+        snap0_obj = objects.Snapshot.get_by_id(self.context, snap0.id)
+        snap1_obj = objects.Snapshot.get_by_id(self.context, snap1.id)
+        # Check provider ids are not changed
+        self.assertIsNone(vol0_obj.provider_id)
+        self.assertIsNone(vol1_obj.provider_id)
+        self.assertIsNone(snap0_obj.provider_id)
+        self.assertIsNone(snap1_obj.provider_id)
+        # Clean up
+        self.volume.delete_snapshot(self.context, snap0_obj)
+        self.volume.delete_snapshot(self.context, snap1_obj)
+        self.volume.delete_volume(self.context, vol0.id)
+        self.volume.delete_volume(self.context, vol1.id)
+
     @mock.patch('cinder.objects.service.Service.get_minimum_rpc_version')
     @mock.patch('cinder.objects.service.Service.get_minimum_obj_version')
     @mock.patch('cinder.rpc.LAST_RPC_VERSIONS', {'cinder-scheduler': '1.3'})

cinder/utils.py

@@ -247,24 +247,6 @@ def last_completed_audit_period(unit=None):
     return (begin, end)
 
 
-def list_of_dicts_to_dict(seq, key):
-    """Convert list of dicts to an indexed dict.
-
-    Takes a list of dicts, and converts it to a nested dict
-    indexed by <key>
-
-    :param seq: list of dicts
-    :parm key: key in dicts to index by
-
-    example:
-      lst = [{'id': 1, ...}, {'id': 2, ...}...]
-      key = 'id'
-      returns {1:{'id': 1, ...}, 2:{'id':2, ...}
-
-    """
-    return {d[key]: dict(d, index=d[key]) for (i, d) in enumerate(seq)}
-
-
 class ProtectedExpatParser(expatreader.ExpatParser):
     """An expat parser which disables DTD's and entities by default."""
 

cinder/volume/manager.py

@@ -388,17 +388,18 @@ class VolumeManager(manager.SchedulerDependentManager):
 
         updates, snapshot_updates = self.driver.update_provider_info(
             volumes, snapshots)
-        host_vols = utils.list_of_dicts_to_dict(volumes, 'id')
 
-        for u in updates or []:
-            update = {}
-            # NOTE(JDG): Make sure returned item is in this hosts volumes
-            if host_vols.get(u['id'], None):
-                update['provider_id'] = u['provider_id']
-            if update:
-                self.db.volume_update(ctxt,
-                                      u['id'],
-                                      update)
+        if updates:
+            for volume in volumes:
+                # NOTE(JDG): Make sure returned item is in this hosts volumes
+                update = (
+                    [updt for updt in updates if updt['id'] ==
+                        volume['id']][0])
+                if update:
+                    self.db.volume_update(
+                        ctxt,
+                        update['id'],
+                        {'provider_id': update['provider_id']})
 
         # NOTE(jdg): snapshots are slighty harder, because
         # we do not have a host column and of course no get
@@ -415,8 +416,8 @@ class VolumeManager(manager.SchedulerDependentManager):
                     if update:
                         self.db.snapshot_update(
                             ctxt,
-                            updt['id'],
-                            {'provider_id': updt['provider_id']})
+                            update['id'],
+                            {'provider_id': update['provider_id']})
 
     def init_host(self):
         """Perform any required initialization."""