From 83d7eb961dc8a5d4e815bc3f110f8523a532caeb Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 15 Oct 2019 15:51:40 +0000 Subject: [PATCH] Add OVS QoS driver cache for minimum bandwidth rules 1. Add OVS QoS driver cache Added minimum bandwidth rules cache in the OVS QoS driver. If a new port is detected in the integration bridge, managed by the OVS agent driver, the QoS extension is applied (if configured). If this new port does not have a QoS rule, by default the QoS configuration is reset in this port. Because this port has no Queue related, the call "QosOVSAgentDriver.delete_minimum_bandwidth" should not execute the rule cleanup. This QoS rule cache implementation per porti is currently used in the OVS QoS driver for the DSCP and the bandwitdh limit rules. 2. If a Queue cannot be deleted, log the QoS registers using it If a Queue is still used in a QoS register in the OVSDB, it cannot be deleted. With the current design, only one QoS rule is created in the database and several Queues are assigned to it. If something external to the OVS agent is handling the Queues and by mistake a Queue is assigned to other QoS registers, this error is now logged before raising the exception. Closes-Bug: #1845176 Change-Id: Ia9077fc20e4ca360819a2e368c8c1f9250e5a6d8 --- neutron/agent/common/ovs_lib.py | 16 +++++++ .../agent/extension_drivers/qos_driver.py | 20 +++++++++ neutron/tests/fullstack/test_qos.py | 44 +++++++++++++------ .../functional/agent/common/test_ovs_lib.py | 16 +++++++ .../extension_drivers/test_qos_driver.py | 14 ++++-- 5 files changed, 93 insertions(+), 17 deletions(-) diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index 81210ce4210..b104c63aa5a 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -25,6 +25,7 @@ from neutron_lib import exceptions from neutron_lib.services.qos import constants as qos_constants from oslo_config import cfg from oslo_log import log as logging +from oslo_utils import excutils from oslo_utils import uuidutils from ovsdbapp.backend.ovs_idl import idlutils @@ -1085,6 +1086,21 @@ class OVSBridge(BaseOVS): self.ovsdb.db_destroy('Queue', queue_id).execute(check_error=True) except idlutils.RowNotFound: LOG.info('OVS Queue %s was already deleted', queue_id) + except RuntimeError as exc: + with excutils.save_and_reraise_exception(): + if 'referential integrity violation' not in str(exc): + return + qos_regs = self._list_qos() + qos_uuids = [] + for qos_reg in qos_regs: + queue_nums = [num for num, q in qos_reg['queues'].items() + if q.uuid == queue_id] + if queue_nums: + qos_uuids.append(str(qos_reg['_uuid'])) + LOG.error('Queue %(queue)s was still in use by the following ' + 'QoS rules: %(qoses)s', + {'queue': str(queue_id), + 'qoses': ', '.join(sorted(qos_uuids))}) def _update_qos(self, qos_id=None, queues=None): queues = queues or {} diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py b/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py index df8daa29ced..bda3b29f4dd 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/extension_drivers/qos_driver.py @@ -168,6 +168,10 @@ class QosOVSAgentDriver(qos.QosLinuxAgentDriver): 'vif_port was not found. It seems that port is already ' 'deleted', port.get('port_id')) return + + self.ports[port['port_id']][(qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH, + rule.direction)] = port + # queue_num is used to identify the port which traffic come from, # it needs to be unique across br-int. It is convenient to use ofport # as queue_num because it is unique in br-int and start from 1. @@ -187,10 +191,26 @@ class QosOVSAgentDriver(qos.QosLinuxAgentDriver): 'qos_id': qos_id, 'ports': egress_port_names}) def delete_minimum_bandwidth(self, port): + rule_port = self.ports[port['port_id']].pop( + (qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH, + constants.EGRESS_DIRECTION), None) + if not rule_port: + LOG.debug('delete_minimum_bandwidth was received for port %s but ' + 'no port information was stored to be deleted', + port['port_id']) + return self.br_int.delete_minimum_bandwidth_queue(port['port_id']) LOG.debug("Minimum bandwidth rule was deleted for port: %s.", port['port_id']) def delete_minimum_bandwidth_ingress(self, port): + rule_port = self.ports[port['port_id']].pop( + (qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH, + constants.INGRESS_DIRECTION), None) + if not rule_port: + LOG.debug('delete_minimum_bandwidth_ingress was received for port ' + '%s but no port information was stored to be deleted', + port['port_id']) + return LOG.debug("Minimum bandwidth rule for ingress direction was deleted " "for port %s", port['port_id']) diff --git a/neutron/tests/fullstack/test_qos.py b/neutron/tests/fullstack/test_qos.py index bfd43c6110c..9451319980a 100644 --- a/neutron/tests/fullstack/test_qos.py +++ b/neutron/tests/fullstack/test_qos.py @@ -91,17 +91,19 @@ class BaseQoSRuleTestCase(object): shared='False', is_default='False') def _prepare_vm_with_qos_policy(self, rule_add_functions): - qos_policy = self._create_qos_policy() - qos_policy_id = qos_policy['id'] + if rule_add_functions: + qos_policy = self._create_qos_policy() + qos_policy_id = qos_policy['id'] + for rule_add in rule_add_functions: + rule_add(qos_policy) + else: + qos_policy_id = qos_policy = None port = self.safe_client.create_port( self.tenant_id, self.network['id'], self.environment.hosts[0].hostname, qos_policy_id) - for rule_add in rule_add_functions: - rule_add(qos_policy) - vm = self.useFixture( machine.FakeFullstackMachine( self.environment.hosts[0], @@ -691,23 +693,37 @@ class TestMinBwQoSOvs(_TestMinBwQoS, base.BaseFullStackTestCase): self.fail('QoS register not found with queue-num %s' % queue_num) def test_min_bw_qos_port_removed(self): - """Test if min BW limit config is properly removed when port removed""" + """Test if min BW limit config is properly removed when port removed. + + In case another port is added without a QoS policy, the L2 agent QoS + extension will call "handle_port" and then it will force the reset of + this port (self._process_reset_port(port)). This test will check that + if the port is not present in the agent QoS cache, the policy is not + removed. + """ + # Create port without qos policy attached + vm_noqos, _ = self._prepare_vm_with_qos_policy(None) + # Create port with qos policy attached - vm, qos_policy = self._prepare_vm_with_qos_policy( + vm_qos, qos_policy = self._prepare_vm_with_qos_policy( [functools.partial( self._add_min_bw_rule, MIN_BANDWIDTH, self.direction)]) self._wait_for_min_bw_rule_applied( - vm, MIN_BANDWIDTH, self.direction) + vm_qos, MIN_BANDWIDTH, self.direction) - qos, queue = self._find_agent_qos_and_queue(vm) + # Check QoS policy and Queue rule. + qos, queue = self._find_agent_qos_and_queue(vm_qos) self.assertEqual({'min-rate': str(MIN_BANDWIDTH * 1000)}, queue.other_config) - queues = vm.bridge._list_queues(port=vm.neutron_port['id']) + queues = vm_qos.bridge._list_queues(port=vm_qos.neutron_port['id']) self.assertEqual(1, len(queues)) self.assertEqual(queue.uuid, queues[0]['_uuid']) # Delete port with qos policy attached - vm.destroy() - self._wait_for_min_bw_rule_removed(vm, self.direction) - self.assertEqual([], - vm.bridge._list_queues(port=vm.neutron_port['id'])) + vm_qos.destroy() + self._wait_for_min_bw_rule_removed(vm_qos, self.direction) + self.assertEqual( + [], + vm_qos.bridge._list_queues(port=vm_qos.neutron_port['id'])) + + vm_noqos.destroy() diff --git a/neutron/tests/functional/agent/common/test_ovs_lib.py b/neutron/tests/functional/agent/common/test_ovs_lib.py index b39f7d219b9..02de426e31f 100644 --- a/neutron/tests/functional/agent/common/test_ovs_lib.py +++ b/neutron/tests/functional/agent/common/test_ovs_lib.py @@ -225,6 +225,22 @@ class BaseOVSTestCase(base.BaseSudoTestCase): self.ovs._delete_queue(queue_id) self._check_value(None, self._list_queues, queue_id=queue_id) + def test__delete_queue_still_used_in_a_qos(self): + queue_id, port_id = self._create_queue() + queues = {1: queue_id} + qos_id_1 = self._create_qos(queues=queues) + self.ovs._min_bw_qos_id = uuidutils.generate_uuid() + qos_id_2 = self._create_qos(queues=queues) + with mock.patch.object(ovs_lib.LOG, 'error') as mock_error: + self.assertRaises(RuntimeError, self.ovs._delete_queue, + queue_id) + + qoses = ', '.join(sorted([str(qos_id_1), str(qos_id_2)])) + msg = ('Queue %(queue)s was still in use by the following QoS rules: ' + '%(qoses)s') + mock_error.assert_called_once_with( + msg, {'queue': str(queue_id), 'qoses': qoses}) + def test__update_qos_new(self): queue_id, port_id = self._create_queue() queues = {1: queue_id} diff --git a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/extension_drivers/test_qos_driver.py b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/extension_drivers/test_qos_driver.py index 513c6b8569b..7248941e370 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/extension_drivers/test_qos_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/openvswitch/agent/extension_drivers/test_qos_driver.py @@ -15,6 +15,7 @@ import copy import mock from neutron_lib import constants from neutron_lib import context +from neutron_lib.services.qos import constants as qos_consts from oslo_utils import uuidutils from neutron.objects.qos import policy @@ -209,9 +210,16 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase): with mock.patch.object(self.qos_driver.br_int, 'delete_minimum_bandwidth_queue') \ as mock_delete_minimum_bandwidth_queue: - self.qos_driver.delete_minimum_bandwidth({'port_id': 'port_id'}) - mock_delete_minimum_bandwidth_queue.assert_called_once_with( - 'port_id') + self.qos_driver.ports['p_id'] = {} + self.qos_driver.delete_minimum_bandwidth({'port_id': 'p_id'}) + mock_delete_minimum_bandwidth_queue.assert_not_called() + + mock_delete_minimum_bandwidth_queue.reset_mock() + self.qos_driver.ports['p_id'] = { + (qos_consts.RULE_TYPE_MINIMUM_BANDWIDTH, + constants.EGRESS_DIRECTION): 'rule_port'} + self.qos_driver.delete_minimum_bandwidth({'port_id': 'p_id'}) + mock_delete_minimum_bandwidth_queue.assert_called_once_with('p_id') def test_update_minimum_bandwidth_no_vif_port(self): with mock.patch.object(self.qos_driver.br_int,