Merge lp:~vishvananda/nova/fix-describe-groups into lp:~hudson-openstack/nova/trunk

[Vish Ishaya]

Fixes and optimizes filtering for describe_security_groups. Also adds a unit test.

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[Jay Pipes (jaypipes)]

Hi Vish! Could I ask why the test_describe_instances() test was removed?

review: Needs Information

[Vish Ishaya (vishvananda)]

Jay: It was a duplicate test leftover from a bad merge. There is still a (more complete) describe_instances test in there.

[Jay Pipes (jaypipes)]

gotcha. approved. thx. :)

review: Approve

[OpenStack Infra (hudson-openstack)]

Attempt to merge into lp:nova failed due to conflicts:

text conflict in nova/tests/test_cloud.py

[Jay Pipes (jaypipes)]

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~vishvananda/nova/fix-describe-groups into lp:nova failed. Below is the output from the failed tests.

AdminAPITest
    test_admin_disabled ok
    test_admin_enabled ok
APITest
    test_exceptions_are_converted_to_faults ok
Test
    test_authorize_token ok
    test_authorize_user ok
    test_bad_token ok
    test_bad_user ok
    test_no_user ok
    test_token_expiry ok
TestLimiter
    test_authorize_token ok
LimiterTest
    test_limiter_custom_max_limit ok
    test_limiter_limit_and_offset ok
    test_limiter_limit_medium ok
    test_limiter_limit_over_max ok
    test_limiter_limit_zero ok
    test_limiter_nothing ok
    test_limiter_offset_bad ok
    test_limiter_offset_blank ok
    test_limiter_offset_medium ok
    test_limiter_offset_over_max ok
    test_limiter_offset_zero ok
TestFaults
    test_fault_parts ok
    test_raise ok
    test_retry_header ok
FlavorsTest
    test_get_flavor_by_id ok
    test_get_flavor_list ok
GlanceImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
ImageControllerWithGlanceServiceTest
    test_get_image_details ok
    test_get_image_index ok
LocalImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
LimiterTest
    test_minute ok
    test_one_per_period ok
    test_second ok
    test_users_get_separate_buckets ok
    test_we_can_go_indefinitely_if_we_spread_out_requests ok
WSGIAppProxyTest
    test_200 ok
    test_403 ok
    test...

[lp:~vishvananda/nova/fix-describe-groups]

711. By Vish Ishaya on 2011-02-23

merged trunk

712. By Vish Ishaya on 2011-02-23

fix for failing describe_instances test

[Vish Ishaya (vishvananda)]

Wow it looks like removing the extra describe instances (so that the longer, correct one runs) uncovered an error in cloud.py using the direct api. It is fixed now.

Vish
On Feb 23, 2011, at 10:45 AM, OpenStack Hudson wrote:

> The proposal to merge lp:~vishvananda/nova/fix-describe-groups into lp:nova has been updated.
>
> Status: Approved => Needs review
>
> For more details, see:
> https://code.launchpad.net/~vishvananda/nova/fix-describe-groups/+merge/50851
> --
> https://code.launchpad.net/~vishvananda/nova/fix-describe-groups/+merge/50851
> You are the owner of lp:~vishvananda/nova/fix-describe-groups.

[Todd Willey (xtoddx)]

lgtm

review: Approve

[Jay Pipes (jaypipes)]

review: Approve

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2011-02-23 18:11:35 +0000
+++ nova/api/ec2/cloud.py	2011-02-23 19:30:27 +0000
@@ -318,14 +318,19 @@
 
     def describe_security_groups(self, context, group_name=None, **kwargs):
         self.compute_api.ensure_default_security_group(context)
-        if context.is_admin:
+        if group_name:
+            groups = []
+            for name in group_name:
+                group = db.security_group_get_by_name(context,
+                                                      context.project_id,
+                                                      name)
+                groups.append(group)
+        elif context.is_admin:
             groups = db.security_group_get_all(context)
         else:
             groups = db.security_group_get_by_project(context,
                                                       context.project_id)
         groups = [self._format_security_group(context, g) for g in groups]
-        if not group_name is None:
-            groups = [g for g in groups if g.name in group_name]
 
         return {'securityGroupInfo':
                 list(sorted(groups,
@@ -670,7 +675,8 @@
             instances = []
             for ec2_id in instance_id:
                 internal_id = ec2_id_to_id(ec2_id)
-                instance = self.compute_api.get(context, internal_id)
+                instance = self.compute_api.get(context,
+                                                instance_id=internal_id)
                 instances.append(instance)
         else:
             instances = self.compute_api.get_all(context, **kwargs)

=== modified file 'nova/tests/test_cloud.py'
--- nova/tests/test_cloud.py	2011-02-23 07:21:01 +0000
+++ nova/tests/test_cloud.py	2011-02-23 19:30:27 +0000
@@ -136,6 +136,22 @@
         db.instance_destroy(self.context, inst['id'])
         db.floating_ip_destroy(self.context, address)
 
+    def test_describe_security_groups(self):
+        """Makes sure describe_security_groups works and filters results."""
+        sec = db.security_group_create(self.context,
+                                       {'project_id': self.context.project_id,
+                                        'name': 'test'})
+        result = self.cloud.describe_security_groups(self.context)
+        # NOTE(vish): should have the default group as well
+        self.assertEqual(len(result['securityGroupInfo']), 2)
+        result = self.cloud.describe_security_groups(self.context,
+                      group_name=[sec['name']])
+        self.assertEqual(len(result['securityGroupInfo']), 1)
+        self.assertEqual(
+                result['securityGroupInfo'][0]['groupName'],
+                sec['name'])
+        db.security_group_destroy(self.context, sec['id'])
+
     def test_describe_volumes(self):
         """Makes sure describe_volumes works and filters results."""
         vol1 = db.volume_create(self.context, {})
@@ -294,19 +310,6 @@
                 LOG.debug(_("Terminating instance %s"), instance_id)
                 rv = self.compute.terminate_instance(instance_id)
 
-    def test_describe_instances(self):
-        """Makes sure describe_instances works."""
-        instance1 = db.instance_create(self.context, {'host': 'host2'})
-        comp1 = db.service_create(self.context, {'host': 'host2',
-                                                 'availability_zone': 'zone1',
-                                                 'topic': "compute"})
-        result = self.cloud.describe_instances(self.context)
-        self.assertEqual(result['reservationSet'][0]
-                         ['instancesSet'][0]
-                         ['placement']['availabilityZone'], 'zone1')
-        db.instance_destroy(self.context, instance1['id'])
-        db.service_destroy(self.context, comp1['id'])
-
     @staticmethod
     def _fake_set_image_description(ctxt, image_id, description):
         from nova.objectstore import handler