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 <[email protected]>
Change-Id: I7bf4dc396a044d0d67d2d2f6e68140c063f3e3d8
(cherry picked from commit 4ebd69f)

Author: ralonsoh

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

@@ -348,14 +348,25 @@ def _add_port_qos_rules(self, txn, port_id, network_id, network_type,
         _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)
-                self._update_lsp_qos_options(txn, lsp, port_id, ovn_rule_lsp)
-                # In this particular case, the QoS rules should be defined 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)
+                if min_bw:
+                    ovn_rule_lsp = self._ovn_lsp_rule(rules)
+                    self._update_lsp_qos_options(txn, lsp, port_id,
+                                                 ovn_rule_lsp)
+                    # In this particular case, the QoS rules should be defined
+                    # 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_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 @@ def setUp(self, maintenance_worker=False):
     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 @@ def _check_rules_qos(self, rules, port_id, network_id, network_type,
                 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 @@ def test_update_policy(self):
                                   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 @@ def test_create_router_port_multiple_routers(self):
                     # GW network MTU=1400
                     self.assertEqual(1400, int(lrp.options['gateway_mtu']))
 
-    def test_update_port_with_qos(self):
-        def _check_bw(port_id, max_kbps=None, max_burst_kbps=None):
+    def test_update_port_with_qos_in_lsp_options(self):
+        # 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 @@ def _check_bw(port_id, max_kbps=None, max_burst_kbps=None):
             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.port(subnet) as port:
-                    port_data = port['port']
-                    # Check no QoS options.
-                    _check_bw(port_data['id'])
-
-                    # Add QoS policy.
-                    req = self.new_update_request(
-                        'ports',
-                        {'port': {'qos_policy_id': qos['id']}},
-                        port_data['id'])
-                    req.get_response(self.api)
-                    _check_bw(port_data['id'], max_kbps, max_burst_kbps)
-
-                    # Update port.
-                    req = self.new_update_request(
-                        'ports',
-                        {'port': {'name': uuidutils.generate_uuid()}},
-                        port_data['id'])
-                    req.get_response(self.api)
-                    _check_bw(port_data['id'], max_kbps, max_burst_kbps)
+            with self.subnet(net) as subnet, self.port(subnet) as port:
+                port_data = port['port']
+                # Check no QoS options.
+                _check_bw(port_data['id'])
+
+                # Add QoS policy.
+                req = self.new_update_request(
+                    'ports',
+                    {'port': {'qos_policy_id': qos['id']}},
+                    port_data['id'])
+                req.get_response(self.api)
+                _check_bw(port_data['id'], max_kbps, max_burst_kbps, min_kbps)
+
+                # Update port.
+                req = self.new_update_request(
+                    'ports',
+                    {'port': {'name': uuidutils.generate_uuid()}},
+                    port_data['id'])
+                req.get_response(self.api)
+                _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.