[Qos] Fix residues of ovs in ingress bw limit
When we delete vm port with attached QoS policy, it is just doing nothing if vif_port does not exist. This is fine for egress bandwidth limit as it is configured directly on vif_port in OVS. For ingress bw limit however it uses additional records in Openvswitch database: qos and queue. Those records are not cleaned up in such case. This patch also records port in self.ports in the case of bandwidth limit rules, just as in the case of dscp rules. Never execute port clear if vif_port not exists. Finally, ovs driver can clean such qos and queue records Change-Id: Iddeb49e1e6538a178ca468df0fdf9e0617ca4f1c Closes-Bug: #1726732
This commit is contained in:
parent
5fc8e47786
commit
ee423e1fa0
|
@ -697,7 +697,7 @@ class OVSBridge(BaseOVS):
|
||||||
self._set_egress_bw_limit_for_port(
|
self._set_egress_bw_limit_for_port(
|
||||||
port_name, 0, 0)
|
port_name, 0, 0)
|
||||||
|
|
||||||
def _find_qos(self, port_name):
|
def find_qos(self, port_name):
|
||||||
qos = self.ovsdb.db_find(
|
qos = self.ovsdb.db_find(
|
||||||
'QoS',
|
'QoS',
|
||||||
('external_ids', '=', {'id': port_name}),
|
('external_ids', '=', {'id': port_name}),
|
||||||
|
@ -705,7 +705,7 @@ class OVSBridge(BaseOVS):
|
||||||
if qos:
|
if qos:
|
||||||
return qos[0]
|
return qos[0]
|
||||||
|
|
||||||
def _find_queue(self, port_name, queue_type):
|
def find_queue(self, port_name, queue_type):
|
||||||
queues = self.ovsdb.db_find(
|
queues = self.ovsdb.db_find(
|
||||||
'Queue',
|
'Queue',
|
||||||
('external_ids', '=', {'id': port_name,
|
('external_ids', '=', {'id': port_name,
|
||||||
|
@ -751,8 +751,8 @@ class OVSBridge(BaseOVS):
|
||||||
'max-rate': max_bw_in_bits,
|
'max-rate': max_bw_in_bits,
|
||||||
'burst': max_burst_in_bits,
|
'burst': max_burst_in_bits,
|
||||||
}
|
}
|
||||||
qos = self._find_qos(port_name)
|
qos = self.find_qos(port_name)
|
||||||
queue = self._find_queue(port_name, QOS_DEFAULT_QUEUE)
|
queue = self.find_queue(port_name, QOS_DEFAULT_QUEUE)
|
||||||
qos_uuid = qos['_uuid'] if qos else None
|
qos_uuid = qos['_uuid'] if qos else None
|
||||||
queue_uuid = queue['_uuid'] if queue else None
|
queue_uuid = queue['_uuid'] if queue else None
|
||||||
with self.ovsdb.transaction(check_error=True) as txn:
|
with self.ovsdb.transaction(check_error=True) as txn:
|
||||||
|
@ -771,7 +771,7 @@ class OVSBridge(BaseOVS):
|
||||||
def get_ingress_bw_limit_for_port(self, port_name):
|
def get_ingress_bw_limit_for_port(self, port_name):
|
||||||
max_kbps = None
|
max_kbps = None
|
||||||
max_burst_kbit = 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:
|
if res:
|
||||||
other_config = res['other_config']
|
other_config = res['other_config']
|
||||||
max_bw_in_bits = other_config.get('max-rate')
|
max_bw_in_bits = other_config.get('max-rate')
|
||||||
|
@ -783,15 +783,12 @@ class OVSBridge(BaseOVS):
|
||||||
return max_kbps, max_burst_kbit
|
return max_kbps, max_burst_kbit
|
||||||
|
|
||||||
def delete_ingress_bw_limit_for_port(self, port_name):
|
def delete_ingress_bw_limit_for_port(self, port_name):
|
||||||
if not self.port_exists(port_name):
|
qos = self.find_qos(port_name)
|
||||||
return
|
queue = self.find_queue(port_name, QOS_DEFAULT_QUEUE)
|
||||||
self._delete_ingress_bw_limit_for_port(port_name)
|
does_port_exist = self.port_exists(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)
|
|
||||||
with self.ovsdb.transaction(check_error=True) as txn:
|
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:
|
if qos:
|
||||||
txn.add(self.ovsdb.db_destroy('QoS', qos['_uuid']))
|
txn.add(self.ovsdb.db_destroy('QoS', qos['_uuid']))
|
||||||
if queue:
|
if queue:
|
||||||
|
|
|
@ -55,29 +55,39 @@ class QosOVSAgentDriver(qos.QosLinuxAgentDriver):
|
||||||
"vif_port was not found. It seems that port is already "
|
"vif_port was not found. It seems that port is already "
|
||||||
"deleted", port_id)
|
"deleted", port_id)
|
||||||
return
|
return
|
||||||
|
self.ports[port['port_id']][(qos_consts.RULE_TYPE_BANDWIDTH_LIMIT,
|
||||||
|
rule.direction)] = port
|
||||||
if rule.direction == constants.INGRESS_DIRECTION:
|
if rule.direction == constants.INGRESS_DIRECTION:
|
||||||
self._update_ingress_bandwidth_limit(vif_port, rule)
|
self._update_ingress_bandwidth_limit(vif_port, rule)
|
||||||
else:
|
else:
|
||||||
self._update_egress_bandwidth_limit(vif_port, rule)
|
self._update_egress_bandwidth_limit(vif_port, rule)
|
||||||
|
|
||||||
def delete_bandwidth_limit(self, port):
|
def delete_bandwidth_limit(self, port):
|
||||||
vif_port = port.get('vif_port')
|
port_id = port.get('port_id')
|
||||||
if not vif_port:
|
port = self.ports[port_id].pop((qos_consts.RULE_TYPE_BANDWIDTH_LIMIT,
|
||||||
port_id = port.get('port_id')
|
constants.EGRESS_DIRECTION),
|
||||||
LOG.debug("delete_bandwidth_limit was received for port %s but "
|
None)
|
||||||
"vif_port was not found. It seems that port is already "
|
if not port:
|
||||||
"deleted", port_id)
|
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
|
return
|
||||||
|
vif_port = port.get('vif_port')
|
||||||
self.br_int.delete_egress_bw_limit_for_port(vif_port.port_name)
|
self.br_int.delete_egress_bw_limit_for_port(vif_port.port_name)
|
||||||
|
|
||||||
def delete_bandwidth_limit_ingress(self, port):
|
def delete_bandwidth_limit_ingress(self, port):
|
||||||
vif_port = port.get('vif_port')
|
port_id = port.get('port_id')
|
||||||
if not vif_port:
|
port = self.ports[port_id].pop((qos_consts.RULE_TYPE_BANDWIDTH_LIMIT,
|
||||||
port_id = port.get('port_id')
|
constants.INGRESS_DIRECTION),
|
||||||
|
None)
|
||||||
|
if not port:
|
||||||
LOG.debug("delete_bandwidth_limit_ingress was received "
|
LOG.debug("delete_bandwidth_limit_ingress was received "
|
||||||
"for port %s but vif_port was not found. "
|
"for port %s but port was not found. "
|
||||||
"It seems that port is already deleted", port_id)
|
"It seems that bandwidth_limit is already deleted",
|
||||||
|
port_id)
|
||||||
return
|
return
|
||||||
|
vif_port = port.get('vif_port')
|
||||||
self.br_int.delete_ingress_bw_limit_for_port(vif_port.port_name)
|
self.br_int.delete_ingress_bw_limit_for_port(vif_port.port_name)
|
||||||
|
|
||||||
def create_dscp_marking(self, port, rule):
|
def create_dscp_marking(self, port, rule):
|
||||||
|
|
|
@ -19,6 +19,7 @@ from neutronclient.common import exceptions
|
||||||
from oslo_utils import uuidutils
|
from oslo_utils import uuidutils
|
||||||
import testscenarios
|
import testscenarios
|
||||||
|
|
||||||
|
from neutron.agent.common import ovs_lib
|
||||||
from neutron.agent.linux import tc_lib
|
from neutron.agent.linux import tc_lib
|
||||||
from neutron.common import utils
|
from neutron.common import utils
|
||||||
from neutron.services.qos import qos_consts
|
from neutron.services.qos import qos_consts
|
||||||
|
@ -219,6 +220,26 @@ class TestBwLimitQoSOvs(_TestBwLimitQoS, base.BaseFullStackTestCase):
|
||||||
lambda: vm.bridge.get_ingress_bw_limit_for_port(
|
lambda: vm.bridge.get_ingress_bw_limit_for_port(
|
||||||
vm.port.name) == (limit, burst))
|
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):
|
class TestBwLimitQoSLinuxbridge(_TestBwLimitQoS, base.BaseFullStackTestCase):
|
||||||
l2_agent_type = constants.AGENT_TYPE_LINUXBRIDGE
|
l2_agent_type = constants.AGENT_TYPE_LINUXBRIDGE
|
||||||
|
|
|
@ -813,26 +813,6 @@ class OVS_Lib_Test(base.BaseTestCase):
|
||||||
port_exists_mock.assert_called_once_with("test_port")
|
port_exists_mock.assert_called_once_with("test_port")
|
||||||
set_egress_mock.assert_not_called()
|
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):
|
def test_get_vifs_by_ids(self):
|
||||||
db_list_res = [
|
db_list_res = [
|
||||||
{'name': 'qvo1', 'ofport': 1,
|
{'name': 'qvo1', 'ofport': 1,
|
||||||
|
|
|
@ -55,7 +55,6 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase):
|
||||||
self.qos_driver.br_int.delete_egress_bw_limit_for_port)
|
self.qos_driver.br_int.delete_egress_bw_limit_for_port)
|
||||||
self.delete_ingress = (
|
self.delete_ingress = (
|
||||||
self.qos_driver.br_int.delete_ingress_bw_limit_for_port)
|
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.create_egress = (
|
||||||
self.qos_driver.br_int.create_egress_bw_limit_for_port)
|
self.qos_driver.br_int.create_egress_bw_limit_for_port)
|
||||||
self.update_ingress = (
|
self.update_ingress = (
|
||||||
|
@ -102,6 +101,7 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase):
|
||||||
|
|
||||||
class FakeVifPort(object):
|
class FakeVifPort(object):
|
||||||
port_name = self.port_name
|
port_name = self.port_name
|
||||||
|
ofport = 111
|
||||||
|
|
||||||
return {'vif_port': FakeVifPort(),
|
return {'vif_port': FakeVifPort(),
|
||||||
'qos_policy_id': policy_id,
|
'qos_policy_id': policy_id,
|
||||||
|
@ -142,19 +142,28 @@ class QosOVSAgentDriverTestCase(ovs_test_base.OVSAgentConfigTestBase):
|
||||||
self.create_egress.assert_not_called()
|
self.create_egress.assert_not_called()
|
||||||
self.update_ingress.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(
|
self.qos_driver.br_int.get_ingress_bw_limit_for_port = mock.Mock(
|
||||||
return_value=(self.rules[1].max_kbps,
|
return_value=(self.rules[1].max_kbps,
|
||||||
self.rules[1].max_burst_kbps))
|
self.rules[1].max_burst_kbps))
|
||||||
self.qos_driver.delete(self.port)
|
self.qos_driver.delete(self.port)
|
||||||
self.delete_egress.assert_called_once_with(self.port_name)
|
self.delete_egress.assert_not_called()
|
||||||
self.delete_ingress.assert_called_once_with(self.port_name)
|
self.delete_ingress.assert_not_called()
|
||||||
|
|
||||||
def test_delete_rules(self):
|
def test_delete_rules(self):
|
||||||
self._test_delete_rules(self.qos_policy)
|
self._test_delete_rules(self.qos_policy)
|
||||||
|
|
||||||
def test_delete_rules_no_policy(self):
|
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):
|
def test_delete_rules_no_vif_port(self):
|
||||||
port = copy.copy(self.port)
|
port = copy.copy(self.port)
|
||||||
|
|
Loading…
Reference in New Issue