Merge pull request #2044 from sever-sever/T5295

T5295: Fix QoS shaper rate limit

1 files changed, 62 insertions, 52 deletions

diff --git a/python/vyos/qos/base.py b/python/vyos/qos/base.py
index 33bb8ae28..bd1676107 100644
--- a/python/vyos/qos/base.py
+++ b/python/vyos/qos/base.py
@@ -1,4 +1,4 @@
-# Copyright 2022 VyOS maintainers and contributors <maintainers@vyos.io>
+# Copyright 2022-2023 VyOS maintainers and contributors <maintainers@vyos.io>
 #
 # This library is free software; you can redistribute it and/or
 # modify it under the terms of the GNU Lesser General Public
@@ -243,60 +243,70 @@ class QoSBase:
                 # The police block allows limiting of the byte or packet rate of
                 # traffic matched by the filter it is attached to.
                 # https://man7.org/linux/man-pages/man8/tc-police.8.html
-                if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in cls_config):
-                    filter_cmd += f' action police'
 
-                if 'exceed' in cls_config:
-                    action = cls_config['exceed']
-                    filter_cmd += f' conform-exceed {action}'
-                    if 'not_exceed' in cls_config:
-                        action = cls_config['not_exceed']
-                        filter_cmd += f'/{action}'
-
-                if 'bandwidth' in cls_config:
-                    rate = self._rate_convert(cls_config['bandwidth'])
-                    filter_cmd += f' rate {rate}'
-
-                if 'burst' in cls_config:
-                    burst = cls_config['burst']
-                    filter_cmd += f' burst {burst}'
+                # T5295: We do not handle rate via tc filter directly,
+                # but rather set the tc filter to direct traffic to the correct tc class flow.
+                #
+                # if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in cls_config):
+                #     filter_cmd += f' action police'
+                #
+                # if 'exceed' in cls_config:
+                #     action = cls_config['exceed']
+                #     filter_cmd += f' conform-exceed {action}'
+                #     if 'not_exceed' in cls_config:
+                #         action = cls_config['not_exceed']
+                #         filter_cmd += f'/{action}'
+                #
+                # if 'bandwidth' in cls_config:
+                #     rate = self._rate_convert(cls_config['bandwidth'])
+                #     filter_cmd += f' rate {rate}'
+                #
+                # if 'burst' in cls_config:
+                #     burst = cls_config['burst']
+                #     filter_cmd += f' burst {burst}'
 
                 cls = int(cls)
                 filter_cmd += f' flowid {self._parent:x}:{cls:x}'
                 self._cmd(filter_cmd)
 
-        if 'default' in config:
-            if 'class' in config:
-                class_id_max = self._get_class_max_id(config)
-                default_cls_id = int(class_id_max) +1
-                self._build_base_qdisc(config['default'], default_cls_id)
-
-            filter_cmd = f'tc filter replace dev {self._interface} parent {self._parent:x}: '
-            filter_cmd += 'prio 255 protocol all basic'
-
-            # The police block allows limiting of the byte or packet rate of
-            # traffic matched by the filter it is attached to.
-            # https://man7.org/linux/man-pages/man8/tc-police.8.html
-            if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in config['default']):
-                filter_cmd += f' action police'
-
-            if 'exceed' in config['default']:
-                action = config['default']['exceed']
-                filter_cmd += f' conform-exceed {action}'
-                if 'not_exceed' in config['default']:
-                    action = config['default']['not_exceed']
-                    filter_cmd += f'/{action}'
-
-            if 'bandwidth' in config['default']:
-                rate = self._rate_convert(config['default']['bandwidth'])
-                filter_cmd += f' rate {rate}'
-
-            if 'burst' in config['default']:
-                burst = config['default']['burst']
-                filter_cmd += f' burst {burst}'
-
-            if 'class' in config:
-                filter_cmd += f' flowid {self._parent:x}:{default_cls_id:x}'
-
-            self._cmd(filter_cmd)
-
+        # T5295: Do not do any tc filter action for 'default'
+        # In VyOS 1.4, we have the following configuration:
+        # tc filter replace dev eth0 parent 1: prio 255 protocol all basic action police rate 300000000 burst 15k
+        # However, this caused unexpected random speeds.
+        # In VyOS 1.3, we do not use any 'tc filter' for rate limits,
+        # It gets rate from  tc class classid 1:1
+        #
+        # if 'default' in config:
+        #     if 'class' in config:
+        #         class_id_max = self._get_class_max_id(config)
+        #         default_cls_id = int(class_id_max) +1
+        #         self._build_base_qdisc(config['default'], default_cls_id)
+        #
+        #     filter_cmd = f'tc filter replace dev {self._interface} parent {self._parent:x}: '
+        #     filter_cmd += 'prio 255 protocol all basic'
+        #
+        #     # The police block allows limiting of the byte or packet rate of
+        #     # traffic matched by the filter it is attached to.
+        #     # https://man7.org/linux/man-pages/man8/tc-police.8.html
+        #     if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in config['default']):
+        #         filter_cmd += f' action police'
+        #
+        #     if 'exceed' in config['default']:
+        #         action = config['default']['exceed']
+        #         filter_cmd += f' conform-exceed {action}'
+        #         if 'not_exceed' in config['default']:
+        #             action = config['default']['not_exceed']
+        #             filter_cmd += f'/{action}'
+        #
+        #     if 'bandwidth' in config['default']:
+        #         rate = self._rate_convert(config['default']['bandwidth'])
+        #         filter_cmd += f' rate {rate}'
+        #
+        #     if 'burst' in config['default']:
+        #         burst = config['default']['burst']
+        #         filter_cmd += f' burst {burst}'
+        #
+        #     if 'class' in config:
+        #         filter_cmd += f' flowid {self._parent:x}:{default_cls_id:x}'
+        #
+        #     self._cmd(filter_cmd)