diff --git a/neutron/agent/common/ovs_lib.py b/neutron/agent/common/ovs_lib.py index e3f04639c36..e9587043397 100644 --- a/neutron/agent/common/ovs_lib.py +++ b/neutron/agent/common/ovs_lib.py @@ -669,7 +669,7 @@ class OVSBridge(BaseOVS): self._set_egress_bw_limit_for_port( port_name, 0, 0) - def _find_qos(self, port_name): + def find_qos(self, port_name): qos = self.ovsdb.db_find( 'QoS', ('external_ids', '=', {'id': port_name}), @@ -677,7 +677,7 @@ class OVSBridge(BaseOVS): if qos: return qos[0] - def _find_queue(self, port_name, queue_type): + def find_queue(self, port_name, queue_type): queues = self.ovsdb.db_find( 'Queue', ('external_ids', '=', {'id': port_name, @@ -723,8 +723,8 @@ class OVSBridge(BaseOVS): 'max-rate': max_bw_in_bits, 'burst': max_burst_in_bits, } - qos = self._find_qos(port_name) - queue = self._find_queue(port_name, QOS_DEFAULT_QUEUE) + qos = self.find_qos(port_name) + queue = self.find_queue(port_name, QOS_DEFAULT_QUEUE) qos_uuid = qos['_uuid'] if qos else None queue_uuid = queue['_uuid'] if queue else None with self.ovsdb.transaction(check_error=True) as txn: @@ -743,7 +743,7 @@ class OVSBridge(BaseOVS): def get_ingress_bw_limit_for_port(self, port_name): max_kbps = None max_burst_kbit = None - res = self._find_queue(port_name, QOS_DEFAULT_QUEUE) + res = self.find_queue(port_name, QOS_DEFAULT_QUEUE) if res: other_config = res['other_config'] max_bw_in_bits = other_config.get('max-rate') @@ -755,15 +755,12 @@ class OVSBridge(BaseOVS): return max_kbps, max_burst_kbit def delete_ingress_bw_limit_for_port(self, port_name): - if not self.port_exists(port_name): - return - self._delete_ingress_bw_limit_for_port(port_name) - - def _delete_ingress_bw_limit_for_port(self, port_name): - qos = self._find_qos(port_name) - queue = self._find_queue(port_name, QOS_DEFAULT_QUEUE) + qos = self.find_qos(port_name) + queue = self.find_queue(port_name, QOS_DEFAULT_QUEUE) + does_port_exist = self.port_exists(port_name) with self.ovsdb.transaction(check_error=True) as txn: - txn.add(self.ovsdb.db_clear("Port", port_name, 'qos')) + if does_port_exist: + txn.add(self.ovsdb.db_clear("Port", port_name, 'qos')) if qos: txn.add(self.ovsdb.db_destroy('QoS', qos['_uuid'])) if queue: 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 7dff5e98670..4da07200a51 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 @@ -55,29 +55,39 @@ class QosOVSAgentDriver(qos.QosLinuxAgentDriver): "vif_port was not found. It seems that port is already " "deleted", port_id) return + self.ports[port['port_id']][(qos_consts.RULE_TYPE_BANDWIDTH_LIMIT, + rule.direction)] = port if rule.direction == constants.INGRESS_DIRECTION: self._update_ingress_bandwidth_limit(vif_port, rule) else: self._update_egress_bandwidth_limit(vif_port, rule) def delete_bandwidth_limit(self, port): - vif_port = port.get('vif_port') - if not vif_port: - port_id = port.get('port_id') - LOG.debug("delete_bandwidth_limit was received for port %s but " - "vif_port was not found. It seems that port is already " - "deleted", port_id) + port_id = port.get('port_id') + port = self.ports[port_id].pop((qos_consts.RULE_TYPE_BANDWIDTH_LIMIT, + constants.EGRESS_DIRECTION), + None) + if not port: + LOG.debug("delete_bandwidth_limit was received " + "for port %s but port was not found. " + "It seems that bandwidth_limit is already deleted", + port_id) return + vif_port = port.get('vif_port') self.br_int.delete_egress_bw_limit_for_port(vif_port.port_name) def delete_bandwidth_limit_ingress(self, port): - vif_port = port.get('vif_port') - if not vif_port: - port_id = port.get('port_id') + port_id = port.get('port_id') + port = self.ports[port_id].pop((qos_consts.RULE_TYPE_BANDWIDTH_LIMIT, + constants.INGRESS_DIRECTION), + None) + if not port: LOG.debug("delete_bandwidth_limit_ingress was received " - "for port %s but vif_port was not found. " - "It seems that port is already deleted", port_id) + "for port %s but port was not found. " + "It seems that bandwidth_limit is already deleted", + port_id) return + vif_port = port.get('vif_port') self.br_int.delete_ingress_bw_limit_for_port(vif_port.port_name) def create_dscp_marking(self, port, rule): diff --git a/neutron/tests/fullstack/test_qos.py b/neutron/tests/fullstack/test_qos.py index 65b0d4c21e3..427b2382f37 100644 --- a/neutron/tests/fullstack/test_qos.py +++ b/neutron/tests/fullstack/test_qos.py @@ -19,6 +19,7 @@ from neutronclient.common import exceptions from oslo_utils import uuidutils import testscenarios +from neutron.agent.common import ovs_lib from neutron.agent.linux import tc_lib from neutron.common import constants as common_constants from neutron.common import utils @@ -220,6 +221,26 @@ class TestBwLimitQoSOvs(_TestBwLimitQoS, base.BaseFullStackTestCase): lambda: vm.bridge.get_ingress_bw_limit_for_port( vm.port.name) == (limit, burst)) + def test_bw_limit_qos_port_removed(self): + """Test if rate limit config is properly removed when whole port is + removed. + """ + + # Create port with qos policy attached + vm, qos_policy = self._prepare_vm_with_qos_policy( + [functools.partial( + self._add_bw_limit_rule, + BANDWIDTH_LIMIT, BANDWIDTH_BURST, self.direction)]) + self._wait_for_bw_rule_applied( + vm, BANDWIDTH_LIMIT, BANDWIDTH_BURST, self.direction) + + # Delete port with qos policy attached + vm.destroy() + self._wait_for_bw_rule_removed(vm, self.direction) + self.assertIsNone(vm.bridge.find_qos(vm.port.name)) + self.assertIsNone(vm.bridge.find_queue(vm.port.name, + ovs_lib.QOS_DEFAULT_QUEUE)) + class TestBwLimitQoSLinuxbridge(_TestBwLimitQoS, base.BaseFullStackTestCase): l2_agent_type = constants.AGENT_TYPE_LINUXBRIDGE diff --git a/neutron/tests/unit/agent/common/test_ovs_lib.py b/neutron/tests/unit/agent/common/test_ovs_lib.py index 655757eb567..2b415d28795 100644 --- a/neutron/tests/unit/agent/common/test_ovs_lib.py +++ b/neutron/tests/unit/agent/common/test_ovs_lib.py @@ -782,26 +782,6 @@ class OVS_Lib_Test(base.BaseTestCase): port_exists_mock.assert_called_once_with("test_port") set_egress_mock.assert_not_called() - def test_delete_ingress_bw_limit_for_port(self): - with mock.patch.object( - self.br, "_delete_ingress_bw_limit_for_port" - ) as delete_ingress_mock, mock.patch.object( - self.br, "port_exists", return_value=True - ) as port_exists_mock: - self.br.delete_ingress_bw_limit_for_port("test_port") - port_exists_mock.assert_called_once_with("test_port") - delete_ingress_mock.assert_called_once_with("test_port") - - def test_delete_ingress_bw_limit_for_port_port_not_exists(self): - with mock.patch.object( - self.br, "_delete_ingress_bw_limit_for_port" - ) as delete_ingress_mock, mock.patch.object( - self.br, "port_exists", return_value=False - ) as port_exists_mock: - self.br.delete_ingress_bw_limit_for_port("test_port") - port_exists_mock.assert_called_once_with("test_port") - delete_ingress_mock.assert_not_called() - def test_get_vifs_by_ids(self): db_list_res = [ {'name': 'qvo1', 'ofport': 1, 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 dba464b3ddb..ca46735965e 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 @@ -55,7 +55,6 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase): self.qos_driver.br_int.delete_egress_bw_limit_for_port) self.delete_ingress = ( self.qos_driver.br_int.delete_ingress_bw_limit_for_port) - self.qos_driver.br_int.create_egress_bw_limit_for_port = mock.Mock() self.create_egress = ( self.qos_driver.br_int.create_egress_bw_limit_for_port) self.update_ingress = ( @@ -102,6 +101,7 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase): class FakeVifPort(object): port_name = self.port_name + ofport = 111 return {'vif_port': FakeVifPort(), 'qos_policy_id': policy_id, @@ -142,19 +142,28 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase): self.create_egress.assert_not_called() self.update_ingress.assert_not_called() - def _test_delete_rules(self, policy): + def _test_delete_rules(self, qos_policy): + self.qos_driver.br_int.get_ingress_bw_limit_for_port = mock.Mock( + return_value=(self.rules[1].max_kbps, + self.rules[1].max_burst_kbps)) + self.qos_driver.create(self.port, qos_policy) + self.qos_driver.delete(self.port, qos_policy) + self.delete_egress.assert_called_once_with(self.port_name) + self.delete_ingress.assert_called_once_with(self.port_name) + + def _test_delete_rules_no_policy(self): self.qos_driver.br_int.get_ingress_bw_limit_for_port = mock.Mock( return_value=(self.rules[1].max_kbps, self.rules[1].max_burst_kbps)) self.qos_driver.delete(self.port) - self.delete_egress.assert_called_once_with(self.port_name) - self.delete_ingress.assert_called_once_with(self.port_name) + self.delete_egress.assert_not_called() + self.delete_ingress.assert_not_called() def test_delete_rules(self): self._test_delete_rules(self.qos_policy) def test_delete_rules_no_policy(self): - self._test_delete_rules(None) + self._test_delete_rules_no_policy() def test_delete_rules_no_vif_port(self): port = copy.copy(self.port)