From 4ebd69f2c5456d8f3a357eefac1e07c46c54fea3 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Fri, 4 Jul 2025 14:17:18 +0000 Subject: [PATCH] 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 Change-Id: I7bf4dc396a044d0d67d2d2f6e68140c063f3e3d8 --- .../ovn/mech_driver/ovsdb/extensions/qos.py | 23 ++++-- .../mech_driver/ovsdb/extensions/test_qos.py | 72 ++++++++++++++++++- .../ovn/mech_driver/ovsdb/test_ovn_client.py | 59 +++++++++------ ...bw-physical-networks-843dfce4a60fc38f.yaml | 7 ++ 4 files changed, 130 insertions(+), 31 deletions(-) create mode 100644 releasenotes/notes/ovn-qos-max-bw-physical-networks-843dfce4a60fc38f.yaml diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/qos.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/qos.py index 6e4bc330b41..f896f720614 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/qos.py +++ b/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) - 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) diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/test_qos.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/test_qos.py index 2a9e01e835b..3cf8db87ecc 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/extensions/test_qos.py +++ b/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): diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py index 7023869d4be..8d9fa3ea999 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_client.py +++ b/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 _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 @@ 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.port(subnet) as port: - port_data = port['port'] - # Check no QoS options. - _check_bw(port_data['id']) + 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) + # 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) + # 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) diff --git a/releasenotes/notes/ovn-qos-max-bw-physical-networks-843dfce4a60fc38f.yaml b/releasenotes/notes/ovn-qos-max-bw-physical-networks-843dfce4a60fc38f.yaml new file mode 100644 index 00000000000..d42bba27a59 --- /dev/null +++ b/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.