Merge "Convert security_group calls to use instance objs"

4 files changed, 40 insertions, 28 deletions

diff --git a/nova/api/openstack/contrib/security_groups.py b/nova/api/openstack/contrib/security_groups.py
index bb4cd48b2..fd1187629 100644
--- a/nova/api/openstack/contrib/security_groups.py
+++ b/nova/api/openstack/contrib/security_groups.py
@@ -387,8 +387,8 @@ class Security_groups(extensions.ExtensionDescriptor):
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
         try:
-            self.compute_api.add_security_group(context, instance_id,
-                                                group_name)
+            instance = self.compute_api.get(context, instance_id)
+            self.compute_api.add_security_group(context, instance, group_name)
         except exception.SecurityGroupNotFound as exp:
             raise exc.HTTPNotFound(explanation=unicode(exp))
         except exception.InstanceNotFound as exp:
@@ -416,7 +416,8 @@ class Security_groups(extensions.ExtensionDescriptor):
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
         try:
-            self.compute_api.remove_security_group(context, instance_id,
+            instance = self.compute_api.get(context, instance_id)
+            self.compute_api.remove_security_group(context, instance,
                                                    group_name)
         except exception.SecurityGroupNotFound as exp:
             raise exc.HTTPNotFound(explanation=unicode(exp))
diff --git a/nova/compute/api.py b/nova/compute/api.py
index b57435716..aabc4838c 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -690,63 +690,59 @@ class API(base.Base):
 
         return False
 
-    def add_security_group(self, context, instance_id, security_group_name):
+    def add_security_group(self, context, instance, security_group_name):
         """Add security group to the instance"""
         security_group = self.db.security_group_get_by_name(context,
                 context.project_id,
                 security_group_name)
-        # check if the server exists
-        if utils.is_uuid_like(instance_id):
-            inst = self.db.instance_get_by_uuid(context, instance_id)
-        else:
-            inst = self.db.instance_get(context, instance_id)
-        instance_id = inst['id']
+
+        instance_id = instance['id']
+
         #check if the security group is associated with the server
         if self._is_security_group_associated_with_server(security_group,
-                                                        instance_id):
+                                                          instance_id):
             raise exception.SecurityGroupExistsForInstance(
                                         security_group_id=security_group['id'],
                                         instance_id=instance_id)
 
         #check if the instance is in running state
-        if inst['state'] != power_state.RUNNING:
+        if instance['power_state'] != power_state.RUNNING:
             raise exception.InstanceNotRunning(instance_id=instance_id)
 
         self.db.instance_add_security_group(context.elevated(),
                                             instance_id,
                                             security_group['id'])
+        host = instance['host']
         rpc.cast(context,
-             self.db.queue_get_for(context, FLAGS.compute_topic, inst['host']),
+             self.db.queue_get_for(context, FLAGS.compute_topic, host),
              {"method": "refresh_security_group_rules",
               "args": {"security_group_id": security_group['id']}})
 
-    def remove_security_group(self, context, instance_id, security_group_name):
+    def remove_security_group(self, context, instance, security_group_name):
         """Remove the security group associated with the instance"""
         security_group = self.db.security_group_get_by_name(context,
                 context.project_id,
                 security_group_name)
-        # check if the server exists
-        if utils.is_uuid_like(instance_id):
-            inst = self.db.instance_get_by_uuid(context, instance_id)
-        else:
-            inst = self.db.instance_get(context, instance_id)
-        instance_id = inst['id']
+
+        instance_id = instance['id']
+
         #check if the security group is associated with the server
         if not self._is_security_group_associated_with_server(security_group,
-                                                        instance_id):
+                                                              instance_id):
             raise exception.SecurityGroupNotExistsForInstance(
                                     security_group_id=security_group['id'],
                                     instance_id=instance_id)
 
         #check if the instance is in running state
-        if inst['state'] != power_state.RUNNING:
+        if instance['power_state'] != power_state.RUNNING:
             raise exception.InstanceNotRunning(instance_id=instance_id)
 
         self.db.instance_remove_security_group(context.elevated(),
                                                instance_id,
                                                security_group['id'])
+        host = instance['host']
         rpc.cast(context,
-             self.db.queue_get_for(context, FLAGS.compute_topic, inst['host']),
+             self.db.queue_get_for(context, FLAGS.compute_topic, host),
              {"method": "refresh_security_group_rules",
               "args": {"security_group_id": security_group['id']}})
 
diff --git a/nova/tests/api/openstack/contrib/test_security_groups.py b/nova/tests/api/openstack/contrib/test_security_groups.py
index b3e1507e0..1e7a439b0 100644
--- a/nova/tests/api/openstack/contrib/test_security_groups.py
+++ b/nova/tests/api/openstack/contrib/test_security_groups.py
@@ -72,20 +72,23 @@ def security_group_rule_db(rule, id=None):
 
 def return_server(context, server_id):
     return {'id': int(server_id),
-            'state': 0x01,
+            'power_state': 0x01,
             'host': "localhost",
-            'uuid': FAKE_UUID}
+            'uuid': FAKE_UUID,
+            'name': 'asdf'}
 
 
 def return_server_by_uuid(context, server_uuid):
     return {'id': 1,
-            'state': 0x01,
+            'power_state': 0x01,
             'host': "localhost",
-            'uuid': server_uuid}
+            'uuid': server_uuid,
+            'name': 'asdf'}
 
 
 def return_non_running_server(context, server_id):
-    return {'id': server_id, 'state': 0x02, 'host': "localhost"}
+    return {'id': server_id, 'power_state': 0x02,
+            'host': "localhost", 'name': 'asdf'}
 
 
 def return_security_group_by_name(context, project_id, group_name):
diff --git a/nova/tests/test_compute.py b/nova/tests/test_compute.py
index 0dd6c14b9..69b998d68 100644
--- a/nova/tests/test_compute.py
+++ b/nova/tests/test_compute.py
@@ -2094,6 +2094,18 @@ class ComputeAPITestCase(BaseTestCase):
         self.assertFalse(self.compute_api.get_lock(self.context, instance))
         db.instance_update(self.context, instance_id, {'locked': True})
         self.assertTrue(self.compute_api.get_lock(self.context, instance))
+
+    def test_add_remove_security_group(self):
+        instance_id = self._create_instance()
+        self.compute.run_instance(self.context, instance_id)
+        instance = self.compute_api.get(self.context, instance_id)
+        security_group_name = self._create_group()['name']
+        self.compute_api.add_security_group(self.context,
+                                            instance,
+                                            security_group_name)
+        self.compute_api.remove_security_group(self.context,
+                                               instance,
+                                               security_group_name)
         self.compute_api.delete(self.context, instance)
 
     def test_inject_file(self):