From f766fc71bedc58a0cff0c6dc6e5576f0ab5e2507 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C5=82awek=20Kap=C5=82o=C5=84ski?= Date: Sat, 30 Apr 2016 00:53:22 +0200 Subject: [PATCH] Add setting default max_burst value if not given by user If user will not provide burst value for bandwidth limit rule there is need to set such value on appropriate to ensure that bandwith limit will work fine. To ensure at least proper limitation of TCP traffic we decided to set burst as 80% of bw_limit value. LinuxBridge agent already sets is like that. This patch add same behaviour for QoS extension driver in openvswitch L2 agent. DocImpact: Add setting default max_burst value in LB and OvS drivers Change-Id: Ib12a7cbf88cdffd10c8f6f8442582bf7377ca27d Closes-Bug: #1572670 --- neutron/agent/l2/extensions/qos.py | 10 ++++++++ neutron/agent/linux/tc_lib.py | 24 ++++++++++++------- .../agent/extension_drivers/qos_driver.py | 4 ++-- .../agent/extension_drivers/qos_driver.py | 5 +++- neutron/services/qos/qos_consts.py | 6 +++++ neutron/tests/fullstack/test_qos.py | 12 ++++++---- .../unit/agent/l2/extensions/test_qos.py | 8 +++++++ neutron/tests/unit/agent/linux/test_tc_lib.py | 17 ++++++------- ...ault-qos-burst-value-0790773703fa08fc.yaml | 7 ++++++ 9 files changed, 69 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/set-of-default-qos-burst-value-0790773703fa08fc.yaml 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