[OVN] Implement GW IP network QoS inheritance

This patch enables the gateway IP network QoS inheritance in
the OVN backend driver. The OVN QoS extension will use the
router external network (GW network) QoS policy if the gateway
IP port has no QoS policy assigned.

Partial-Bug: #1950454

Change-Id: I5ee51dc124ae464b9e9fd366cf7bf85176376c25

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

@@ -126,10 +126,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:
@@ -373,7 +375,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]
@@ -393,7 +395,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,
@@ -403,24 +405,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.
@@ -432,10 +440,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):
@@ -459,7 +468,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,
@@ -489,7 +498,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(
@@ -531,8 +540,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), \
@@ -663,7 +674,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()
@@ -671,9 +683,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.