Perform a joined query for ports and security group associations

bug 1174111

Instead of loading security group associations with a separate query
each time a port is loaded from the database, perform the load operation
with a join using a joined sqlalchemy relationship.
Also, this patch removes the need for invoking the mixin method
'extend_port_dict_security_group' from the plugin code.

Change-Id: I40b22281d45ff4f4bf8149211883799a9051c1a0

quantum/api/v2/attributes.py

@@ -451,6 +451,13 @@ def convert_to_list(data):
               'type:uuid_list': _validate_uuid_list,
               'type:values': _validate_values}
 
+# Define constants for base resource name
+NETWORK = 'network'
+NETWORKS = '%ss' % NETWORK
+PORT = 'port'
+PORTS = '%ss' % PORT
+SUBNET = 'subnet'
+SUBNETS = '%ss' % SUBNET
 # Note: a default of ATTR_NOT_SPECIFIED indicates that an
 # attribute is not required, but will be generated by the plugin
 # if it is not specified.  Particularly, a value of ATTR_NOT_SPECIFIED
@@ -475,7 +482,7 @@ def convert_to_list(data):
 # mechanism, ie: there might be rules which refer to this attribute.
 
 RESOURCE_ATTRIBUTE_MAP = {
-    'networks': {
+    NETWORKS: {
         'id': {'allow_post': False, 'allow_put': False,
                'validate': {'type:uuid': None},
                'is_visible': True,
@@ -504,7 +511,7 @@ def convert_to_list(data):
                  'required_by_policy': True,
                  'enforce_policy': True},
     },
-    'ports': {
+    PORTS: {
         'id': {'allow_post': False, 'allow_put': False,
                'validate': {'type:uuid': None},
                'is_visible': True,
@@ -546,7 +553,7 @@ def convert_to_list(data):
         'status': {'allow_post': False, 'allow_put': False,
                    'is_visible': True},
     },
-    'subnets': {
+    SUBNETS: {
         'id': {'allow_post': False, 'allow_put': False,
                'validate': {'type:uuid': None},
                'is_visible': True,
@@ -606,13 +613,13 @@ def convert_to_list(data):
 # Resources without parents, such as networks, are not in this list
 
 RESOURCE_HIERARCHY_MAP = {
-    'ports': {'parent': 'networks', 'identified_by': 'network_id'},
-    'subnets': {'parent': 'networks', 'identified_by': 'network_id'}
+    PORTS: {'parent': NETWORKS, 'identified_by': 'network_id'},
+    SUBNETS: {'parent': NETWORKS, 'identified_by': 'network_id'}
 }
 
-PLURALS = {'networks': 'network',
-           'ports': 'port',
-           'subnets': 'subnet',
+PLURALS = {NETWORKS: NETWORK,
+           PORTS: PORT,
+           SUBNETS: SUBNET,
            'dns_nameservers': 'dns_nameserver',
            'host_routes': 'host_route',
            'allocation_pools': 'allocation_pool',

quantum/db/db_base_plugin_v2.py

@@ -70,6 +70,10 @@ class method (e.g., network_created()).  The result is that this class
     # To this aim, the register_model_query_hook and unregister_query_hook
     # from this class should be invoked
     _model_query_hooks = {}
+    # This dictionary will store methods for extending attributes of
+    # api resources. Mixins can use this dict for adding their own methods
+    # TODO(salvatore-orlando): Avoid using class-level variables
+    _dict_extend_functions = {}
 
     def __init__(self):
         # NOTE(jkoelker) This is an incomlete implementation. Subclasses
@@ -117,6 +121,12 @@ def _model_query(self, context, model):
             query = query.filter(query_filter)
         return query
 
+    @classmethod
+    def register_dict_extend_funcs(cls, resource, funcs):
+        cur_funcs = cls._dict_extend_functions.get(resource, [])
+        cur_funcs.extend(funcs)
+        cls._dict_extend_functions[resource] = cur_funcs
+
     @classmethod
     def register_model_query_hook(cls, model, name, query_hook, filter_hook,
                                   result_filters=None):
@@ -142,6 +152,14 @@ def register_model_query_hook(cls, model, name, query_hook, filter_hook,
         model_hooks[name] = {'query': query_hook, 'filter': filter_hook,
                              'result_filters': result_filters}
 
+    def _filter_non_model_columns(self, data, model):
+        """Remove all the attributes from data which are not columns of
+        the model passed as second parameter.
+        """
+        columns = [c.name for c in model.__table__.columns]
+        return dict((k, v) for (k, v) in
+                    data.iteritems() if k in columns)
+
     def _get_by_id(self, context, model, id):
         query = self._model_query(context, model)
         return query.filter(model.id == id).one()
@@ -909,7 +927,8 @@ def _make_subnet_dict(self, subnet, fields=None):
                }
         return self._fields(res, fields)
 
-    def _make_port_dict(self, port, fields=None):
+    def _make_port_dict(self, port, fields=None,
+                        process_extensions=True):
         res = {"id": port["id"],
                'name': port['name'],
                "network_id": port["network_id"],
@@ -922,6 +941,11 @@ def _make_port_dict(self, port, fields=None):
                              for ip in port["fixed_ips"]],
                "device_id": port["device_id"],
                "device_owner": port["device_owner"]}
+        # Call auxiliary extend functions, if any
+        if process_extensions:
+            for func in self._dict_extend_functions.get(attributes.PORTS,
+                                                        []):
+                func(self, res, port)
         return self._fields(res, fields)
 
     def _create_bulk(self, resource, context, request_items):
@@ -1331,7 +1355,7 @@ def create_port(self, context, port):
                     )
                     context.session.add(allocated)
 
-        return self._make_port_dict(port)
+        return self._make_port_dict(port, process_extensions=False)
 
     def update_port(self, context, id, port):
         p = port['port']
@@ -1342,24 +1366,22 @@ def update_port(self, context, id, port):
             if 'fixed_ips' in p:
                 self._recycle_expired_ip_allocations(context,
                                                      port['network_id'])
-                original = self._make_port_dict(port)
+                original = self._make_port_dict(port, process_extensions=False)
                 ips = self._update_ips_for_port(context,
                                                 port["network_id"],
                                                 id,
                                                 original["fixed_ips"],
                                                 p['fixed_ips'])
-                # 'fixed_ip's not part of DB so it is deleted
-                del p['fixed_ips']
-
                 # Update ips if necessary
                 for ip in ips:
                     allocated = models_v2.IPAllocation(
                         network_id=port['network_id'], port_id=port.id,
                         ip_address=ip['ip_address'], subnet_id=ip['subnet_id'],
                         expiration=self._default_allocation_expiration())
                     context.session.add(allocated)
-
-            port.update(p)
+        # Remove all attributes in p which are not in the port DB model
+        # and then update the port
+        port.update(self._filter_non_model_columns(p, models_v2.Port))
 
         return self._make_port_dict(port)
 

quantum/db/securitygroups_db.py

@@ -22,6 +22,7 @@
 from sqlalchemy.orm import scoped_session
 
 from quantum.api.v2 import attributes as attr
+from quantum.db import db_base_plugin_v2
 from quantum.db import model_base
 from quantum.db import models_v2
 from quantum.extensions import securitygroup as ext_sg
@@ -46,6 +47,13 @@ class SecurityGroupPortBinding(model_base.BASEV2):
                                   sa.ForeignKey("securitygroups.id"),
                                   primary_key=True)
 
+    # Add a relationship to the Port model in order to instruct SQLAlchemy to
+    # eagerly load security group bindings
+    ports = orm.relationship(
+        models_v2.Port,
+        backref=orm.backref("security_groups",
+                            lazy='joined', cascade='delete'))
+
 
 class SecurityGroupRule(model_base.BASEV2, models_v2.HasId,
                         models_v2.HasTenant):
@@ -385,25 +393,29 @@ def delete_security_group_rule(self, context, id):
             rule = self._get_security_group_rule(context, id)
             context.session.delete(rule)
 
-    def _extend_port_dict_security_group(self, context, port):
-        filters = {'port_id': [port['id']]}
-        fields = {'security_group_id': None}
-        security_group_id = self._get_port_security_group_bindings(
-            context, filters, fields)
-
-        port[ext_sg.SECURITYGROUPS] = []
-        for security_group_id in security_group_id:
-            port[ext_sg.SECURITYGROUPS].append(
-                security_group_id['security_group_id'])
-        return port
-
-    def _process_port_create_security_group(self, context, port_id,
-                                            security_group_id):
-        if not attr.is_attr_set(security_group_id):
-            return
-        for security_group_id in security_group_id:
-            self._create_port_security_group_binding(context, port_id,
-                                                     security_group_id)
+    def _extend_port_dict_security_group(self, port_res, port_db):
+        # If port_db is provided, security groups will be accessed via
+        # sqlalchemy models. As they're loaded together with ports this
+        # will not cause an extra query.
+        security_group_ids = [sec_group_mapping['security_group_id'] for
+                              sec_group_mapping in port_db.security_groups]
+        port_res[ext_sg.SECURITYGROUPS] = security_group_ids
+        return port_res
+
+    # Register dict extend functions for ports
+    db_base_plugin_v2.QuantumDbPluginV2.register_dict_extend_funcs(
+        attr.PORTS, [_extend_port_dict_security_group])
+
+    def _process_port_create_security_group(self, context, port,
+                                            security_group_ids):
+        if attr.is_attr_set(security_group_ids):
+            for security_group_id in security_group_ids:
+                self._create_port_security_group_binding(context, port['id'],
+                                                         security_group_id)
+        # Convert to list as a set might be passed here and
+        # this has to be serialized
+        port[ext_sg.SECURITYGROUPS] = (security_group_ids and
+                                       list(security_group_ids) or [])
 
     def _ensure_default_security_group(self, context, tenant_id):
         """Create a default security group if one doesn't exist.

quantum/db/securitygroups_rpc_base.py

@@ -77,10 +77,12 @@ def update_security_group_on_port(self, context, id, port,
             self._delete_port_security_group_bindings(context, id)
             self._process_port_create_security_group(
                 context,
-                id,
+                updated_port,
                 port['port'][ext_sg.SECURITYGROUPS])
             need_notify = True
-        self._extend_port_dict_security_group(context, updated_port)
+        else:
+            updated_port[ext_sg.SECURITYGROUPS] = (
+                original_port[ext_sg.SECURITYGROUPS])
         return need_notify
 
     def is_security_group_member_updated(self, context,

quantum/plugins/brocade/QuantumPlugin.py

@@ -382,15 +382,16 @@ def update_port(self, context, port_id, port):
                 port['port'][ext_sg.SECURITYGROUPS] = (
                     self._get_security_groups_on_port(context, port))
                 self._delete_port_security_group_bindings(context, port_id)
+                # process_port_create_security_group also needs port id
+                port['port']['id'] = port_id
                 self._process_port_create_security_group(
                     context,
-                    port_id,
+                    port['port'],
                     port['port'][ext_sg.SECURITYGROUPS])
                 port_updated = True
 
             port = super(BrocadePluginV2, self).update_port(
                 context, port_id, port)
-            self._extend_port_dict_security_group(context, port)
 
         if original_port['admin_state_up'] != port['admin_state_up']:
             port_updated = True
@@ -411,7 +412,6 @@ def get_port(self, context, port_id, fields=None):
         with context.session.begin(subtransactions=True):
             port = super(BrocadePluginV2, self).get_port(
                 context, port_id, fields)
-            self._extend_port_dict_security_group(context, port)
             self._extend_port_dict_binding(context, port)
 
         return self._fields(port, fields)
@@ -423,7 +423,6 @@ def get_ports(self, context, filters=None, fields=None):
                                                            filters,
                                                            fields)
             for port in ports:
-                self._extend_port_dict_security_group(context, port)
                 self._extend_port_dict_binding(context, port)
                 res_ports.append(self._fields(port, fields))
 

quantum/plugins/linuxbridge/lb_quantum_plugin.py

@@ -471,7 +471,6 @@ def get_port(self, context, id, fields=None):
             port = super(LinuxBridgePluginV2, self).get_port(context,
                                                              id,
                                                              fields)
-            self._extend_port_dict_security_group(context, port)
         self._extend_port_dict_binding(context, port),
         return self._fields(port, fields)
 
@@ -484,7 +483,6 @@ def get_ports(self, context, filters=None, fields=None,
                                           limit, marker, page_reverse)
             #TODO(nati) filter by security group
             for port in ports:
-                self._extend_port_dict_security_group(context, port)
                 self._extend_port_dict_binding(context, port)
                 res_ports.append(self._fields(port, fields))
         return res_ports
@@ -500,8 +498,7 @@ def create_port(self, context, port):
             port = super(LinuxBridgePluginV2,
                          self).create_port(context, port)
             self._process_port_create_security_group(
-                context, port['id'], sgids)
-            self._extend_port_dict_security_group(context, port)
+                context, port, sgids)
         self.notify_security_groups_member_updated(context, port)
         return self._extend_port_dict_binding(context, port)
 

quantum/plugins/midonet/plugin.py

@@ -422,7 +422,9 @@ def create_port(self, context, port):
         with session.begin(subtransactions=True):
             port_db_entry = super(MidonetPluginV2,
                                   self).create_port(context, port)
-            self._extend_port_dict_security_group(context, port_db_entry)
+            # Caveat: port_db_entry is not a db model instance
+            sg_ids = self._get_security_groups_on_port(context, port)
+            self._process_port_create_security_group(context, port, sg_ids)
             if is_compute_interface:
                 # Create a DHCP entry if needed.
                 if 'ip_address' in (port_db_entry['fixed_ips'] or [{}])[0]:
@@ -453,8 +455,6 @@ def get_port(self, context, id, fields=None):
         # get the quantum port from DB.
         port_db_entry = super(MidonetPluginV2, self).get_port(context,
                                                               id, fields)
-        self._extend_port_dict_security_group(context, port_db_entry)
-
         # verify that corresponding port exists in MidoNet.
         try:
             self.mido_api.get_port(id)
@@ -477,7 +477,6 @@ def get_ports(self, context, filters=None, fields=None):
             try:
                 for port in ports_db_entry:
                     self.mido_api.get_port(port['id'])
-                    self._extend_port_dict_security_group(context, port)
             except w_exc.HTTPNotFound:
                 raise MidonetResourceNotFound(resource_type='Port',
                                               id=port['id'])

quantum/plugins/nec/nec_plugin.py

@@ -380,8 +380,7 @@ def create_port(self, context, port):
             sgids = self._get_security_groups_on_port(context, port)
             port = super(NECPluginV2, self).create_port(context, port)
             self._process_port_create_security_group(
-                context, port['id'], sgids)
-            self._extend_port_dict_security_group(context, port)
+                context, port, sgids)
         self.notify_security_groups_member_updated(context, port)
         self._update_resource_status(context, "port", port['id'],
                                      OperationalStatus.BUILD)
@@ -416,9 +415,6 @@ def update_port(self, context, id, port):
             else:
                 self.deactivate_port(context, old_port)
 
-        # NOTE: _extend_port_dict_security_group() is called in
-        # update_security_group_on_port() above, so we don't need to
-        # call it here.
         return self._extend_port_dict_binding(context, new_port)
 
     def delete_port(self, context, id, l3_port_check=True):
@@ -452,7 +448,6 @@ def delete_port(self, context, id, l3_port_check=True):
     def get_port(self, context, id, fields=None):
         with context.session.begin(subtransactions=True):
             port = super(NECPluginV2, self).get_port(context, id, fields)
-            self._extend_port_dict_security_group(context, port)
             self._extend_port_dict_binding(context, port)
         return self._fields(port, fields)
 
@@ -462,7 +457,6 @@ def get_ports(self, context, filters=None, fields=None):
                                                        fields)
             # TODO(amotoki) filter by security group
             for port in ports:
-                self._extend_port_dict_security_group(context, port)
                 self._extend_port_dict_binding(context, port)
         return [self._fields(port, fields) for port in ports]