Reverted some changes to instance_get_all_by_filters() that was added in rev 1594. An additional argument for filtering on instance uuids is not needed, as you can add 'uuid: uuid_list' into the filters dictionary. Just needed to add 'uuid' as an exact_match_filter. This restores the filtering to do a single DB query.

Also updated ID/UUID mapping code to be a little more efficient, by returning a dictionary of 'ID: UUID'... vs a list.

Fixed a test that assumed list order.

A couple of typo fixes and a pep8 issue in trunk also fixed.

7 files changed, 37 insertions, 51 deletions

diff --git a/nova/compute/api.py b/nova/compute/api.py
index 853e6ef9e..76e1e7a60 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -937,9 +937,10 @@ class API(base.Base):
                                                                    filters)
             # NOTE(jkoelker) It is possible that we will get the same
             #                instance uuid twice (one for ipv4 and ipv6)
-            ids = set([r['instance_uuid'] for r in res])
+            uuids = set([r['instance_uuid'] for r in res])
+            filters['uuid'] = uuids
 
-        return self.db.instance_get_all_by_filters(context, filters, ids)
+        return self.db.instance_get_all_by_filters(context, filters)
 
     def _cast_compute_message(self, method, context, instance_id, host=None,
                               params=None):
diff --git a/nova/db/api.py b/nova/db/api.py
index cdc7089cc..8c4c78374 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -468,7 +468,7 @@ def virtual_interface_delete_by_instance(context, instance_id):
 
 
 def virtual_interface_get_all(context):
-    """Gets all virtual interfaces from the table,"""
+    """Gets all virtual interfaces from the table"""
     return IMPL.virtual_interface_get_all(context)
 
 
@@ -510,10 +510,9 @@ def instance_get_all(context):
     return IMPL.instance_get_all(context)
 
 
-def instance_get_all_by_filters(context, filters, instance_uuids=None):
+def instance_get_all_by_filters(context, filters):
     """Get all instances that match all filters."""
-    return IMPL.instance_get_all_by_filters(context, filters,
-                                            instance_uuids=instance_uuids)
+    return IMPL.instance_get_all_by_filters(context, filters)
 
 
 def instance_get_active_by_window(context, begin, end=None, project_id=None):
@@ -617,9 +616,9 @@ def instance_get_actions(context, instance_id):
     return IMPL.instance_get_actions(context, instance_id)
 
 
-def instance_get_uuids_by_ids(context, ids):
-    """Return the UUIDs of the instances given the ids"""
-    return IMPL.instance_get_uuids_by_ids(context, ids)
+def instance_get_id_to_uuid_mapping(context, ids):
+    """Return a dictionary containing 'ID: UUID' given the ids"""
+    return IMPL.instance_get_id_to_uuid_mapping(context, ids)
 
 
 ###################
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index e7bd852f4..d8b5ef5c2 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -1204,7 +1204,7 @@ def instance_get_all(context):
 
 
 @require_context
-def instance_get_all_by_filters(context, filters, instance_uuids=None):
+def instance_get_all_by_filters(context, filters):
     """Return instances that match all filters.  Deleted instances
     will be returned by default, unless there's a filter that says
     otherwise"""
@@ -1235,7 +1235,7 @@ def instance_get_all_by_filters(context, filters, instance_uuids=None):
         """Do exact match against a column.  value to match can be a list
         so you can match any value in the list.
         """
-        if isinstance(value, list):
+        if isinstance(value, list) or isinstance(value, set):
             column_attr = getattr(models.Instance, column)
             return query.filter(column_attr.in_(value))
         else:
@@ -1269,7 +1269,7 @@ def instance_get_all_by_filters(context, filters, instance_uuids=None):
     # Filters for exact matches that we can do along with the SQL query...
     # For other filters that don't match this, we will do regexp matching
     exact_match_filter_names = ['project_id', 'user_id', 'image_ref',
-            'vm_state', 'instance_type_id', 'deleted']
+            'vm_state', 'instance_type_id', 'deleted', 'uuid']
 
     query_filters = [key for key in filters.iterkeys()
             if key in exact_match_filter_names]
@@ -1280,30 +1280,10 @@ def instance_get_all_by_filters(context, filters, instance_uuids=None):
         query_prefix = _exact_match_filter(query_prefix, filter_name,
                 filters.pop(filter_name))
 
-    db_instances = query_prefix.all()
-    db_uuids = [instance['uuid'] for instance in db_instances \
-                if instance['uuid']]
-
-    if instance_uuids is None:
-        instance_uuids = []
-
-    uuids = []
-
-    # NOTE(jkoelker): String UUIDs only!
-    if not instance_uuids:
-        uuids = db_uuids
-    elif not db_instances:
-        uuids = instance_uuids
-    else:
-        uuids = list(set(instance_uuids) & set(db_uuids))
-
-    if not uuids:
+    instances = query_prefix.all()
+    if not instances:
         return []
 
-    instances = session.query(models.Instance).\
-                        filter(models.Instance.uuid.in_(uuids)).\
-                        all()
-
     # Now filter on everything else for regexp matching..
     # For filters not in the list, we'll attempt to use the filter_name
     # as a column name in Instance..
@@ -1321,6 +1301,8 @@ def instance_get_all_by_filters(context, filters, instance_uuids=None):
             filter_l = lambda instance: _regexp_filter_by_column(instance,
                     filter_name, filter_re)
         instances = filter(filter_l, instances)
+        if not instances:
+            break
 
     return instances
 
@@ -1565,13 +1547,15 @@ def instance_get_actions(context, instance_id):
 
 
 @require_context
-def instance_get_uuids_by_ids(context, ids):
+def instance_get_id_to_uuid_mapping(context, ids):
     session = get_session()
     instances = session.query(models.Instance).\
                         filter(models.Instance.id.in_(ids)).\
                         all()
-    return [{'uuid': instance['uuid'],
-             'id': instance['id']} for instance in instances]
+    mapping = {}
+    for instance in instances:
+        mapping[instance['id']] = instance['uuid']
+    return mapping
 
 
 ###################
diff --git a/nova/network/manager.py b/nova/network/manager.py
index 2687d8faf..ffb9f976c 100644
--- a/nova/network/manager.py
+++ b/nova/network/manager.py
@@ -410,7 +410,7 @@ class NetworkManager(manager.SchedulerDependentManager):
         ip_filter = re.compile(str(filters.get('ip')))
         ipv6_filter = re.compile(str(filters.get('ip6')))
 
-        # NOTE(jkoelker) Should probably figur out a better way to do
+        # NOTE(jkoelker) Should probably figure out a better way to do
         #                this. But for now it "works", this could suck on
         #                large installs.
 
@@ -448,12 +448,9 @@ class NetworkManager(manager.SchedulerDependentManager):
 
         # NOTE(jkoelker) Until we switch over to instance_uuid ;)
         ids = [res['instance_id'] for res in results]
-        uuids = self.db.instance_get_uuids_by_ids(context, ids)
+        uuid_map = self.db.instance_get_id_to_uuid_mapping(context, ids)
         for res in results:
-            uuid = [u['uuid'] for u in uuids if u['id'] == res['instance_id']]
-            # NOTE(jkoelker) UUID must exist, so no test here
-            res['instance_uuid'] = uuid[0]
-
+            res['instance_uuid'] = uuid_map.get(res['instance_id'])
         return results
 
     def _get_networks_for_instance(self, context, instance_id, project_id,
diff --git a/nova/tests/fake_network.py b/nova/tests/fake_network.py
index d839f20b0..febac5e09 100644
--- a/nova/tests/fake_network.py
+++ b/nova/tests/fake_network.py
@@ -105,10 +105,12 @@ class FakeNetworkManager(network_manager.NetworkManager):
                                     'floating_ips': [floats[2]]}]}]
             return vifs
 
-        def instance_get_uuids_by_ids(self, context, ids):
+        def instance_get_id_to_uuid_mapping(self, context, ids):
             # NOTE(jkoelker): This is just here until we can rely on UUIDs
-            return [{'uuid': str(utils.gen_uuid()),
-                     'id': id} for id in ids]
+            mapping = {}
+            for id in ids:
+                mapping[id] = str(utils.gen_uuid())
+            return mapping
 
     def __init__(self):
         self.db = self.FakeDB()
diff --git a/nova/tests/test_compute.py b/nova/tests/test_compute.py
index 4ef57b760..948c7ad40 100644
--- a/nova/tests/test_compute.py
+++ b/nova/tests/test_compute.py
@@ -1033,8 +1033,8 @@ class ComputeTestCase(test.TestCase):
                        'get_instance_uuids_by_ip_filter',
                        network_manager.get_instance_uuids_by_ip_filter)
         self.stubs.Set(network_manager.db,
-                       'instance_get_uuids_by_ids',
-                       db.instance_get_uuids_by_ids)
+                       'instance_get_id_to_uuid_mapping',
+                       db.instance_get_id_to_uuid_mapping)
 
         instance_id1 = self._create_instance({'display_name': 'woot',
                                               'id': 0})
diff --git a/nova/tests/test_db_api.py b/nova/tests/test_db_api.py
index 35c2ed0e1..81194e3f9 100644
--- a/nova/tests/test_db_api.py
+++ b/nova/tests/test_db_api.py
@@ -94,9 +94,12 @@ class DbApiTestCase(test.TestCase):
         db.instance_destroy(self.context, inst1.id)
         result = db.instance_get_all_by_filters(self.context.elevated(), {})
         self.assertEqual(2, len(result))
-        self.assertEqual(result[0].id, inst1.id)
-        self.assertEqual(result[1].id, inst2.id)
-        self.assertTrue(result[0].deleted)
+        self.assertIn(inst1.id, [result[0].id, result[1].id])
+        self.assertIn(inst2.id, [result[0].id, result[1].id])
+        if inst1.id == result[0].id:
+            self.assertTrue(result[0].deleted)
+        else:
+            self.assertTrue(result[1].deleted)
 
     def test_migration_get_all_unconfirmed(self):
         ctxt = context.get_admin_context()