Merge lp:~jtran/nova/lp824008 into lp:~hudson-openstack/nova/trunk

[John Tran]

Fixed bug in which DescribeInstances was returning deleted instances. Added tests for pertinent api methods.

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[William Wolf (throughnothing)]

I think for all of the tests you added, should we have args1 and args2, with distinct values, and actually check that the filtered results are giving us the correct instance? This just seems more explicit of a test to me, rather than just checking that the length is 1 instead of 2...is it the right 1?

[lp:~jtran/nova/lp824008]

1438. By John Tran on 2011-08-16

test improvements per peer review

[John Tran (jtran)]

> I think for all of the tests you added, should we have args1 and args2, with
> distinct values, and actually check that the filtered results are giving us
> the correct instance? This just seems more explicit of a test to me, rather
> than just checking that the length is 1 instead of 2...is it the right 1?

Thanks for the feedback. I've implemented fixes as suggested.

[Rick Harris (rconradharris)]

lgtm

review: Approve

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-08-15 20:31:43 +0000
+++ nova/db/sqlalchemy/api.py	2011-08-16 16:19:48 +0000
@@ -1209,7 +1209,8 @@
                    options(joinedload('security_groups')).\
                    options(joinedload_all('fixed_ips.network')).\
                    options(joinedload('metadata')).\
-                   options(joinedload('instance_type'))
+                   options(joinedload('instance_type')).\
+                   filter_by(deleted=can_read_deleted(context))
 
     # Make a copy of the filters dictionary to use going forward, as we'll
     # be modifying it and we shouldn't affect the caller's use of it.

=== modified file 'nova/tests/test_cloud.py'
--- nova/tests/test_cloud.py	2011-08-05 05:06:22 +0000
+++ nova/tests/test_cloud.py	2011-08-16 16:19:48 +0000
@@ -487,6 +487,17 @@
         db.service_destroy(self.context, comp1['id'])
         db.service_destroy(self.context, comp2['id'])
 
+    def test_describe_instances_deleted(self):
+        args1 = {'reservation_id': 'a', 'image_ref': 1, 'host': 'host1'}
+        inst1 = db.instance_create(self.context, args1)
+        args2 = {'reservation_id': 'b', 'image_ref': 1, 'host': 'host1'}
+        inst2 = db.instance_create(self.context, args2)
+        db.instance_destroy(self.context, inst1.id)
+        result = self.cloud.describe_instances(self.context)
+        result = result['reservationSet'][0]['instancesSet']
+        self.assertEqual(result[0]['instanceId'],
+                         ec2utils.id_to_ec2_id(inst2.id))
+
     def _block_device_mapping_create(self, instance_id, mappings):
         volumes = []
         for bdm in mappings:

=== modified file 'nova/tests/test_db_api.py'
--- nova/tests/test_db_api.py	2011-08-12 19:00:48 +0000
+++ nova/tests/test_db_api.py	2011-08-16 16:19:48 +0000
@@ -76,3 +76,20 @@
         self.assertEqual(instance['id'], result['id'])
         self.assertEqual(result['fixed_ips'][0]['floating_ips'][0].address,
                          '1.2.1.2')
+
+    def test_instance_get_all_by_filters(self):
+        args = {'reservation_id': 'a', 'image_ref': 1, 'host': 'host1'}
+        inst1 = db.instance_create(self.context, args)
+        inst2 = db.instance_create(self.context, args)
+        result = db.instance_get_all_by_filters(self.context, {})
+        self.assertTrue(2, len(result))
+
+    def test_instance_get_all_by_filters_deleted(self):
+        args1 = {'reservation_id': 'a', 'image_ref': 1, 'host': 'host1'}
+        inst1 = db.instance_create(self.context, args1)
+        args2 = {'reservation_id': 'b', 'image_ref': 1, 'host': 'host1'}
+        inst2 = db.instance_create(self.context, args2)
+        db.instance_destroy(self.context, inst1.id)
+        result = db.instance_get_all_by_filters(self.context.elevated(), {})
+        self.assertEqual(1, len(result))
+        self.assertEqual(result[0].id, inst2.id)