Merge "Add setting default max_burst value if not given by user"

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)
+            self.tenant_id, qos_policy_id, new_limit)
-        _wait_for_rule_applied(vm, new_limit, new_burst)
+        _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):
+    def test_get_ingress_qdisc_burst_no_burst_value_given(self):
-        expected_burst = BW_LIMIT * 0.8
+        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):
+    def test_get_ingress_qdisc_burst_burst_value_zero(self):
-        expected_burst = BW_LIMIT * 0.8
+        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