diff --git a/neutron/agent/l2/extensions/qos.py b/neutron/agent/l2/extensions/qos.py index bf0866b6ade..8fb599af96e 100644 --- a/neutron/agent/l2/extensions/qos.py +++ b/neutron/agent/l2/extensions/qos.py @@ -23,6 +23,7 @@ import six from neutron._i18n import _LW, _LI from neutron.agent.l2 import agent_extension +from neutron.agent.linux import tc_lib from neutron.api.rpc.callbacks.consumer import registry from neutron.api.rpc.callbacks import events from neutron.api.rpc.callbacks import resources @@ -121,6 +122,15 @@ class QosAgentDriver(object): LOG.debug("Port %(port)s excluded from QoS rule %(rule)s", {'port': port, 'rule': rule.id}) + def _get_egress_burst_value(self, rule): + """Return burst value used for egress bandwidth limitation. + + Because Egress bw_limit is done on ingress qdisc by LB and ovs drivers + so it will return burst_value used by tc on as ingress_qdisc. + """ + return tc_lib.TcCommand.get_ingress_qdisc_burst_value( + rule.max_kbps, rule.max_burst_kbps) + class PortPolicyMap(object): def __init__(self): diff --git a/neutron/agent/linux/tc_lib.py b/neutron/agent/linux/tc_lib.py index d6f5dc1dd33..df6de290b04 100644 --- a/neutron/agent/linux/tc_lib.py +++ b/neutron/agent/linux/tc_lib.py @@ -19,6 +19,7 @@ from neutron_lib import exceptions from neutron._i18n import _ from neutron.agent.linux import ip_lib +from neutron.services.qos import qos_consts INGRESS_QDISC_ID = "ffff:" @@ -102,6 +103,17 @@ class TcCommand(ip_lib.IPDevice): ip_wrapper = ip_lib.IPWrapper(self.namespace) return ip_wrapper.netns.execute(cmd, run_as_root=True, **kwargs) + @staticmethod + def get_ingress_qdisc_burst_value(bw_limit, burst_limit): + """Return burst value used in ingress qdisc. + + If burst value is not specified given than it will be set to default + rate to ensure that limit for TCP traffic will work well + """ + if not burst_limit: + return float(bw_limit) * qos_consts.DEFAULT_BURST_RATE + return burst_limit + def get_filters_bw_limits(self, qdisc_id=INGRESS_QDISC_ID): cmd = ['filter', 'show', 'dev', self.name, 'parent', qdisc_id] cmd_result = self._execute_tc_cmd(cmd) @@ -190,14 +202,6 @@ class TcCommand(ip_lib.IPDevice): # are trying to delete qdisc return self._execute_tc_cmd(cmd, extra_ok_codes=[2]) - def _get_filters_burst_value(self, bw_limit, burst_limit): - if not burst_limit: - # NOTE(slaweq): If burst value was not specified by user than it - # will be set as 80% of bw_limit to ensure that limit for TCP - # traffic will work well: - return float(bw_limit) * 0.8 - return burst_limit - def _get_tbf_burst_value(self, bw_limit, burst_limit): min_burst_value = float(bw_limit) / float(self.kernel_hz) return max(min_burst_value, burst_limit) @@ -220,7 +224,9 @@ class TcCommand(ip_lib.IPDevice): qdisc_id=INGRESS_QDISC_ID): rate_limit = "%s%s" % (bw_limit, BW_LIMIT_UNIT) burst = "%s%s" % ( - self._get_filters_burst_value(bw_limit, burst_limit), BURST_UNIT) + self.get_ingress_qdisc_burst_value(bw_limit, burst_limit), + BURST_UNIT + ) #NOTE(slaweq): it is made in exactly same way how openvswitch is doing # it when configuing ingress traffic limit on port. It can be found in # lib/netdev-linux.c#L4698 in openvswitch sources: diff --git a/neutron/plugins/ml2/drivers/linuxbridge/agent/extension_drivers/qos_driver.py b/neutron/plugins/ml2/drivers/linuxbridge/agent/extension_drivers/qos_driver.py index 67e9667a913..2d2b88d3741 100644 --- a/neutron/plugins/ml2/drivers/linuxbridge/agent/extension_drivers/qos_driver.py +++ b/neutron/plugins/ml2/drivers/linuxbridge/agent/extension_drivers/qos_driver.py @@ -38,14 +38,14 @@ class QosLinuxbridgeAgentDriver(qos.QosAgentDriver): def create_bandwidth_limit(self, port, rule): tc_wrapper = self._get_tc_wrapper(port) tc_wrapper.set_filters_bw_limit( - rule.max_kbps, rule.max_burst_kbps + rule.max_kbps, self._get_egress_burst_value(rule) ) @log_helpers.log_method_call def update_bandwidth_limit(self, port, rule): tc_wrapper = self._get_tc_wrapper(port) tc_wrapper.update_filters_bw_limit( - rule.max_kbps, rule.max_burst_kbps + rule.max_kbps, self._get_egress_burst_value(rule) ) @log_helpers.log_method_call 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 7e7a50b33a1..84fcdf3f264 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 @@ -53,7 +53,10 @@ class QosOVSAgentDriver(qos.QosAgentDriver): "deleted", port_id) return max_kbps = rule.max_kbps - max_burst_kbps = rule.max_burst_kbps + # NOTE(slaweq): According to ovs docs: + # http://openvswitch.org/support/dist-docs/ovs-vswitchd.conf.db.5.html + # ovs accepts only integer values of burst: + max_burst_kbps = int(self._get_egress_burst_value(rule)) self.br_int.create_egress_bw_limit_for_port(vif_port.port_name, max_kbps, diff --git a/neutron/services/qos/qos_consts.py b/neutron/services/qos/qos_consts.py index 50ec27d7593..662857a893d 100644 --- a/neutron/services/qos/qos_consts.py +++ b/neutron/services/qos/qos_consts.py @@ -18,3 +18,9 @@ RULE_TYPE_DSCP_MARK = 'dscp_marking' VALID_RULE_TYPES = [RULE_TYPE_BANDWIDTH_LIMIT, RULE_TYPE_DSCP_MARK] QOS_POLICY_ID = 'qos_policy_id' + +# NOTE(slaweq): Value used to calculate burst value for egress bandwidth limit +# if burst is not given by user. In such case burst value will be calculated +# as 80% of bw_limit to ensure that at least limits for TCP traffic will work +# fine. +DEFAULT_BURST_RATE = 0.8 diff --git a/neutron/tests/fullstack/test_qos.py b/neutron/tests/fullstack/test_qos.py index 6d95c4012f3..1b65e1cfc0e 100644 --- a/neutron/tests/fullstack/test_qos.py +++ b/neutron/tests/fullstack/test_qos.py @@ -118,7 +118,6 @@ class TestQoSWithL2Agent(base.BaseFullStackTestCase): def test_qos_policy_rule_lifecycle(self): new_limit = BANDWIDTH_LIMIT + 100 - new_burst = BANDWIDTH_BURST + 50 self.tenant_id = uuidutils.generate_uuid() self.network = self.safe_client.create_network(self.tenant_id, @@ -141,10 +140,15 @@ class TestQoSWithL2Agent(base.BaseFullStackTestCase): self.client.delete_bandwidth_limit_rule(rule['id'], qos_policy_id) _wait_for_rule_removed(vm) - # Create new rule + # Create new rule with no given burst value, in such case ovs and lb + # agent should apply burst value as + # bandwidth_limit * qos_consts.DEFAULT_BURST_RATE + new_expected_burst = int( + new_limit * qos_consts.DEFAULT_BURST_RATE + ) new_rule = self.safe_client.create_bandwidth_limit_rule( - self.tenant_id, qos_policy_id, new_limit, new_burst) - _wait_for_rule_applied(vm, new_limit, new_burst) + self.tenant_id, qos_policy_id, new_limit) + _wait_for_rule_applied(vm, new_limit, new_expected_burst) # Update qos policy rule id self.client.update_bandwidth_limit_rule( diff --git a/neutron/tests/unit/agent/l2/extensions/test_qos.py b/neutron/tests/unit/agent/l2/extensions/test_qos.py index 5d81a4d10be..2473dc71513 100644 --- a/neutron/tests/unit/agent/l2/extensions/test_qos.py +++ b/neutron/tests/unit/agent/l2/extensions/test_qos.py @@ -124,6 +124,14 @@ class QosAgentDriverTestCase(base.BaseTestCase): self.driver.create(self.port, self.policy) self.assertTrue(self.driver.create_bandwidth_limit.called) + def test__get_max_burst_value(self): + rule = self.rule + rule.max_burst_kbps = 0 + expected_burst = rule.max_kbps * qos_consts.DEFAULT_BURST_RATE + self.assertEqual( + expected_burst, self.driver._get_egress_burst_value(rule) + ) + class QosExtensionBaseTestCase(base.BaseTestCase): diff --git a/neutron/tests/unit/agent/linux/test_tc_lib.py b/neutron/tests/unit/agent/linux/test_tc_lib.py index 2fb656704e4..4a3fcf726ba 100644 --- a/neutron/tests/unit/agent/linux/test_tc_lib.py +++ b/neutron/tests/unit/agent/linux/test_tc_lib.py @@ -16,6 +16,7 @@ import mock from neutron.agent.linux import tc_lib +from neutron.services.qos import qos_consts from neutron.tests import base DEVICE_NAME = "tap_device" @@ -275,23 +276,23 @@ class TestTcCommand(base.BaseTestCase): extra_ok_codes=[2] ) - def test__get_filters_burst_value_burst_not_none(self): + def test_get_ingress_qdisc_burst_value_burst_not_none(self): self.assertEqual( - BURST, self.tc._get_filters_burst_value(BW_LIMIT, BURST) + BURST, self.tc.get_ingress_qdisc_burst_value(BW_LIMIT, BURST) ) - def test__get_filters_burst_no_burst_value_given(self): - expected_burst = BW_LIMIT * 0.8 + def test_get_ingress_qdisc_burst_no_burst_value_given(self): + expected_burst = BW_LIMIT * qos_consts.DEFAULT_BURST_RATE self.assertEqual( expected_burst, - self.tc._get_filters_burst_value(BW_LIMIT, None) + self.tc.get_ingress_qdisc_burst_value(BW_LIMIT, None) ) - def test__get_filters_burst_burst_value_zero(self): - expected_burst = BW_LIMIT * 0.8 + def test_get_ingress_qdisc_burst_burst_value_zero(self): + expected_burst = BW_LIMIT * qos_consts.DEFAULT_BURST_RATE self.assertEqual( expected_burst, - self.tc._get_filters_burst_value(BW_LIMIT, 0) + self.tc.get_ingress_qdisc_burst_value(BW_LIMIT, 0) ) def test__get_tbf_burst_value_when_burst_bigger_then_minimal(self): diff --git a/releasenotes/notes/set-of-default-qos-burst-value-0790773703fa08fc.yaml b/releasenotes/notes/set-of-default-qos-burst-value-0790773703fa08fc.yaml new file mode 100644 index 00000000000..94797ff707f --- /dev/null +++ b/releasenotes/notes/set-of-default-qos-burst-value-0790773703fa08fc.yaml @@ -0,0 +1,7 @@ +--- +prelude: > + By default, the QoS driver for the Open vSwitch + and Linuxbridge agents calculates the burst value + as 80% of the available bandwidth. +fixes: + - Fixes bug 1572670 \ No newline at end of file