T6806: Rework QoS Policy for HFSC Shaper (#4181)

- Removed default `m1` and `m2` values from interface definitions
- Adjusted filter priorities for shapers
- Fixed SFQ qdisc and HFSC class creation to fully support `m1`, `d`, and `m2` parameters
- Added validation logic similar to VyOS 1.3 to improve error handling and user experience

interface-definitions/include/qos/hfsc-m1.xml.i

@@ -27,6 +27,5 @@
       <description>bps(8),kbps(8*10^3),mbps(8*10^6), gbps, tbps - Byte/sec</description>
     </valueHelp>
   </properties>
-  <defaultValue>0bit</defaultValue>
 </leafNode>
 <!-- include end -->

interface-definitions/include/qos/hfsc-m2.xml.i

@@ -27,6 +27,5 @@
       <description>bps(8),kbps(8*10^3),mbps(8*10^6), gbps, tbps - Byte/sec</description>
     </valueHelp>
   </properties>
-  <defaultValue>100%</defaultValue>
 </leafNode>
 <!-- include end -->

python/vyos/qos/base.py

@@ -259,7 +259,7 @@ def update(self, config, direction, priority=None):
                                     has_filter = True
                                     break
 
-                        if self.qostype == 'shaper' and 'prio ' not in filter_cmd:
+                        if self.qostype in ['shaper', 'shaper_hfsc'] and 'prio ' not in filter_cmd:
                             filter_cmd += f' prio {index}'
                         if 'mark' in match_config:
                             mark = match_config['mark']

python/vyos/qos/trafficshaper.py

@@ -126,91 +126,71 @@ def update(self, config, direction):
         # call base class
         super().update(config, direction)
 
+
 class TrafficShaperHFSC(QoSBase):
+    """
+    Traffic shaper using Hierarchical Fair Service Curve (HFSC).
+    Documentation: https://man7.org/linux/man-pages/man8/tc-hfsc.8.html
+    """
+
     _parent = 1
     qostype = 'shaper_hfsc'
 
-    # https://man7.org/linux/man-pages/man8/tc-hfsc.8.html
-    def update(self, config, direction):
-        class_id_max = 0
-        if 'class' in config:
-            tmp = list(config['class'])
-            tmp.sort()
-            class_id_max = tmp[-1]
+    criteria = ['linkshare', 'realtime', 'upperlimit']
+    short_criterion = {
+        'linkshare': 'ls',
+        'realtime': 'rt',
+        'upperlimit': 'ul',
+    }
+
+    def _gen_class(self, cls: int, cls_config: dict):
+        """
+        Generate HFSC class and add Stochastic Fair Queueing (SFQ) qdisc.
+
+        Args:
+            cls (int): Class ID
+            cls_config (dict): Configuration for the class
+        """
+        tmp = f'tc class replace dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{cls:x} hfsc'
+
+        for crit in self.criteria:
+            param = cls_config.get(crit)
+            if param:
+                tmp += (
+                    f' {self.short_criterion[crit]}'
+                    f' m1 {self._rate_convert(param["m1"]) if param.get("m1") else 0}'
+                    f' d {param.get("d", 0)}ms'
+                    f' m2 {self._rate_convert(param["m2"])}'
+                )
 
-        r2q = 10
-        # bandwidth is a mandatory CLI node
-        speed = self._rate_convert(config['bandwidth'])
-        speed_bps = int(speed) // 8
+        self._cmd(tmp)
 
-        # need a bigger r2q if going fast than 16 mbits/sec
-        if (speed_bps // r2q) >= MAXQUANTUM: # integer division
-            r2q = ceil(speed_bps // MAXQUANTUM)
-        else:
-            # if there is a slow class then may need smaller value
-            if 'class' in config:
-                min_speed = speed_bps
-                for cls, cls_options in config['class'].items():
-                    # find class with the lowest bandwidth used
-                    if 'bandwidth' in cls_options:
-                        bw_bps = int(self._rate_convert(cls_options['bandwidth'])) // 8 # bandwidth in bytes per second
-                        if bw_bps < min_speed:
-                            min_speed = bw_bps
+        tmp = f'tc qdisc replace dev {self._interface} parent {self._parent:x}:{cls:x} sfq perturb 10'
+        self._cmd(tmp)
 
-                while (r2q > 1) and (min_speed // r2q) < MINQUANTUM:
-                    tmp = r2q -1
-                    if (speed_bps // tmp) >= MAXQUANTUM:
-                        break
-                    r2q = tmp
+    def update(self, config, direction):
+        class_id_max = self._get_class_max_id(config)
+        default_cls_id = int(class_id_max) + 1 if class_id_max else 2
 
-        default_minor_id = int(class_id_max) +1
-        tmp = f'tc qdisc replace dev {self._interface} root handle {self._parent:x}: hfsc default {default_minor_id:x}' # default is in hex
+        speed = self._rate_convert(config['bandwidth'])
+
+        tmp = f'tc qdisc replace dev {self._interface} root handle {self._parent:x}: hfsc default {default_cls_id:x}'  # default is in hex
         self._cmd(tmp)
 
         tmp = f'tc class replace dev {self._interface} parent {self._parent:x}: classid {self._parent:x}:1 hfsc sc rate {speed} ul rate {speed}'
         self._cmd(tmp)
 
+        # tmp = f'tc qdisc add dev {self._interface} parent {self._parent:x}:1 handle f1: sfq perturb 10'
+        # self._cmd(tmp)
+
         if 'class' in config:
             for cls, cls_config in config['class'].items():
-                # class id is used later on and passed as hex, thus this needs to be an int
-                cls = int(cls)
-                # ls m1
-                if cls_config.get('linkshare', {}).get('m1').endswith('%'):
-                    percent = cls_config['linkshare']['m1'].rstrip('%')
-                    m_one_rate = self._rate_convert(config['bandwidth']) * int(percent) // 100
-                else:
-                    m_one_rate = cls_config['linkshare']['m1']
-                # ls m2
-                if cls_config.get('linkshare', {}).get('m2').endswith('%'):
-                    percent = cls_config['linkshare']['m2'].rstrip('%')
-                    m_two_rate = self._rate_convert(config['bandwidth']) * int(percent) // 100
-                else:
-                    m_two_rate = self._rate_convert(cls_config['linkshare']['m2'])
-
-                tmp = f'tc class replace dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{cls:x} hfsc ls m1 {m_one_rate} m2 {m_two_rate} '
-                self._cmd(tmp)
-
-                tmp = f'tc qdisc replace dev {self._interface} parent {self._parent:x}:{cls:x} sfq perturb 10'
-                self._cmd(tmp)
+                self._gen_class(cls=int(cls), cls_config=cls_config)
 
         if 'default' in config:
-            # ls m1
-            if config.get('default', {}).get('linkshare', {}).get('m1').endswith('%'):
-                percent = config['default']['linkshare']['m1'].rstrip('%')
-                m_one_rate = self._rate_convert(config['default']['linkshare']['m1']) * int(percent) // 100
-            else:
-                m_one_rate = config['default']['linkshare']['m1']
-            # ls m2
-            if config.get('default', {}).get('linkshare', {}).get('m2').endswith('%'):
-                percent = config['default']['linkshare']['m2'].rstrip('%')
-                m_two_rate = self._rate_convert(config['default']['linkshare']['m2']) * int(percent) // 100
-            else:
-                m_two_rate = self._rate_convert(config['default']['linkshare']['m2'])
-            tmp = f'tc class replace dev {self._interface} parent {self._parent:x}:1 classid {self._parent:x}:{default_minor_id:x} hfsc ls m1 {m_one_rate} m2 {m_two_rate} '
-            self._cmd(tmp)
-
-            tmp = f'tc qdisc replace dev {self._interface} parent {self._parent:x}:{default_minor_id:x} sfq perturb 10'
-            self._cmd(tmp)
+            self._gen_class(
+                cls=int(default_cls_id), cls_config=config.get('default', {})
+            )
 
         # call base class
         super().update(config, direction)

smoketest/scripts/cli/test_qos.py

@@ -985,6 +985,137 @@ def test_20_round_robin_policy_default(self):
             tmp[2]['options'],
         )
 
+    def test_21_shaper_hfsc(self):
+        interface = self._interfaces[0]
+        policy_name = f'qos-policy-{interface}'
+        ul = {
+            'm1': '100kbit',
+            'm2': '150kbit',
+            'd': '100',
+        }
+        ls = {'m2': '120kbit'}
+        rt = {
+            'm1': '110kbit',
+            'm2': '130kbit',
+            'd': '75',
+        }
+        self.cli_set(base_path + ['interface', interface, 'egress', policy_name])
+        self.cli_set(base_path + ['policy', 'shaper-hfsc', policy_name])
+
+        # Policy {policy_name} misses "default" class!
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'upperlimit']
+        )
+
+        # At least one m2 value needs to be set for class: {class_name}
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'upperlimit', 'm1', ul['m1']]
+        )
+        # {class_name} upperlimit m1 value is set, but no m2 was found!
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'upperlimit', 'm2', ul['m2']]
+        )
+        # {class_name} upperlimit m1 value is set, but no d was found!
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'upperlimit', 'd', ul['d']]
+        )
+        # Linkshare m2 needs to be defined to use upperlimit m2 for class: {class_name}
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'linkshare', 'm2', ls['m2']]
+        )
+        self.cli_commit()
+
+        # use raw because tc json is incorrect here
+        tmp = cmd(f'tc -details qdisc show dev {interface}')
+        for rec in tmp.split('\n'):
+            rec = rec.strip()
+            if 'root' in rec:
+                self.assertEqual(rec, 'qdisc hfsc 1: root refcnt 2 default 2')
+            else:
+                self.assertRegex(
+                    rec,
+                    r'qdisc sfq \S+: parent 1:2 limit 127p quantum 1514b depth 127 flows 128 divisor 1024 perturb 10sec',
+                )
+        # use raw because tc json is incorrect here
+        tmp = cmd(f'tc -details class show dev {interface}')
+        for rec in tmp.split('\n'):
+            rec = rec.strip().lower()
+            if 'root' in rec:
+                self.assertEqual(rec, 'class hfsc 1: root')
+            elif 'hfsc 1:1' in rec:
+                # m2 \S+bit is auto bandwidth
+                self.assertRegex(
+                    rec,
+                    r'class hfsc 1:1 parent 1: sc m1 0bit d 0us m2 \S+bit ul m1 0bit d 0us m2 \S+bit',
+                )
+            else:
+                self.assertRegex(
+                    rec,
+                    rf'class hfsc 1:2 parent 1:1 leaf \S+: ls m1 0bit d 0us m2 {ls["m2"]} ul m1 {ul["m1"]} d {ul["d"]}ms m2 {ul["m2"]}',
+                )
+
+        for key, val in rt.items():
+            self.cli_set(
+                base_path + ['policy', 'shaper-hfsc', policy_name, 'default', 'realtime', key, val]
+            )
+        self.cli_commit()
+
+        tmp = cmd(f'tc -details class show dev {interface}')
+        for rec in tmp.split('\n'):
+            rec = rec.strip().lower()
+            if 'hfsc 1:2' in rec:
+                self.assertTrue(
+                    f'rt m1 {rt["m1"]} d {rt["d"]}ms m2 {rt["m2"]} ls m1 0bit d 0us m2 {ls["m2"]} ul m1 {ul["m1"]} d {ul["d"]}ms m2 {ul["m2"]}'
+                    in rec
+                )
+
+        # add some class
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '10', 'linkshare', 'm2', '300kbit']
+        )
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '10', 'match', 'tst', 'ip', 'dscp', 'internet']
+        )
+
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '30', 'realtime', 'm2', '250kbit']
+        )
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '30', 'realtime', 'd', '77']
+        )
+        self.cli_set(
+            base_path + ['policy', 'shaper-hfsc', policy_name, 'class', '30', 'match', 'tst30', 'ip', 'dscp', 'critical']
+        )
+        self.cli_commit()
+
+        tmp = cmd(f'tc -details qdisc show dev {interface}')
+        self.assertEqual(4, len(tmp.split('\n')))
+
+        tmp = cmd(f'tc -details class show dev {interface}')
+        tmp = tmp.lower()
+
+        self.assertTrue(
+            f'rt m1 {rt["m1"]} d {rt["d"]}ms m2 {rt["m2"]} ls m1 0bit d 0us m2 {ls["m2"]} ul m1 {ul["m1"]} d {ul["d"]}ms m2 {ul["m2"]}'
+            in tmp
+        )
+        self.assertTrue(': ls m1 0bit d 0us m2 300kbit' in tmp)
+        self.assertTrue(': rt m1 0bit d 77ms m2 250kbit' in tmp)
+
     def test_22_rate_control_default(self):
         interface = self._interfaces[0]
         policy_name = f'qos-policy-{interface}'

src/conf_mode/qos.py

@@ -216,6 +216,46 @@ def _verify_match_group_exist(cls_config, qos):
                 Warning(f'Match group "{group}" does not exist!')
 
 
+def _verify_default_policy_exist(policy, policy_config):
+    if 'default' not in policy_config:
+        raise ConfigError(f'Policy {policy} misses "default" class!')
+
+
+def _check_shaper_hfsc_rate(cls, cls_conf):
+    is_m2_exist = False
+    for crit in TrafficShaperHFSC.criteria:
+        if cls_conf.get(crit, {}).get('m2') is not None:
+            is_m2_exist = True
+
+        if cls_conf.get(crit, {}).get('m1') is not None:
+            for crit_val in ['m2', 'd']:
+                if cls_conf.get(crit, {}).get(crit_val) is None:
+                    raise ConfigError(
+                        f'{cls} {crit} m1 value is set, but no {crit_val} was found!'
+                    )
+
+    if not is_m2_exist:
+        raise ConfigError(f'At least one m2 value needs to be set for class: {cls}')
+
+    if (
+        cls_conf.get('upperlimit', {}).get('m2') is not None
+        and cls_conf.get('linkshare', {}).get('m2') is None
+    ):
+        raise ConfigError(
+            f'Linkshare m2 needs to be defined to use upperlimit m2 for class: {cls}'
+        )
+
+
+def _verify_shaper_hfsc(policy, policy_config):
+    _verify_default_policy_exist(policy, policy_config)
+
+    _check_shaper_hfsc_rate('default', policy_config.get('default'))
+
+    if 'class' in policy_config:
+        for cls, cls_conf in policy_config['class'].items():
+            _check_shaper_hfsc_rate(cls, cls_conf)
+
+
 def verify(qos):
     if not qos or 'interface' not in qos:
         return None
@@ -259,11 +299,13 @@ def verify(qos):
                                 if queue_lim < max_tr:
                                     raise ConfigError(f'Policy "{policy}" uses queue-limit "{queue_lim}" < max-threshold "{max_tr}"!')
                 if policy_type in ['priority_queue']:
-                    if 'default' not in policy_config:
-                        raise ConfigError(f'Policy {policy} misses "default" class!')
+                    _verify_default_policy_exist(policy, policy_config)
                 if policy_type in ['rate_control']:
                     if 'bandwidth' not in policy_config:
                         raise ConfigError('Bandwidth not defined')
+                if policy_type in ['shaper_hfsc']:
+                    _verify_shaper_hfsc(policy, policy_config)
+
                 if 'default' in policy_config:
                     if 'bandwidth' not in policy_config['default'] and policy_type not in ['priority_queue', 'round_robin', 'shaper_hfsc']:
                         raise ConfigError('Bandwidth not defined for default traffic!')
@@ -299,6 +341,7 @@ def generate(qos):
 
     return None
 
+
 def apply(qos):
     # Always delete "old" shapers first
     for interface in interfaces():