Merge "[OVN] Implement GW IP network QoS inheritance"

neutron/common/utils.py

@@ -38,6 +38,7 @@ from neutron_lib.api.definitions import availability_zone as az_def
 from neutron_lib import constants as n_const
 from neutron_lib import context as n_context
 from neutron_lib.db import api as db_api
+from neutron_lib.services.qos import constants as qos_consts
 from neutron_lib.services.trunk import constants as trunk_constants
 from neutron_lib.utils import helpers
 from oslo_config import cfg
@@ -1046,3 +1047,15 @@ def is_session_active(session):
     if not (session.dirty or session.deleted or session.new):
         return False
     return True
+
+
+def effective_qos_policy_id(resource):
+    """Return the resource effective QoS policy
+
+    If the resource does not have any QoS policy reference, returns the
+    QoS policy inherited from the network (if exists).
+    :param resource: [dict] resource with QoS policies
+    :return: [str] resource QoS policy ID or network QoS policy ID
+    """
+    return (resource.get(qos_consts.QOS_POLICY_ID) or
+            resource.get(qos_consts.QOS_NETWORK_POLICY_ID))

neutron/db/l3_gateway_ip_qos.py

@@ -39,11 +39,17 @@ class L3_gw_ip_qos_dbonly_mixin(l3_gwmode_db.L3_NAT_dbonly_mixin):
     @staticmethod
     @resource_extend.extends([l3_apidef.ROUTERS])
     def _extend_router_dict_gw_qos(router_res, router_db):
-        if router_db.gw_port_id and router_db.get('qos_policy_binding'):
-            policy_id = router_db.qos_policy_binding.policy_id
-            router_res[l3_apidef.EXTERNAL_GW_INFO].update(
-                {qos_consts.QOS_POLICY_ID: policy_id})
+        if router_db.gw_port and (
+                router_db.qos_policy_binding or
+                router_db.gw_port.qos_network_policy_binding):
+            qos_bind = router_db.qos_policy_binding
+            qos_net_bind = router_db.gw_port.qos_network_policy_binding
+            policy_id = qos_bind.policy_id if qos_bind else None
+            net_policy_id = qos_net_bind.policy_id if qos_net_bind else None
+            router_res[l3_apidef.EXTERNAL_GW_INFO].update({
+                qos_consts.QOS_POLICY_ID: policy_id})
             router_res[qos_consts.QOS_POLICY_ID] = policy_id
+            router_res[qos_consts.QOS_NETWORK_POLICY_ID] = net_policy_id
 
     @property
     def _is_gw_ip_qos_supported(self):

neutron/objects/qos/binding.py

@@ -141,3 +141,26 @@ class QosPolicyRouterGatewayIPBinding(base.NeutronDbObject,
     primary_keys = ['policy_id', 'router_id']
     fields_no_update = ['policy_id', 'router_id']
     _bound_model_id = db_model.router_id
+
+    @classmethod
+    @db_api.CONTEXT_READER
+    def get_routers_by_network_id(cls, context, network_id, policy_id=None):
+        """Return the routers that have a network as GW, filtered by QoS policy
+
+        This method returns the routers that have a gateway port on this
+        network. If "policy_id" is defined, it will return those routers that
+        have a gateway IP QoS policy associated. If "policy_id" is None, this
+        method will return only those routers that doesn't have any gateway
+        IP QoS policy associated.
+        """
+        query = context.session.query(models_l3.Router).filter(
+            models_l3.Router.gw_port_id == models_v2.Port.id,
+            models_v2.Port.network_id == network_id)
+        if policy_id:
+            query = query.filter(exists().where(and_(
+                cls.db_model.router_id == models_l3.Router.id,
+                cls.db_model.policy_id == policy_id)))
+        else:
+            query = query.filter(~exists().where(
+                cls.db_model.router_id == models_l3.Router.id))
+        return query.all()

neutron/objects/qos/policy.py

@@ -337,7 +337,7 @@ class QosPolicy(rbac_db.NeutronRbacObject):
             self.obj_context, self.id)
 
     def get_bound_routers(self):
-        return binding.QosPolicyRouterGatewayIPBinding.get_objects(
+        return binding.QosPolicyRouterGatewayIPBinding.get_bound_ids(
             self.obj_context, policy_id=self.id)
 
     @classmethod

neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/qos.py

@@ -25,6 +25,7 @@ from oslo_log import log as logging
 
 from neutron.common.ovn import constants as ovn_const
 from neutron.common.ovn import utils
+from neutron.common import utils as n_utils
 from neutron.conf.plugins.ml2.drivers.ovn import ovn_conf
 
 
@@ -199,9 +200,8 @@ class OVNClientQosExtension(object):
         If the port does not have any QoS policy reference or is a network
         device, then return None.
         """
-        policy_exists = bool(port.get('qos_policy_id') or
-                             port.get('qos_network_policy_id'))
-        if not policy_exists or utils.is_network_device_port(port):
+        policy_id = n_utils.effective_qos_policy_id(port)
+        if not policy_id or utils.is_network_device_port(port):
             return None, None
 
         if port.get('qos_policy_id'):
@@ -282,17 +282,18 @@ class OVNClientQosExtension(object):
                        qos_rules=None):
         updated_port_ids = set([])
         updated_fip_ids = set([])
+        updated_router_ids = set([])
         if not reset and not original_network:
             # If there is no information about the previous QoS policy, do not
             # make any change.
-            return updated_port_ids, updated_fip_ids
+            return updated_port_ids, updated_fip_ids, updated_router_ids
 
         qos_policy_id = network.get('qos_policy_id')
         if not reset:
             original_qos_policy_id = original_network.get('qos_policy_id')
             if qos_policy_id == original_qos_policy_id:
                 # No QoS policy change
-                return updated_port_ids, updated_fip_ids
+                return updated_port_ids, updated_fip_ids, updated_router_ids
 
         # NOTE(ralonsoh): we don't use the transaction context because some
         # ports can belong to other projects.
@@ -315,7 +316,13 @@ class OVNClientQosExtension(object):
             self.update_floatingip(txn, floatingip)
             updated_fip_ids.add(floatingip['id'])
 
-        return updated_port_ids, updated_fip_ids
+        for router in (qos_binding.QosPolicyRouterGatewayIPBinding.
+                get_routers_by_network_id(admin_context, network['id'])):
+            router_dict = self._plugin_l3._make_router_dict(router)
+            self.update_router(txn, router_dict)
+            updated_router_ids.add(router.id)
+
+        return updated_port_ids, updated_fip_ids, updated_router_ids
 
     def _delete_fip_qos_rules(self, txn, fip_id, network_id):
         if network_id:
@@ -385,7 +392,7 @@ class OVNClientQosExtension(object):
 
     def update_router(self, txn, router):
         gw_info = router.get(l3_api.EXTERNAL_GW_INFO) or {}
-        qos_policy_id = router.get('qos_policy_id')
+        qos_policy_id = n_utils.effective_qos_policy_id(router)
         router_id = router.get('id')
         gw_port_id = router.get('gw_port_id')
         gw_network_id = gw_info.get('network_id')
@@ -393,7 +400,8 @@ class OVNClientQosExtension(object):
             # NOTE(ralonsoh): when the gateway network is detached, the gateway
             # port is deleted. Any QoS policy related to this port_id is
             # deleted in "self.update_port()".
-            LOG.debug('Router %s does not have ID or gateway assigned', router)
+            LOG.debug('Router %s does not have ID or gateway assigned',
+                      router_id)
             return
 
         admin_context = n_context.get_admin_context()
@@ -419,9 +427,11 @@ class OVNClientQosExtension(object):
     def update_policy(self, context, policy):
         updated_port_ids = set([])
         updated_fip_ids = set([])
+        updated_router_ids = set([])
         bound_networks = policy.get_bound_networks()
         bound_ports = policy.get_bound_ports()
         bound_fips = policy.get_bound_floatingips()
+        bound_routers = policy.get_bound_routers()
         qos_rules = self._qos_rules(context, policy.id)
         # TODO(ralonsoh): we need to benchmark this transaction in systems with
         # a huge amount of ports. This can take a while and could block other
@@ -429,10 +439,11 @@ class OVNClientQosExtension(object):
         with self.nb_idl.transaction(check_error=True) as txn:
             for network_id in bound_networks:
                 network = {'qos_policy_id': policy.id, 'id': network_id}
-                port_ids, fip_ids = self.update_network(
+                port_ids, fip_ids, router_ids = self.update_network(
                     txn, network, {}, reset=True, qos_rules=qos_rules)
                 updated_port_ids.update(port_ids)
                 updated_fip_ids.update(fip_ids)
+                updated_router_ids.update(router_ids)
 
             # Update each port bound to this policy, not handled previously in
             # the network update loop
@@ -451,7 +462,9 @@ class OVNClientQosExtension(object):
                         context, filters={'id': fip_ids}):
                     self.update_floatingip(txn, fip)
 
-            for router_binding in policy.get_bound_routers():
-                router = self._plugin_l3.get_router(context,
-                                                    router_binding.router_id)
-                self.update_router(txn, router)
+            router_ids = [r for r in bound_routers if
+                          r not in updated_router_ids]
+            if router_ids:
+                for router in self._plugin_l3.get_routers(
+                        context, filters={'id': router_ids}):
+                    self.update_router(txn, router)

neutron/tests/unit/extensions/test_l3_ext_gw_mode.py

@@ -471,7 +471,7 @@ class ExtGwModeIntTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase,
                              external_gateway_info=input_value) as router:
                 res = self._show('routers', router['router']['id'])
                 for k, v in expected_value:
-                    self.assertEqual(res['router'][k], v)
+                    self.assertEqual(v, res['router'][k])
 
     def test_router_create_show_ext_gwinfo_default(self):
         self._test_router_create_show_ext_gwinfo(None, True)

neutron/tests/unit/objects/qos/test_binding.py

@@ -11,6 +11,7 @@
 #    under the License.
 
 from neutron.objects.qos import binding
+from neutron.objects import router
 from neutron.tests.unit.objects import test_base
 from neutron.tests.unit import testlib_api
 
@@ -102,3 +103,76 @@ class QosPolicyRouterGatewayIPBindingDbObjectTestCase(
         for db_obj in self.db_objs:
             self._create_test_qos_policy(id=db_obj['policy_id'])
             self._create_test_router_id(router_id=db_obj['router_id'])
+
+    def test_get_routers_by_network_id(self):
+        qos_policy_router_obj = self._create_test_qos_policy()
+        qos_policy_net_obj = self._create_test_qos_policy()
+        # External network 1, no QoS policy
+        ext_network_id_1 = self._create_external_network_id()
+        gw_port_id_1a = self._create_test_port_id(network_id=ext_network_id_1)
+        gw_port_id_1b = self._create_test_port_id(network_id=ext_network_id_1)
+        # External network 2, "qos_policy_network" assigned
+        ext_network_id_2 = self._create_external_network_id(
+            qos_policy_id=qos_policy_net_obj.id)
+        gw_port_id_2a = self._create_test_port_id(network_id=ext_network_id_2)
+        gw_port_id_2b = self._create_test_port_id(network_id=ext_network_id_2)
+
+        # Router 1: no GW
+        self._create_test_router_id(name='router1')
+
+        # Router 2: GW assigned, no router QoS, not public network QoS
+        router2 = self._create_test_router_id(name='router2')
+        router2_obj = router.Router.get_object(self.context, id=router2)
+        router2_obj.gw_port_id = gw_port_id_1a
+        router2_obj.update()
+
+        # Router 3: GW assigned, router QoS, not public network QoS
+        router3 = self._create_test_router_id(name='router3')
+        router3_obj = router.Router.get_object(self.context, id=router3)
+        router3_obj.gw_port_id = gw_port_id_1b
+        router3_obj.qos_policy_id = qos_policy_router_obj.id
+        router3_obj.update()
+
+        # Router 4: GW assigned, no router QoS, public network with QoS
+        router4 = self._create_test_router_id(name='router4')
+        router4_obj = router.Router.get_object(self.context, id=router4)
+        router4_obj.gw_port_id = gw_port_id_2a
+        router4_obj.update()
+
+        # Router 5: GW assigned, router QoS, public network with QoS
+        router5 = self._create_test_router_id(name='router5')
+        router5_obj = router.Router.get_object(self.context, id=router5)
+        router5_obj.gw_port_id = gw_port_id_2b
+        router5_obj.qos_policy_id = qos_policy_router_obj.id
+        router5_obj.update()
+
+        # Check that only router3 and router5 have
+        # "QosPolicyRouterGatewayIPBinding" related registers.
+        qos_gw_binds = self._test_class.get_objects(self.context)
+        self.assertEqual(2, len(qos_gw_binds))
+        router_ids = [qos_gw_bind.router_id for qos_gw_bind in qos_gw_binds]
+        self.assertEqual(sorted([router3, router5]), sorted(router_ids))
+
+        result = self._test_class.get_routers_by_network_id(
+            self.context, ext_network_id_1, policy_id=None)
+        self.assertEqual([router2], [r.id for r in result])
+
+        result = self._test_class.get_routers_by_network_id(
+            self.context, ext_network_id_1, policy_id=qos_policy_router_obj.id)
+        self.assertEqual([router3], [r.id for r in result])
+
+        result = self._test_class.get_routers_by_network_id(
+            self.context, ext_network_id_1, policy_id=qos_policy_net_obj.id)
+        self.assertEqual([], [r.id for r in result])
+
+        result = self._test_class.get_routers_by_network_id(
+            self.context, ext_network_id_2, policy_id=None)
+        self.assertEqual([router4], [r.id for r in result])
+
+        result = self._test_class.get_routers_by_network_id(
+            self.context, ext_network_id_2, policy_id=qos_policy_router_obj.id)
+        self.assertEqual([router5], [r.id for r in result])
+
+        result = self._test_class.get_routers_by_network_id(
+            self.context, ext_network_id_2, policy_id=qos_policy_net_obj.id)
+        self.assertEqual([], [r.id for r in result])

neutron/tests/unit/objects/test_base.py

@@ -1555,12 +1555,14 @@ class BaseDbObjectTestCase(_BaseObjectTestCase,
         _network.create()
         return _network
 
-    def _create_test_network_id(self):
+    def _create_test_network_id(self, qos_policy_id=None):
         return self._create_test_network(
-            "test-network-%s" % helpers.get_random_string(4)).id
+            name="test-network-%s" % helpers.get_random_string(4),
+            qos_policy_id=qos_policy_id).id
 
-    def _create_external_network_id(self):
-        test_network_id = self._create_test_network_id()
+    def _create_external_network_id(self, qos_policy_id=None):
+        test_network_id = self._create_test_network_id(
+            qos_policy_id=qos_policy_id)
         ext_net = net_obj.ExternalNetwork(self.context,
             network_id=test_network_id)
         ext_net.create()
@@ -1639,9 +1641,9 @@ class BaseDbObjectTestCase(_BaseObjectTestCase,
         segment.create()
         return segment.id
 
-    def _create_test_router_id(self, router_id=None):
+    def _create_test_router_id(self, router_id=None, name=None):
         attrs = {
-            'name': 'test_router',
+            'name': name or 'test_router',
         }
         if router_id:
             attrs['id'] = router_id

neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/test_qos.py

@@ -128,10 +128,12 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
         router.create()
         return router, network
 
-    def _update_router_qos(self, router_id, qos_policy_id, attach=True):
+    @db_api.CONTEXT_WRITER
+    def _update_router_qos(self, context, router_id, qos_policy_id,
+                           attach=True):
         # NOTE(ralonsoh): router QoS policy is not yet implemented in Router
         # OVO. Once we have this feature, this method can be removed.
-        qos = policy_obj.QosPolicy.get_policy_obj(self.ctx, qos_policy_id)
+        qos = policy_obj.QosPolicy.get_policy_obj(context, qos_policy_id)
         if attach:
             qos.attach_router(router_id)
         else:
@@ -375,7 +377,7 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
             mock.ANY, self.ports[1].id, self.ports[1].network_id, None, None)
 
     def test_update_network(self):
-        """Test update network.
+        """Test update network (internal ports).
 
         net1: [(1) from qos_policy0 to no QoS policy,
                (2) from qos_policy0 to qos_policy1]
@@ -395,7 +397,7 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
             self.networks[0].qos_policy_id = qos_policy_id
             self.networks[0].update()
             original_network = {'qos_policy_id': self.qos_policies[0]}
-            reviewed_port_ids, _ = self.qos_driver.update_network(
+            reviewed_port_ids, _, _ = self.qos_driver.update_network(
                 mock.ANY, self.networks[0], original_network)
             self.assertEqual(reference_ports, reviewed_port_ids)
             calls = [mock.call(mock.ANY, self.ports[0].id,
@@ -405,24 +407,30 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
             self.mock_rules.reset_mock()
 
     def test_update_external_network(self):
-        """Test update external network (floating IPs).
+        """Test update external network (floating IPs and GW IPs).
 
         - fip0: qos_policy0
         - fip1: no QoS FIP policy (inherits from external network QoS)
+        - router_fips: no QoS FIP policy (inherits from external network QoS)
         """
-        network_policies = [
-            (self.qos_policies[1].id, {self.fips[1].id}),
-            (None, {self.fips[1].id})]
+        network_policies = [(self.qos_policies[1].id,
+                             {self.fips[1].id},
+                             {self.router_fips.id}),
+                            (None,
+                             {self.fips[1].id},
+                             {self.router_fips.id})]
 
         self.fips[0].qos_policy_id = self.qos_policies[0].id
         self.fips[0].update()
-        for qos_policy_id, reference_fips in network_policies:
+        for qos_policy_id, ref_fips, ref_routers in network_policies:
             self.fips_network.qos_policy_id = qos_policy_id
             self.fips_network.update()
             original_network = {'qos_policy_id': self.qos_policies[0]}
-            _, reviewed_fips_ids = self.qos_driver.update_network(
-                mock.Mock(), self.fips_network, original_network)
-            self.assertEqual(reference_fips, reviewed_fips_ids)
+            _, reviewed_fips_ids, reviewed_router_ids = (
+                self.qos_driver.update_network(
+                    mock.Mock(), self.fips_network, original_network))
+            self.assertEqual(ref_fips, reviewed_fips_ids)
+            self.assertEqual(ref_routers, reviewed_router_ids)
 
     def test_update_network_no_policy_change(self):
         """Test update network if the QoS policy is the same.
@@ -434,10 +442,11 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
             self.networks[0].qos_policy_id = qos_policy_id
             self.networks[0].update()
             original_network = {'qos_policy_id': qos_policy_id}
-            port_ids, fip_ids = self.qos_driver.update_network(
+            port_ids, fip_ids, router_ids = self.qos_driver.update_network(
                 mock.ANY, self.networks[0], original_network)
             self.assertEqual(set([]), port_ids)
             self.assertEqual(set([]), fip_ids)
+            self.assertEqual(set([]), router_ids)
             self.mock_rules.assert_not_called()
 
     def test_update_network_reset(self):
@@ -461,7 +470,7 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
             self.networks[0].qos_policy_id = qos_policy_id
             self.networks[0].update()
             original_network = {'qos_policy_id': self.qos_policies[0]}
-            reviewed_port_ids, _ = self.qos_driver.update_network(
+            reviewed_port_ids, _, _ = self.qos_driver.update_network(
                 mock.ANY, self.networks[0], original_network, reset=True)
             self.assertEqual(reference_ports, reviewed_port_ids)
             calls = [mock.call(mock.ANY, self.ports[0].id,
@@ -491,7 +500,7 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
                 self.networks[0].qos_policy_id = qos_policy_id
                 self.networks[0].update()
                 original_network = {'qos_policy_id': self.qos_policies[0]}
-                reviewed_port_ids, _ = self.qos_driver.update_network(
+                reviewed_port_ids, _, _ = self.qos_driver.update_network(
                     mock.ANY, self.networks[0], original_network, reset=True)
                 self.assertEqual(reference_ports, reviewed_port_ids)
                 calls = [mock.call(
@@ -533,8 +542,10 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
         self.fips[0].update()
         self.fips[1].qos_policy_id = self.qos_policies[1].id
         self.fips[1].update()
-        self._update_router_qos(self.routers[0].id, self.qos_policies[0].id)
-        self._update_router_qos(self.routers[1].id, self.qos_policies[1].id)
+        self._update_router_qos(self.ctx, self.routers[0].id,
+                                self.qos_policies[0].id)
+        self._update_router_qos(self.ctx, self.routers[1].id,
+                                self.qos_policies[1].id)
         mock_qos_rules = mock.Mock()
         with mock.patch.object(self.qos_driver, '_qos_rules',
                                return_value=mock_qos_rules), \
@@ -665,7 +676,8 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
         nb_idl.reset_mock()
 
         # Add QoS policy.
-        self._update_router_qos(router['id'], self.qos_policies[0].id)
+        self._update_router_qos(self.ctx, router['id'],
+                                self.qos_policies[0].id)
         router = self._get_router(self.routers[0].id)
         self.qos_driver.update_router(txn, router)
         nb_idl.qos_add.assert_called_once()
@@ -673,9 +685,20 @@ class TestOVNClientQosExtension(test_plugin.Ml2PluginV2TestCase):
         nb_idl.reset_mock()
 
         # Remove QoS
-        self._update_router_qos(router['id'], self.qos_policies[0].id,
-                                attach=False)
+        self._update_router_qos(self.ctx, router['id'],
+                                self.qos_policies[0].id, attach=False)
         router = self._get_router(self.routers[0].id)
         self.qos_driver.update_router(txn, router)
         nb_idl.qos_add.assert_not_called()
         self.assertEqual(2, nb_idl.qos_del.call_count)
+        nb_idl.reset_mock()
+
+        # Add network QoS policy
+        ext_net = self.router_networks[0]
+        ext_net.qos_policy_id = self.qos_policies[1].id
+        ext_net.update()
+        router = self._get_router(self.routers[0].id)
+        self.qos_driver.update_router(txn, router)
+        nb_idl.qos_add.assert_called_once()
+        nb_idl.qos_del.assert_called_once()
+        nb_idl.reset_mock()

releasenotes/notes/qos-gwip-network-inheritance-support-in-ovn-95a53b1dbb5cbac0.yaml

@@ -0,0 +1,6 @@
+---
+features:
+  - |
+    Gateway IP QoS network inheritance is now available for OVN L3 plugin
+    QoS extension. If the router external network (gateway network) has a QoS
+    policy associated, the gateway IP port will inherit the network QoS policy.