qos: T4284: replace qdisc/class instead of always adding it

This makes transitions/updates faster and less error prone

6 files changed, 38 insertions, 31 deletions

diff --git a/python/vyos/qos/base.py b/python/vyos/qos/base.py
index 96f189b1a..28635b5e7 100644
--- a/python/vyos/qos/base.py
+++ b/python/vyos/qos/base.py
@@ -13,17 +13,21 @@
 # You should have received a copy of the GNU Lesser General Public
 # License along with this library.  If not, see <http://www.gnu.org/licenses/>.
 
+import os
+
 from vyos.base import Warning
 from vyos.util import cmd
 from vyos.util import dict_search
 from vyos.util import read_file
 
 class QoSBase:
-    _debug = True
+    _debug = False
     _direction = ['egress']
     _parent = 0xffff
 
     def __init__(self, interface):
+        if os.path.exists('/tmp/vyos.qos.debug'):
+            self._debug = True
         self._interface = interface
 
     def _cmd(self, command):
@@ -41,7 +45,7 @@ class QoSBase:
             return tmp[-1]
         return None
 
-    def _tmp_qdisc(self, config : dict, cls_id : int):
+    def _build_base_qdisc(self, config : dict, cls_id : int):
         """
         Add/replace qdisc for every class (also default is a class). This is
         a genetic method which need an implementation "per" queue-type.
@@ -143,8 +147,7 @@ class QoSBase:
 
         if 'class' in config:
             for cls, cls_config in config['class'].items():
-
-                self._tmp_qdisc(cls_config, int(cls))
+                self._build_base_qdisc(cls_config, int(cls))
 
                 if 'match' in cls_config:
                     for match, match_config in cls_config['match'].items():
@@ -243,11 +246,10 @@ class QoSBase:
                                 self._cmd(filter_cmd)
 
         if 'default' in config:
-            class_id_max = self._get_class_max_id(config)
-            default_cls_id = int(class_id_max) +1
-
-            if 'default' in config:
-                self._tmp_qdisc(config['default'], default_cls_id)
+            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'
@@ -255,25 +257,26 @@ 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):
+            if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in config['default']):
                 filter_cmd += f' action police'
 
-            if 'exceed' in cls_config:
-                action = cls_config['exceed']
+            if 'exceed' in config['default']:
+                action = config['default']['exceed']
                 filter_cmd += f' conform-exceed {action}'
-                if 'not_exceed' in cls_config:
-                    action = cls_config['not_exceed']
+                if 'not_exceed' in config['default']:
+                    action = config['default']['not_exceed']
                     filter_cmd += f'/{action}'
 
-            if 'bandwidth' in cls_config:
-                rate = self._rate_convert(cls_config['bandwidth'])
+            if 'bandwidth' in config['default']:
+                rate = self._rate_convert(config['default']['bandwidth'])
                 filter_cmd += f' rate {rate}'
 
-            if 'burst' in cls_config:
-                burst = cls_config['burst']
+            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}'
 
-            filter_cmd += f' flowid {self._parent:x}:{default_cls_id:x}'
             self._cmd(filter_cmd)
 
diff --git a/python/vyos/qos/priority.py b/python/vyos/qos/priority.py
index 72092b7ef..6d4a60a43 100644
--- a/python/vyos/qos/priority.py
+++ b/python/vyos/qos/priority.py
@@ -33,6 +33,7 @@ class Priority(QoSBase):
             self._cmd(tmp)
 
             for cls in config['class']:
+                cls = int(cls)
                 tmp = f'tc qdisc add dev {self._interface} parent {self._parent:x}:{cls:x} pfifo'
                 self._cmd(tmp)
 
diff --git a/python/vyos/qos/roundrobin.py b/python/vyos/qos/roundrobin.py
index 4a0cb18aa..80814ddfb 100644
--- a/python/vyos/qos/roundrobin.py
+++ b/python/vyos/qos/roundrobin.py
@@ -26,10 +26,10 @@ class RoundRobin(QoSBase):
         if 'class' in config:
             for cls in config['class']:
                 cls = int(cls)
-                tmp = f'tc class add dev {self._interface} parent 1:1 classid 1:{cls:x} drr'
+                tmp = f'tc class replace dev {self._interface} parent 1:1 classid 1:{cls:x} drr'
                 self._cmd(tmp)
 
-                tmp = f'tc qdisc add dev {self._interface} parent 1:{cls:x} pfifo'
+                tmp = f'tc qdisc replace dev {self._interface} parent 1:{cls:x} pfifo'
                 self._cmd(tmp)
 
         if 'default' in config:
@@ -37,7 +37,7 @@ class RoundRobin(QoSBase):
             default_cls_id = int(class_id_max) +1
 
             # class ID via CLI is in range 1-4095, thus 1000 hex = 4096
-            tmp = f'tc class add dev {self._interface} parent 1:1 classid 1:{default_cls_id:x} drr'
+            tmp = f'tc class replace dev {self._interface} parent 1:1 classid 1:{default_cls_id:x} drr'
             self._cmd(tmp)
 
         # call base class
diff --git a/python/vyos/qos/trafficshaper.py b/python/vyos/qos/trafficshaper.py
index 6d465b38e..f42f4d022 100644
--- a/python/vyos/qos/trafficshaper.py
+++ b/python/vyos/qos/trafficshaper.py
@@ -39,7 +39,6 @@ class TrafficShaper(QoSBase):
         # need a bigger r2q if going fast than 16 mbits/sec
         if (speed_bps // r2q) >= MAXQUANTUM: # integer division
             r2q = ceil(speed_bps // MAXQUANTUM)
-            print(r2q)
         else:
             # if there is a slow class then may need smaller value
             if 'class' in config:
@@ -59,10 +58,10 @@ class TrafficShaper(QoSBase):
 
 
         default_minor_id = int(class_id_max) +1
-        tmp = f'tc qdisc add dev {self._interface} root handle {self._parent:x}: htb r2q {r2q} default {default_minor_id:x}' # default is in hex
+        tmp = f'tc qdisc replace dev {self._interface} root handle {self._parent:x}: htb r2q {r2q} default {default_minor_id:x}' # default is in hex
         self._cmd(tmp)
 
-        tmp = f'tc class add dev {self._interface} parent {self._parent:x}: classid {self._parent:x}:1 htb rate {speed}'
+        tmp = f'tc class replace dev {self._interface} parent {self._parent:x}: classid {self._parent:x}:1 htb rate {speed}'
         self._cmd(tmp)
 
         if 'class' in config:
@@ -75,23 +74,26 @@ class TrafficShaper(QoSBase):
                 burst = cls_config['burst']
                 quantum = cls_config['codel_quantum']
 
-                tmp = f'tc class add dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{cls:x} htb rate {rate} burst {burst} quantum {quantum}'
+                tmp = f'tc class replace dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{cls:x} htb rate {rate} burst {burst} quantum {quantum}'
                 if 'priority' in cls_config:
                     priority = cls_config['priority']
                     tmp += f' prio {priority}'
                 self._cmd(tmp)
 
-                tmp = f'tc qdisc add dev {self._interface} parent {self._parent:x}:{cls:x} sfq'
+                tmp = f'tc qdisc replace dev {self._interface} parent {self._parent:x}:{cls:x} sfq'
                 self._cmd(tmp)
 
         if 'default' in config:
-                tmp = f'tc class add dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{default_minor_id:x} htb rate {rate} burst {burst} quantum {quantum}'
+                rate = self._rate_convert(config['default']['bandwidth'])
+                burst = config['default']['burst']
+                quantum = config['default']['codel_quantum']
+                tmp = f'tc class replace dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{default_minor_id:x} htb rate {rate} burst {burst} quantum {quantum}'
                 if 'priority' in config['default']:
                     priority = config['default']['priority']
                     tmp += f' prio {priority}'
                 self._cmd(tmp)
 
-                tmp = f'tc qdisc add dev {self._interface} parent {self._parent:x}:{default_minor_id:x} sfq'
+                tmp = f'tc qdisc replace dev {self._interface} parent {self._parent:x}:{default_minor_id:x} sfq'
                 self._cmd(tmp)
 
         # call base class
diff --git a/smoketest/scripts/cli/test_qos.py b/smoketest/scripts/cli/test_qos.py
index d1fa3d07b..c2fc71e04 100755
--- a/smoketest/scripts/cli/test_qos.py
+++ b/smoketest/scripts/cli/test_qos.py
@@ -282,8 +282,6 @@ class TestQoS(VyOSUnitTestSHIM.TestCase):
         # commit changes
         self.cli_commit()
 
-        self.skipTest('iproute2 bug - invalid JSON')
-
         for interface in self._interfaces:
             for filter in get_tc_filter_json(interface, 'ingress'):
                 # bail out early if filter has no attached action
diff --git a/src/conf_mode/qos.py b/src/conf_mode/qos.py
index 2eb03237c..1fe3b6aa9 100755
--- a/src/conf_mode/qos.py
+++ b/src/conf_mode/qos.py
@@ -187,6 +187,9 @@ def verify(qos):
                                 if queue_lim < max_tr:
                                     raise ConfigError(f'Policy "{policy}" uses queue-limit "{queue_lim}" < max-threshold "{max_tr}"!')
 
+                if 'default' in policy_config:
+                    if 'bandwidth' not in policy_config['default']:
+                        raise ConfigError('Bandwidth not defined for default traffic!')
 
     # we should check interface ingress/egress configuration after verifying that
     # the policy name is used only once - this makes the logic easier!