From 94d6e38fa08f8a52b6f18077c2a05a05b89b42b5 Mon Sep 17 00:00:00 2001 From: Weronika Sikora Date: Fri, 19 Jun 2020 17:16:29 +0200 Subject: [PATCH] Fix the wrong value for QoS rate conversion to bytes/s The rate value is converted to bytes per second before being sent to Pyroute2, but it used the wrong value for the calculations. This resulted in incorrect rates. It should be multiplied by 1000 (kbit), not 1024 (Kibit). The same applies to the burst value (kb). Change-Id: I70cb1fe651a50b2f6495d7a365a6beb2ba111c6d Closes-Bug: #1884273 --- neutron/agent/linux/tc_lib.py | 28 +++++++++---------- .../functional/agent/linux/test_tc_lib.py | 8 +++--- .../privileged/agent/linux/test_tc_lib.py | 8 +++--- neutron/tests/unit/agent/linux/test_tc_lib.py | 18 ++++++------ 4 files changed, 31 insertions(+), 31 deletions(-) diff --git a/neutron/agent/linux/tc_lib.py b/neutron/agent/linux/tc_lib.py index 6fad36debd6..f64c6f7c2b8 100644 --- a/neutron/agent/linux/tc_lib.py +++ b/neutron/agent/linux/tc_lib.py @@ -345,8 +345,8 @@ def add_tc_qdisc(device, qdisc_type, parent=None, handle=None, latency_ms=None, qdisc_type=qdisc_type, needed_arguments=['latency_ms', 'max_kbps', 'kernel_hz']) args['burst'] = int( - _get_tbf_burst_value(max_kbps, burst_kb, kernel_hz) * 1024 / 8) - args['rate'] = int(max_kbps * 1024 / 8) + _get_tbf_burst_value(max_kbps, burst_kb, kernel_hz) * 1000 / 8) + args['rate'] = int(max_kbps * 1000 / 8) args['latency'] = latency_ms * 1000 if parent: args['parent'] = rtnl.TC_H_ROOT if parent == 'root' else parent @@ -371,10 +371,10 @@ def list_tc_qdiscs(device, namespace=None): if qdisc_attrs['qdisc_type'] == 'tbf': tca_options = _get_attr(qdisc, 'TCA_OPTIONS') tca_tbf_parms = _get_attr(tca_options, 'TCA_TBF_PARMS') - qdisc_attrs['max_kbps'] = int(tca_tbf_parms['rate'] * 8 / 1024) + qdisc_attrs['max_kbps'] = int(tca_tbf_parms['rate'] * 8 / 1000) burst_bytes = _calc_burst(tca_tbf_parms['rate'], tca_tbf_parms['buffer']) - qdisc_attrs['burst_kb'] = int(burst_bytes * 8 / 1024) + qdisc_attrs['burst_kb'] = int(burst_bytes * 8 / 1000) qdisc_attrs['latency_ms'] = _calc_latency_ms( tca_tbf_parms['limit'], burst_bytes, tca_tbf_parms['rate']) retval.append(qdisc_attrs) @@ -427,10 +427,10 @@ def add_tc_policy_class(device, parent, classid, max_kbps, min_kbps=None, # - ceil (max bw): bytes/second # - burst: bytes # [1] https://www.systutorials.com/docs/linux/man/8-tc/ - kwargs = {'ceil': int(max_kbps * 1024 / 8), - 'burst': int(burst_kb * 1024 / 8)} + kwargs = {'ceil': int(max_kbps * 1000 / 8), + 'burst': int(burst_kb * 1000 / 8)} - rate = int((min_kbps or 0) * 1024 / 8) + rate = int((min_kbps or 0) * 1000 / 8) min_rate = _calc_min_rate(kwargs['burst']) if min_rate > rate: LOG.warning('TC HTB class policy rate %(rate)s (bytes/second) is ' @@ -460,9 +460,9 @@ def list_tc_policy_class(device, namespace=None): tca_params = _get_attr(tca_options, 'TCA_' + qdisc_type.upper() + '_PARMS') burst_kb = int( - _calc_burst(tca_params['rate'], tca_params['buffer']) * 8 / 1024) - max_kbps = int(tca_params['ceil'] * 8 / 1024) - min_kbps = int(tca_params['rate'] * 8 / 1024) + _calc_burst(tca_params['rate'], tca_params['buffer']) * 8 / 1000) + max_kbps = int(tca_params['ceil'] * 8 / 1000) + min_kbps = int(tca_params['rate'] * 8 / 1000) return max_kbps, min_kbps, burst_kb tc_classes = priv_tc_lib.list_tc_policy_classes(device, @@ -562,8 +562,8 @@ def add_tc_filter_policy(device, parent, rate_kbps, burst_kb, mtu, action, :param namespace: (string) (optional) namespace name """ - rate = int(rate_kbps * 1024 / 8) - burst = int(burst_kb * 1024 / 8) + rate = int(rate_kbps * 1000 / 8) + burst = int(burst_kb * 1000 / 8) priv_tc_lib.add_tc_filter_policy(device, parent, priority, rate, burst, mtu, action, protocol=protocol, namespace=namespace) @@ -604,10 +604,10 @@ def list_tc_filters(device, parent, namespace=None): if tca_u32_police: tca_police_tbf = _get_attr(tca_u32_police, 'TCA_POLICE_TBF') if tca_police_tbf: - value['rate_kbps'] = int(tca_police_tbf['rate'] * 8 / 1024) + value['rate_kbps'] = int(tca_police_tbf['rate'] * 8 / 1000) value['burst_kb'] = int( _calc_burst(tca_police_tbf['rate'], - tca_police_tbf['burst']) * 8 / 1024) + tca_police_tbf['burst']) * 8 / 1000) value['mtu'] = tca_police_tbf['mtu'] retval.append(value) diff --git a/neutron/tests/functional/agent/linux/test_tc_lib.py b/neutron/tests/functional/agent/linux/test_tc_lib.py index f0211b145c7..f91b56118bb 100644 --- a/neutron/tests/functional/agent/linux/test_tc_lib.py +++ b/neutron/tests/functional/agent/linux/test_tc_lib.py @@ -166,17 +166,17 @@ class TcPolicyClassTestCase(functional_base.BaseSudoTestCase): max_kbps=2000, burst_kb=1000, min_kbps=3, namespace=self.ns[0]) mock_log.warning.assert_called_once_with( - *warning_args(3 * 128, tc_lib._calc_min_rate(1000 * 128))) + *warning_args(3 * 125, tc_lib._calc_min_rate(1000 * 125))) - # rate < min_rate: min_rate = 466 with burst = 0.8 ceil = 256000 + # rate < min_rate: min_rate = 455 with burst = 0.8 ceil = 256000 mock_log.reset_mock() tc_lib.add_tc_policy_class(self.device[0], '1:', '1:10', max_kbps=2000, burst_kb=None, min_kbps=5, namespace=self.ns[0]) min_rate = tc_lib._calc_min_rate(qos_consts.DEFAULT_BURST_RATE * - 2000 * 128) + 2000 * 125) mock_log.warning.assert_called_once_with( - *warning_args(5 * 128, min_rate)) + *warning_args(5 * 125, min_rate)) class TcFiltersTestCase(functional_base.BaseSudoTestCase): diff --git a/neutron/tests/functional/privileged/agent/linux/test_tc_lib.py b/neutron/tests/functional/privileged/agent/linux/test_tc_lib.py index a81128172a5..47a76b29170 100644 --- a/neutron/tests/functional/privileged/agent/linux/test_tc_lib.py +++ b/neutron/tests/functional/privileged/agent/linux/test_tc_lib.py @@ -279,8 +279,8 @@ class TcFilterClassTestCase(functional_base.BaseSudoTestCase): namespace=self.namespace) # NOTE(ralonsoh): - # - rate: 320000 bytes/sec (pyroute2 units) = 2500 kbits/sec (OS units) - # - burst: 192000 bytes/sec = 1500 kbits/sec + # - rate: 320000 bytes/sec (pyroute2 units) = 2560 kbits/sec (OS units) + # - burst: 192000 bytes/sec = 1536 kbits/sec priv_tc_lib.add_tc_filter_policy( self.device, 'ffff:', 49, 320000, 192000, 1200, 'drop', namespace=self.namespace) @@ -288,6 +288,6 @@ class TcFilterClassTestCase(functional_base.BaseSudoTestCase): filters = tc_lib.list_tc_filters( self.device, 'ffff:', namespace=self.namespace) self.assertEqual(1, len(filters)) - self.assertEqual(2500, filters[0]['rate_kbps']) - self.assertEqual(1500, filters[0]['burst_kb']) + self.assertEqual(2560, filters[0]['rate_kbps']) + self.assertEqual(1536, filters[0]['burst_kb']) self.assertEqual(1200, filters[0]['mtu']) diff --git a/neutron/tests/unit/agent/linux/test_tc_lib.py b/neutron/tests/unit/agent/linux/test_tc_lib.py index a379ed532de..0c98df29f15 100644 --- a/neutron/tests/unit/agent/linux/test_tc_lib.py +++ b/neutron/tests/unit/agent/linux/test_tc_lib.py @@ -271,9 +271,9 @@ class TcTestCase(base.BaseTestCase): tc_lib.add_tc_qdisc('device', 'tbf', parent='root', max_kbps=10000, burst_kb=1500, latency_ms=70, kernel_hz=250, namespace=self.namespace) - burst = tc_lib._get_tbf_burst_value(10000, 1500, 70) * 1024 / 8 + burst = tc_lib._get_tbf_burst_value(10000, 1500, 70) * 1000 / 8 self.mock_add_tc_qdisc.assert_called_once_with( - 'device', parent=rtnl.TC_H_ROOT, kind='tbf', rate=10000 * 128, + 'device', parent=rtnl.TC_H_ROOT, kind='tbf', rate=10000 * 125, burst=burst, latency=70000, namespace=self.namespace) def test_add_tc_qdisc_tbf_missing_arguments(self): @@ -318,8 +318,8 @@ class TcTestCase(base.BaseTestCase): self.assertEqual('root', qdiscs[0]['parent']) self.assertEqual('5:1', qdiscs[0]['handle']) self.assertEqual('tbf', qdiscs[0]['qdisc_type']) - self.assertEqual(2500, qdiscs[0]['max_kbps']) - self.assertEqual(1500, qdiscs[0]['burst_kb']) + self.assertEqual(2560, qdiscs[0]['max_kbps']) + self.assertEqual(1536, qdiscs[0]['burst_kb']) self.assertEqual(50, qdiscs[0]['latency_ms']) def test__get_tbf_burst_value_when_burst_bigger_then_minimal(self): @@ -346,8 +346,8 @@ class TcPolicyClassTestCase(base.BaseTestCase): 'device', 'root', '1:10', min_kbps=1000, max_kbps=2000, burst_kb=1600, namespace=self.namespace) self.mock_add_tc_policy_class.assert_called_once_with( - 'device', rtnl.TC_H_ROOT, '1:10', 'htb', rate=1000 * 128, - ceil=2000 * 128, burst=1600 * 128, namespace=self.namespace) + 'device', rtnl.TC_H_ROOT, '1:10', 'htb', rate=1000 * 125, + ceil=2000 * 125, burst=1600 * 125, namespace=self.namespace) @mock.patch('pyroute2.netlink.rtnl.tcmsg.common.tick_in_usec', 15.625) def test_list_tc_policy_classes(self): @@ -367,9 +367,9 @@ class TcPolicyClassTestCase(base.BaseTestCase): 'parent': 'root', 'classid': '1:1', 'qdisc_type': 'htb', - 'min_kbps': 1500, - 'max_kbps': 2000, - 'burst_kb': 1200} + 'min_kbps': 1536, + 'max_kbps': 2048, + 'burst_kb': 1228} self.assertEqual(reference, _class)