Merge pull request #3401 from vyos/mergify/bp/sagitta/pr-3400

qos: T6225: Fix QoS random-detect policy (backport #3400)

4 files changed, 36 insertions, 40 deletions

diff --git a/python/vyos/qos/base.py b/python/vyos/qos/base.py
index 4173a1a43..87927ba9d 100644
--- a/python/vyos/qos/base.py
+++ b/python/vyos/qos/base.py
@@ -90,13 +90,14 @@ class QoSBase:
         else:
             return value
 
-    def _calc_random_detect_queue_params(self, avg_pkt, max_thr, limit=None, min_thr=None, mark_probability=None):
+    def _calc_random_detect_queue_params(self, avg_pkt, max_thr, limit=None, min_thr=None,
+                                         mark_probability=None, precedence=0):
         params = dict()
         avg_pkt = int(avg_pkt)
         max_thr = int(max_thr)
         mark_probability = int(mark_probability)
         limit = int(limit) if limit else 4 * max_thr
-        min_thr = int(min_thr) if min_thr else (9 * max_thr) // 18
+        min_thr = int(min_thr) if min_thr else ((9 + precedence) * max_thr) // 18
 
         params['avg_pkt'] = avg_pkt
         params['limit'] = limit * avg_pkt
diff --git a/python/vyos/qos/randomdetect.py b/python/vyos/qos/randomdetect.py
index d7d84260f..a3a39da36 100644
--- a/python/vyos/qos/randomdetect.py
+++ b/python/vyos/qos/randomdetect.py
@@ -21,33 +21,25 @@ class RandomDetect(QoSBase):
     # https://man7.org/linux/man-pages/man8/tc.8.html
     def update(self, config, direction):
 
-        tmp = f'tc qdisc add dev {self._interface} root handle {self._parent}:0 dsmark indices 8 set_tc_index'
+        # # Generalized Random Early Detection
+        handle = self._parent
+        tmp = f'tc qdisc add dev {self._interface} root handle {self._parent}:0 gred setup DPs 8 default 0 grio'
         self._cmd(tmp)
-
-        tmp = f'tc filter add dev {self._interface} parent {self._parent}:0 protocol ip prio 1 tcindex mask 0xe0 shift 5'
-        self._cmd(tmp)
-
-        # Generalized Random Early Detection
-        handle = self._parent +1
-        tmp = f'tc qdisc add dev {self._interface} parent {self._parent}:0 handle {handle}:0 gred setup DPs 8 default 0 grio'
-        self._cmd(tmp)
-
         bandwidth = self._rate_convert(config['bandwidth'])
 
         # set VQ (virtual queue) parameters
         for precedence, precedence_config in config['precedence'].items():
             precedence = int(precedence)
-            avg_pkt = int(precedence_config['average_packet'])
-            limit = int(precedence_config['queue_limit']) * avg_pkt
-            min_val = int(precedence_config['minimum_threshold']) * avg_pkt
-            max_val = int(precedence_config['maximum_threshold']) * avg_pkt
-
-            tmp  = f'tc qdisc change dev {self._interface} handle {handle}:0 gred limit {limit} min {min_val} max {max_val} avpkt {avg_pkt} '
-
-            burst = (2 * int(precedence_config['minimum_threshold']) + int(precedence_config['maximum_threshold'])) // 3
-            probability = 1 / int(precedence_config['mark_probability'])
-            tmp += f'burst {burst} bandwidth {bandwidth} probability {probability} DP {precedence} prio {8 - precedence:x}'
-
+            qparams = self._calc_random_detect_queue_params(
+                avg_pkt=precedence_config.get('average_packet'),
+                max_thr=precedence_config.get('maximum_threshold'),
+                limit=precedence_config.get('queue_limit'),
+                min_thr=precedence_config.get('minimum_threshold'),
+                mark_probability=precedence_config.get('mark_probability'),
+                precedence=precedence
+            )
+            tmp = f'tc qdisc change dev {self._interface} handle {handle}:0 gred limit {qparams["limit"]} min {qparams["min_val"]} max {qparams["max_val"]} avpkt {qparams["avg_pkt"]} '
+            tmp += f'burst {qparams["burst"]} bandwidth {bandwidth} probability {qparams["probability"]} DP {precedence} prio {8 - precedence:x}'
             self._cmd(tmp)
 
         # call base class
diff --git a/smoketest/scripts/cli/test_qos.py b/smoketest/scripts/cli/test_qos.py
index fef1ff23a..bcf5139c7 100755
--- a/smoketest/scripts/cli/test_qos.py
+++ b/smoketest/scripts/cli/test_qos.py
@@ -441,7 +441,6 @@ class TestQoS(VyOSUnitTestSHIM.TestCase):
         self.cli_commit()
 
     def test_08_random_detect(self):
-        self.skipTest('tc returns invalid JSON here - needs iproute2 fix')
         bandwidth = 5000
 
         first = True
@@ -467,8 +466,11 @@ class TestQoS(VyOSUnitTestSHIM.TestCase):
         bandwidth = 5000
         for interface in self._interfaces:
             tmp = get_tc_qdisc_json(interface)
-            import pprint
-            pprint.pprint(tmp)
+            self.assertTrue('gred' in tmp.get('kind'))
+            self.assertEqual(8, len(tmp.get('options', {}).get('vqs')))
+            self.assertEqual(8, tmp.get('options', {}).get('dp_cnt'))
+            self.assertEqual(0, tmp.get('options', {}).get('dp_default'))
+            self.assertTrue(tmp.get('options', {}).get('grio'))
 
     def test_09_rate_control(self):
         bandwidth = 5000
diff --git a/src/conf_mode/qos.py b/src/conf_mode/qos.py
index ccfc8f6b8..8a590cbc6 100755
--- a/src/conf_mode/qos.py
+++ b/src/conf_mode/qos.py
@@ -39,6 +39,9 @@ from vyos.utils.dict import dict_search_recursive
 from vyos.utils.process import run
 from vyos import ConfigError
 from vyos import airbag
+from vyos.xml_ref import relative_defaults
+
+
 airbag.enable()
 
 map_vyops_tc = {
@@ -115,8 +118,18 @@ def get_config(config=None):
             for rd_name in list(qos['policy'][policy]):
                 # There are eight precedence levels - ensure all are present
                 # to be filled later down with the appropriate default values
-                default_precedence = {'precedence' : { '0' : {}, '1' : {}, '2' : {}, '3' : {},
-                                                       '4' : {}, '5' : {}, '6' : {}, '7' : {} }}
+                default_p_val = relative_defaults(
+                    ['qos', 'policy', 'random-detect', rd_name, 'precedence'],
+                    {'precedence': {'0': {}}},
+                    get_first_key=True, recursive=True
+                )['0']
+                default_p_val = {key.replace('-', '_'): value for key, value in default_p_val.items()}
+                default_precedence = {
+                    'precedence': {'0': default_p_val, '1': default_p_val,
+                                   '2': default_p_val, '3': default_p_val,
+                                   '4': default_p_val, '5': default_p_val,
+                                   '6': default_p_val, '7': default_p_val}}
+
                 qos['policy']['random_detect'][rd_name] = dict_merge(
                     default_precedence, qos['policy']['random_detect'][rd_name])
 
@@ -124,18 +137,6 @@ def get_config(config=None):
 
     for policy in qos.get('policy', []):
         for p_name, p_config in qos['policy'][policy].items():
-            if 'precedence' in p_config:
-                # precedence settings are a bit more complex as they are
-                # calculated under specific circumstances:
-                for precedence in p_config['precedence']:
-                    max_thr = int(qos['policy'][policy][p_name]['precedence'][precedence]['maximum_threshold'])
-                    if 'minimum_threshold' not in qos['policy'][policy][p_name]['precedence'][precedence]:
-                        qos['policy'][policy][p_name]['precedence'][precedence]['minimum_threshold'] = str(
-                            int((9 + int(precedence)) * max_thr) // 18);
-
-                    if 'queue_limit' not in qos['policy'][policy][p_name]['precedence'][precedence]:
-                        qos['policy'][policy][p_name]['precedence'][precedence]['queue_limit'] = \
-                            str(int(4 * max_thr))
             # cleanup empty match config
             if 'class' in p_config:
                 for cls, cls_config in p_config['class'].items():