Refactor db.security_group_get() instance join behavior

Currently, security_group['instances'] is always joined when we
do a db.security_group_get(). This patch adds the growingly-
conventional columns_to_join argument to the call, allowing
us to selectively choose when we want to make this expensive
join operation.

Related to unified-object-model

Change-Id: I1945365c8cf03c027fd143f677cbd6d63a7f8122

nova/compute/api.py

@@ -3216,7 +3216,8 @@ def validate_id(self, id):
     def trigger_rules_refresh(self, context, id):
         """Called when a rule is added to or removed from a security_group."""
 
-        security_group = self.db.security_group_get(context, id)
+        security_group = self.db.security_group_get(
+            context, id, columns_to_join=['instances'])
 
         for instance in security_group['instances']:
             if instance['host'] is not None:
@@ -3242,8 +3243,8 @@ def trigger_members_refresh(self, context, group_ids):
         security_groups = set()
         for rule in security_group_rules:
             security_group = self.db.security_group_get(
-                                                    context,
-                                                    rule['parent_group_id'])
+                context, rule['parent_group_id'],
+                columns_to_join=['instances'])
             security_groups.add(security_group)
 
         # ..then we find the instances that are members of these groups..

nova/db/api.py

@@ -1151,9 +1151,10 @@ def security_group_get_all(context):
     return IMPL.security_group_get_all(context)
 
 
-def security_group_get(context, security_group_id):
+def security_group_get(context, security_group_id, columns_to_join=None):
     """Get security group by its id."""
-    return IMPL.security_group_get(context, security_group_id)
+    return IMPL.security_group_get(context, security_group_id,
+                                   columns_to_join)
 
 
 def security_group_get_by_name(context, project_id, group_name):

nova/db/sqlalchemy/api.py

@@ -3241,13 +3241,17 @@ def security_group_get_all(context):
 
 
 @require_context
-def security_group_get(context, security_group_id, session=None):
-    result = _security_group_get_query(context, session=session,
-                                       project_only=True).\
-                    filter_by(id=security_group_id).\
-                    options(joinedload_all('instances')).\
-                    first()
+def security_group_get(context, security_group_id, columns_to_join=None,
+                       session=None):
+    query = _security_group_get_query(context, session=session,
+                                      project_only=True).\
+                filter_by(id=security_group_id)
+    if columns_to_join is None:
+        columns_to_join = []
+    if 'instances' in columns_to_join:
+        query = query.options(joinedload_all('instances'))
 
+    result = query.first()
     if not result:
         raise exception.SecurityGroupNotFound(
                 security_group_id=security_group_id)

nova/tests/api/openstack/compute/contrib/test_security_groups.py

@@ -716,7 +716,7 @@ def setUp(self):
         db1 = security_group_db(self.sg1)
         db2 = security_group_db(self.sg2)
 
-        def return_security_group(context, group_id):
+        def return_security_group(context, group_id, columns_to_join=None):
             if group_id == db1['id']:
                 return db1
             if group_id == db2['id']:

nova/tests/db/test_db_api.py

@@ -30,6 +30,7 @@
 from sqlalchemy import exc
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy import MetaData
+from sqlalchemy.orm import exc as sqlalchemy_orm_exc
 from sqlalchemy.orm import query
 from sqlalchemy.sql.expression import select
 
@@ -1620,19 +1621,40 @@ def test_security_group_destroy(self):
         self.assertRaises(exception.SecurityGroupNotFound,
                           db.security_group_get,
                           self.ctxt, security_group1['id'])
-        self._assertEqualObjects(db.security_group_get(self.ctxt,
-                                                       security_group2['id']),
-                                 security_group2)
+        self._assertEqualObjects(db.security_group_get(
+                self.ctxt, security_group2['id'],
+                columns_to_join=['instances']), security_group2)
 
     def test_security_group_get(self):
         security_group1 = self._create_security_group({})
         security_group2 = self._create_security_group(
                 {'name': 'fake_sec_group2'})
         real_security_group = db.security_group_get(self.ctxt,
-                                              security_group1['id'])
+                                              security_group1['id'],
+                                              columns_to_join=['instances'])
         self._assertEqualObjects(security_group1,
                                  real_security_group)
 
+    def test_security_group_get_no_instances(self):
+        instance = db.instance_create(self.ctxt, {})
+        sid = self._create_security_group({'instances': [instance]})['id']
+
+        session = get_session()
+        self.mox.StubOutWithMock(sqlalchemy_api, 'get_session')
+        sqlalchemy_api.get_session().AndReturn(session)
+        sqlalchemy_api.get_session().AndReturn(session)
+        self.mox.ReplayAll()
+
+        security_group = db.security_group_get(self.ctxt, sid,
+                                               columns_to_join=['instances'])
+        session.expunge(security_group)
+        self.assertEqual(1, len(security_group['instances']))
+
+        security_group = db.security_group_get(self.ctxt, sid)
+        session.expunge(security_group)
+        self.assertRaises(sqlalchemy_orm_exc.DetachedInstanceError,
+                          getattr, security_group, 'instances')
+
     def test_security_group_get_not_found_exception(self):
         self.assertRaises(exception.SecurityGroupNotFound,
                           db.security_group_get, self.ctxt, 100500)