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

@@ -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):

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:

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

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,

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

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(

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):
 

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):

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