Merge lp:~cbehrens/nova/lp854239 into lp:~hudson-openstack/nova/trunk

[Chris Behrens]

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.

[Chris Behrens (cbehrens)]

Merged trunk and resolved conflict.

[Josh Kearney (jk0)]

These all look like reasonable changes to me.

review: Approve

[Kevin L. Mitchell (klmitch)]

LGTM...

review: Approve

[Jason Kölker (jason-koelker)]

Is mas Bueno!

review: Approve

[Devin Carlen (devcamcar)]

lgtm!

review: Approve

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-09-19 14:35:08 +0000
+++ nova/compute/api.py	2011-09-19 22:43:17 +0000
@@ -937,9 +937,10 @@
                                                                    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):

=== modified file 'nova/db/api.py'
--- nova/db/api.py	2011-09-19 21:02:29 +0000
+++ nova/db/api.py	2011-09-19 22:43:17 +0000
@@ -468,7 +468,7 @@
 
 
 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 @@
     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 @@
     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)
 
 
 ###################

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-09-19 21:02:29 +0000
+++ nova/db/sqlalchemy/api.py	2011-09-19 22:43:17 +0000
@@ -1204,7 +1204,7 @@
 
 
 @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 @@
         """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 @@
     # 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 @@
         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 @@
             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 @@
 
 
 @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
 
 
 ###################

=== modified file 'nova/network/manager.py'
--- nova/network/manager.py	2011-09-19 20:16:49 +0000
+++ nova/network/manager.py	2011-09-19 22:43:17 +0000
@@ -410,7 +410,7 @@
         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 @@
 
         # 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,

=== modified file 'nova/tests/fake_network.py'
--- nova/tests/fake_network.py	2011-09-15 19:25:52 +0000
+++ nova/tests/fake_network.py	2011-09-19 22:43:17 +0000
@@ -105,10 +105,12 @@
                                     '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()

=== modified file 'nova/tests/test_compute.py'
--- nova/tests/test_compute.py	2011-09-19 14:35:08 +0000
+++ nova/tests/test_compute.py	2011-09-19 22:43:17 +0000
@@ -1033,8 +1033,8 @@
                        '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})

=== modified file 'nova/tests/test_db_api.py'
--- nova/tests/test_db_api.py	2011-09-19 21:02:29 +0000
+++ nova/tests/test_db_api.py	2011-09-19 22:43:17 +0000
@@ -94,9 +94,12 @@
         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()