Merge pull request #3365 from vyos/mergify/bp/sagitta/pr-3316

qos: T4248: Allow to remove the only rule from the qos class (backport #3316)

python/vyos/qos/base.py

@@ -246,6 +246,7 @@ def update(self, config, direction, priority=None):
                 filter_cmd_base += ' protocol all'
 
                 if 'match' in cls_config:
+                    is_filtered = False
                     for index, (match, match_config) in enumerate(cls_config['match'].items(), start=1):
                         filter_cmd = filter_cmd_base
                         if self.qostype == 'shaper' and 'prio ' not in filter_cmd:
@@ -330,11 +331,13 @@ def update(self, config, direction, priority=None):
                                 cls = int(cls)
                                 filter_cmd += f' flowid {self._parent:x}:{cls:x}'
                                 self._cmd(filter_cmd)
+                                is_filtered = True
 
                     vlan_expression = "match.*.vif"
                     match_vlan = jmespath.search(vlan_expression, cls_config)
 
-                    if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in cls_config):
+                    if any(tmp in ['exceed', 'bandwidth', 'burst'] for tmp in cls_config) \
+                        and is_filtered:
                         # For "vif" "basic match" is used instead of "action police" T5961
                         if not match_vlan:
                             filter_cmd += f' action police'

smoketest/scripts/cli/test_qos.py

@@ -697,6 +697,45 @@ def test_12_shaper_with_red_queue(self):
         for config_entry in config_entries:
             self.assertIn(config_entry, output)
 
+    def test_13_shaper_delete_only_rule(self):
+        default_bandwidth = 100
+        default_burst = 100
+        interface = self._interfaces[0]
+        class_bandwidth = 50
+        class_ceiling = 5
+        src_address = '10.1.1.0/24'
+
+        shaper_name = f'qos-shaper-{interface}'
+        self.cli_set(base_path + ['interface', interface, 'egress', shaper_name])
+        self.cli_set(base_path + ['policy', 'shaper', shaper_name, 'bandwidth', f'10mbit'])
+        self.cli_set(base_path + ['policy', 'shaper', shaper_name, 'default', 'bandwidth', f'{default_bandwidth}mbit'])
+        self.cli_set(base_path + ['policy', 'shaper', shaper_name, 'default', 'burst', f'{default_burst}'])
+
+        self.cli_set(base_path + ['policy', 'shaper', shaper_name, 'class', '30', 'bandwidth', f'{class_bandwidth}mbit'])
+        self.cli_set(base_path + ['policy', 'shaper', shaper_name, 'class', '30', 'ceiling', f'{class_ceiling}mbit'])
+        self.cli_set(base_path + ['policy', 'shaper', shaper_name, 'class', '30', 'match', 'ADDRESS30', 'ip', 'source', 'address', src_address])
+        self.cli_set(base_path + ['policy', 'shaper', shaper_name, 'class', '30', 'match', 'ADDRESS30', 'description', 'smoketest'])
+        self.cli_set(base_path + ['policy', 'shaper', shaper_name, 'class', '30', 'priority', '5'])
+        self.cli_set(base_path + ['policy', 'shaper', shaper_name, 'class', '30', 'queue-type', 'fair-queue'])
+
+        # commit changes
+        self.cli_commit()
+        # check root htb config
+        output = cmd(f'tc class show dev {interface}')
+
+        config_entries = (
+            f'prio 5 rate {class_bandwidth}Mbit ceil {class_ceiling}Mbit burst 15Kb',  # specified class
+            f'prio 7 rate {default_bandwidth}Mbit ceil 100Mbit burst {default_burst}b',  # default class
+        )
+        for config_entry in config_entries:
+            self.assertIn(config_entry, output)
+
+        self.assertTrue('' != cmd(f'tc filter show dev {interface}'))
+        # self.cli_delete(base_path + ['policy', 'shaper', shaper_name, 'class', '30', 'match', 'ADDRESS30'])
+        self.cli_delete(base_path + ['policy', 'shaper', shaper_name, 'class', '30', 'match', 'ADDRESS30', 'ip', 'source', 'address', src_address])
+        self.cli_commit()
+        self.assertEqual('', cmd(f'tc filter show dev {interface}'))
+
 
 if __name__ == '__main__':
     unittest.main(verbosity=2)

src/conf_mode/qos.py

@@ -70,6 +70,22 @@ def get_shaper(qos, interface_config, direction):
 
     return (map_vyops_tc[shaper_type], shaper_config)
 
+
+def _clean_conf_dict(conf):
+    """
+    Delete empty nodes from config e.g.
+        match ADDRESS30 {
+            ip {
+                source {}
+            }
+        }
+    """
+    if isinstance(conf, dict):
+        return {node: _clean_conf_dict(val) for node, val in conf.items() if val != {} and _clean_conf_dict(val) != {}}
+    else:
+        return conf
+
+
 def get_config(config=None):
     if config:
         conf = config
@@ -120,6 +136,13 @@ def get_config(config=None):
                     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():
+                    if 'match' in cls_config:
+                        cls_config['match'] = _clean_conf_dict(cls_config['match'])
+                        if cls_config['match'] == {}:
+                            del cls_config['match']
 
     return qos