Use the OVN policer for max-bw only QoS policies

As reported in the related bug, when [1] was implemented, that
introduced a regression when a port located in a physical network,
with maximum bandwidth *only* QoS rules and egress direction, was
bound to a compute node with a bond interface on the physical network
bridge. The TC commands have no effect on the interfaces of the bond.

This patch restores the backend enforcement for this scenario (physical
networks, egress direction, maximum bandwidth rule only) to the OVN
policer, using the OVN ``QoS`` registers, instead of relying on the
physical interface bandwidth enforcement, using TC commands.

[1]https://review.opendev.org/c/openstack/neutron/+/934418

Closes-Bug: #2115952
Signed-off-by: Rodolfo Alonso Hernandez <ralonsoh@redhat.com>
Change-Id: I7bf4dc396a044d0d67d2d2f6e68140c063f3e3d8

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

@@ -348,14 +348,25 @@ class OVNClientQosExtension:
         _qos_rules = (copy.deepcopy(qos_rules) if qos_rules else
                       self._qos_rules(admin_context, qos_policy_id))
         for direction, rules in _qos_rules.items():
+            min_bw = rules.get(qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH)
+            # NOTE(ralonsoh): the QoS rules are defined in the LSP.options
+            # dictionary if (1) direction=egress, (2) the network is physical
+            # and (3) there are min-bw rules. Otherwise, the OVN QoS registers
+            # are used (OVN BW policer).
             if (network_type in TYPE_PHYSICAL and
                     direction == constants.EGRESS_DIRECTION):
-                ovn_rule_lsp = self._ovn_lsp_rule(rules)
+                if min_bw:
-                self._update_lsp_qos_options(txn, lsp, port_id, ovn_rule_lsp)
+                    ovn_rule_lsp = self._ovn_lsp_rule(rules)
-                # In this particular case, the QoS rules should be defined in
+                    self._update_lsp_qos_options(txn, lsp, port_id,
-                # LSP.options. Only DSCP rule will create a QoS entry.
+                                                 ovn_rule_lsp)
-                rules.pop(qos_consts.RULE_TYPE_BANDWIDTH_LIMIT, None)
+                    # In this particular case, the QoS rules should be defined
-                rules.pop(qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH, None)
+                    # in LSP.options. Only DSCP rule will create a QoS entry.
+                    rules.pop(qos_consts.RULE_TYPE_BANDWIDTH_LIMIT, None)
+                    rules.pop(qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH, None)
+                else:
+                    # Clear the LSP.options QoS rules.
+                    self._update_lsp_qos_options(txn, lsp, port_id,
+                                                 self._ovn_lsp_rule({}))
 
             ovn_rule_qos = self._ovn_qos_rule(direction, rules, port_id,
                                               network_id)

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

@@ -69,6 +69,13 @@ QOS_RULES_4 = {
         qos_constants.RULE_TYPE_MINIMUM_BANDWIDTH: QOS_RULE_MINBW_1}
 }
 
+QOS_RULES_5 = {
+    constants.EGRESS_DIRECTION: {
+        qos_constants.RULE_TYPE_BANDWIDTH_LIMIT: QOS_RULE_BW_1,
+        qos_constants.RULE_TYPE_DSCP_MARKING: QOS_RULE_DSCP_1,
+        qos_constants.RULE_TYPE_MINIMUM_BANDWIDTH: QOS_RULE_MINBW_1},
+}
+
 
 class _TestOVNClientQosExtensionBase(base.TestOVNFunctionalBase):
     def setUp(self, maintenance_worker=False):
@@ -78,7 +85,9 @@ class _TestOVNClientQosExtensionBase(base.TestOVNFunctionalBase):
     def _check_rules_qos(self, rules, port_id, network_id, network_type,
                          fip_id=None, ip_address=None, expected_ext_ids=None):
         qos_rules = copy.deepcopy(rules)
-        if network_type in (constants.TYPE_VLAN, constants.TYPE_FLAT):
+        min_bw = qos_rules.get(constants.EGRESS_DIRECTION, {}).get(
+            qos_constants.RULE_TYPE_MINIMUM_BANDWIDTH)
+        if network_type in constants.TYPE_PHYSICAL and min_bw:
             # Remove the egress max-rate and min-rate rules, these are defined
             # in the LSP.options field for a physical network.
             try:
@@ -135,12 +144,13 @@ class _TestOVNClientQosExtensionBase(base.TestOVNFunctionalBase):
                 self.assertEqual(bandwidth, rule.bandwidth)
 
     def _check_rules_lsp(self, rules, port_id, network_type):
-        if network_type not in (constants.TYPE_VLAN, constants.TYPE_FLAT):
+        egress_rules = rules.get(constants.EGRESS_DIRECTION, {})
+        min_bw = egress_rules.get(qos_constants.RULE_TYPE_MINIMUM_BANDWIDTH)
+        if not (network_type in constants.TYPE_PHYSICAL and min_bw):
             return
 
         # If there are no egress rules, it is checked that there are no
         # QoS parameters in the LSP.options dictionary.
-        egress_rules = rules.get(constants.EGRESS_DIRECTION, {})
         qos_rule_lsp = self.qos_driver._ovn_lsp_rule(egress_rules)
         lsp = self.qos_driver.nb_idl.lsp_get(port_id).execute(
             check_error=True)
@@ -267,6 +277,62 @@ class TestOVNClientQosExtension(_TestOVNClientQosExtensionBase):
                                   constants.TYPE_VLAN)
             self._check_rules_lsp(_qos_rules, port['id'], constants.TYPE_VLAN)
 
+    def test_set_and_update_physical_network_qos(self):
+        # The goal of this test is to check how the OVN QoS registers and
+        # LSP.options are set and deleted, depending on the QoS policy rules.
+        # Check LP#2115952 for more information.
+        port = uuidutils.generate_uuid()
+        self._add_logical_switch_port(port)
+
+        def _apply_rules(qos_rules):
+            with self.nb_api.transaction(check_error=True) as txn:
+                _qos_rules = copy.deepcopy(qos_rules)
+                for direction in constants.VALID_DIRECTIONS:
+                    _qos_rules[direction] = _qos_rules.get(direction, {})
+                self.mock_qos_rules.return_value = copy.deepcopy(_qos_rules)
+                self.qos_driver._update_port_qos_rules(
+                    txn, port, self.network_1, constants.TYPE_VLAN, 'qos1',
+                    None)
+
+        # Loop this test twice, to check that all the QoS registers and
+        # parameters are correctly created, set or removed, regardless of the
+        # previous state.
+        for _ in range(2):
+            # Apply QOS_RULES_5: egress with max-bw, min-bw and DSCP rules.
+            # * Check the OVN QoS rule created has only DSCP information.
+            # * Check the LSP.options have the correct fields.
+            _apply_rules(QOS_RULES_5)
+            lsp = self.qos_driver.nb_idl.lsp_get(port).execute(
+                check_error=True)
+            for _param in ('qos_max_rate', 'qos_burst', 'qos_min_rate'):
+                self.assertIn(_param, lsp.options)
+            ls = self.qos_driver.nb_idl.lookup(
+                'Logical_Switch', ovn_utils.ovn_name(self.network_1))
+            self.assertEqual(1, len(ls.qos_rules))
+            rule = ls.qos_rules[0]
+            self.assertIn(port, rule.match)
+            self.assertEqual({'dscp': QOS_RULE_DSCP_1['dscp_mark']},
+                             rule.action)
+            self.assertEqual({}, rule.bandwidth)
+
+            # Apply QOS_RULES_3: egress with max-bw only rule.
+            # * Check the OVN QoS rule created has only max-bw information.
+            # * Check the LSP.options has no QoS information.
+            _apply_rules(QOS_RULES_3)
+            lsp = self.qos_driver.nb_idl.lsp_get(port).execute(
+                check_error=True)
+            for _param in ('qos_max_rate', 'qos_burst', 'qos_min_rate'):
+                self.assertNotIn(_param, lsp.options)
+            ls = self.qos_driver.nb_idl.lookup(
+                'Logical_Switch', ovn_utils.ovn_name(self.network_1))
+            self.assertEqual(1, len(ls.qos_rules))
+            rule = ls.qos_rules[0]
+            self.assertIn(port, rule.match)
+            self.assertEqual({}, rule.action)
+            self.assertEqual({'burst': QOS_RULE_BW_1['max_burst_kbps'],
+                              'rate': QOS_RULE_BW_1['max_kbps']},
+                             rule.bandwidth)
+
 
 class TestOVNClientQosExtensionEndToEnd(_TestOVNClientQosExtensionBase):
 

neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py

@@ -386,8 +386,13 @@ class TestOVNClient(testlib_api.MySQLTestCaseMixin,
                     # GW network MTU=1400
                     self.assertEqual(1400, int(lrp.options['gateway_mtu']))
 
-    def test_update_port_with_qos(self):
+    def test_update_port_with_qos_in_lsp_options(self):
-        def _check_bw(port_id, max_kbps=None, max_burst_kbps=None):
+        # The goal of this test is to check that a Neutron port that has a
+        # QoS policy defined in the LSP.options (physical network, egress
+        # rules, max+min BW rules) is correctly updated and the LSP.options
+        # are defined when the Neutron port is updated (see LP#2106231).
+        def _check_bw(port_id, max_kbps=None, max_burst_kbps=None,
+                      min_kbps=None):
             lsp = self.nb_api.lookup('Logical_Switch_Port', port_id)
             if max_kbps:
                 self.assertEqual(
@@ -403,37 +408,47 @@ class TestOVNClient(testlib_api.MySQLTestCaseMixin,
             else:
                 self.assertNotIn(ovn_const.LSP_OPTIONS_QOS_BURST,
                                  lsp.options)
+            if min_kbps:
+                self.assertEqual(
+                    '{}'.format(min_kbps * 1000),
+                    lsp.options[ovn_const.LSP_OPTIONS_QOS_MIN_RATE])
+            else:
+                self.assertNotIn(ovn_const.LSP_OPTIONS_QOS_MIN_RATE,
+                                 lsp.options)
 
         res = self._create_qos_policy(self.fmt, is_admin=True)
         qos = self.deserialize(self.fmt, res)['policy']
-        max_kbps, max_burst_kbps = 1000, 800
+        max_kbps, max_burst_kbps, min_kbps = 1000, 800, 600
+
         self._create_qos_rule(self.fmt, qos['id'],
                               qos_const.RULE_TYPE_BANDWIDTH_LIMIT,
                               max_kbps=max_kbps, max_burst_kbps=max_burst_kbps,
                               is_admin=True)
+        self._create_qos_rule(self.fmt, qos['id'],
+                              qos_const.RULE_TYPE_MINIMUM_BANDWIDTH,
+                              min_kbps=min_kbps, is_admin=True)
         net_args = {provider_net.NETWORK_TYPE: 'flat',
                     provider_net.PHYSICAL_NETWORK: 'datacentre'}
         with self.network(uuidutils.generate_uuid(),
                           arg_list=tuple(net_args.keys()), as_admin=True,
                           **net_args) as net:
-            with self.subnet(net) as subnet:
+            with self.subnet(net) as subnet, self.port(subnet) as port:
-                with self.port(subnet) as port:
+                port_data = port['port']
-                    port_data = port['port']
+                # Check no QoS options.
-                    # Check no QoS options.
+                _check_bw(port_data['id'])
-                    _check_bw(port_data['id'])
 
-                    # Add QoS policy.
+                # Add QoS policy.
-                    req = self.new_update_request(
+                req = self.new_update_request(
-                        'ports',
+                    'ports',
-                        {'port': {'qos_policy_id': qos['id']}},
+                    {'port': {'qos_policy_id': qos['id']}},
-                        port_data['id'])
+                    port_data['id'])
-                    req.get_response(self.api)
+                req.get_response(self.api)
-                    _check_bw(port_data['id'], max_kbps, max_burst_kbps)
+                _check_bw(port_data['id'], max_kbps, max_burst_kbps, min_kbps)
 
-                    # Update port.
+                # Update port.
-                    req = self.new_update_request(
+                req = self.new_update_request(
-                        'ports',
+                    'ports',
-                        {'port': {'name': uuidutils.generate_uuid()}},
+                    {'port': {'name': uuidutils.generate_uuid()}},
-                        port_data['id'])
+                    port_data['id'])
-                    req.get_response(self.api)
+                req.get_response(self.api)
-                    _check_bw(port_data['id'], max_kbps, max_burst_kbps)
+                _check_bw(port_data['id'], max_kbps, max_burst_kbps, min_kbps)

releasenotes/notes/ovn-qos-max-bw-physical-networks-843dfce4a60fc38f.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    When using the ML2/OVN mechanism driver, the QoS policies with maximum
+    bandwidth rules only are always enforced using the internal OVN policers,
+    regardless of the direction and network type. It is not relevant if the
+    QoS policy has or not DSCP rules.