From fd638d4644e569c908a37e080ec9add65abb6485 Mon Sep 17 00:00:00 2001 From: abdosi <58047199+abdosi@users.noreply.github.com> Date: Tue, 21 May 2024 08:43:02 -0700 Subject: [PATCH] Added support to render Feature Table has_global_scope field. (#120) What I did: Added support to render Feature Table has_global_scope based on Device Runtime Metadata Why I did: In some case we want to control a given feature should run in host/global namespace based on device runtime metadata. For example in case of chassis Linecard we want to run lldp on asic namespaces (front panel ports) and not on host/global (eth0 might be connected internally to the Supervisor and not to external management switch) How I verify: Manual Veriifcation UT updated. --- scripts/featured | 21 +++++++++++---------- tests/featured/featured_test.py | 2 +- tests/featured/test_vectors.py | 10 +++++----- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/scripts/featured b/scripts/featured index e5661096..28b8bf97 100644 --- a/scripts/featured +++ b/scripts/featured @@ -77,7 +77,7 @@ class Feature(object): self.state = self._get_feature_table_key_render_value(feature_cfg.get('state'), device_config or {}, ['enabled', 'disabled', 'always_enabled', 'always_disabled']) self.auto_restart = feature_cfg.get('auto_restart', 'disabled') self.delayed = safe_eval(feature_cfg.get('delayed', 'False')) - self.has_global_scope = safe_eval(feature_cfg.get('has_global_scope', 'True')) + self.has_global_scope = safe_eval(self._get_feature_table_key_render_value(feature_cfg.get('has_global_scope', 'True'), device_config or {}, ['True', 'False'])) self.has_per_asic_scope = safe_eval(self._get_feature_table_key_render_value(feature_cfg.get('has_per_asic_scope', 'False'), device_config or {}, ['True', 'False'])) self.has_per_dpu_scope = safe_eval(feature_cfg.get('has_per_dpu_scope', 'False')) @@ -198,7 +198,7 @@ class FeatureHandler(object): # Enable/disable the container service if the feature state was changed from its previous state. if self._cached_config[feature_name].state != feature.state: if self.update_feature_state(feature): - self.sync_feature_asic_scope(feature) + self.sync_feature_scope(feature) self._cached_config[feature_name] = feature else: self.resync_feature_state(self._cached_config[feature_name]) @@ -222,7 +222,7 @@ class FeatureHandler(object): self._cached_config.setdefault(feature_name, feature) self.update_systemd_config(feature) self.update_feature_state(feature) - self.sync_feature_asic_scope(feature) + self.sync_feature_scope(feature) self.resync_feature_state(feature) def update_feature_state(self, feature): @@ -270,10 +270,10 @@ class FeatureHandler(object): return True - def sync_feature_asic_scope(self, feature_config): - """Updates the has_per_asic_scope field in the FEATURE|* tables as the field + def sync_feature_scope(self, feature_config): + """Updates the has_global_scope or has_per_asic_scope field in the FEATURE|* tables as the field might have to be rendered based on DEVICE_METADATA table or Device Running configuration. - Disable the ASIC instance service unit file it the render value is False and update config + Disable the Global/ASIC instance service unit file it the render value is False and update config Args: feature: An object represents a feature's configuration in `FEATURE` @@ -284,12 +284,11 @@ class FeatureHandler(object): """ feature_names, feature_suffixes = self.get_multiasic_feature_instances(feature_config, True) for feature_name in feature_names: - if '@' not in feature_name: - continue unit_file_state = self.get_systemd_unit_state("{}.{}".format(feature_name, feature_suffixes[-1])) if not unit_file_state: continue - if unit_file_state != "disabled" and not feature_config.has_per_asic_scope: + if unit_file_state != "disabled" and \ + ((not feature_config.has_per_asic_scope and '@' in feature_name) or (not feature_config.has_global_scope and '@' not in feature_name)): cmds = [] for suffix in reversed(feature_suffixes): cmds.append(["sudo", "systemctl", "stop", "{}.{}".format(feature_name, suffix)]) @@ -305,10 +304,12 @@ class FeatureHandler(object): self.set_feature_state(feature_config, self.FEATURE_STATE_FAILED) return self._config_db.mod_entry(FEATURE_TBL, feature_config.name, {'has_per_asic_scope': str(feature_config.has_per_asic_scope)}) + self._config_db.mod_entry(FEATURE_TBL, feature_config.name, {'has_global_scope': str(feature_config.has_global_scope)}) # sync has_per_asic_scope to CONFIG_DB in namespaces in multi-asic platform for ns, db in self.ns_cfg_db.items(): db.mod_entry(FEATURE_TBL, feature_config.name, {'has_per_asic_scope': str(feature_config.has_per_asic_scope)}) + db.mod_entry(FEATURE_TBL, feature_config.name, {'has_global_scope': str(feature_config.has_global_scope)}) def update_systemd_config(self, feature_config): """Updates `Restart=` field in feature's systemd configuration file @@ -364,7 +365,7 @@ class FeatureHandler(object): def get_multiasic_feature_instances(self, feature, all_instance=False): # Create feature name suffix depending feature is running in host or namespace or in both feature_names = ( - ([feature.name] if feature.has_global_scope or not self.is_multi_npu else []) + + ([feature.name] if feature.has_global_scope or all_instance or not self.is_multi_npu else []) + ([(feature.name + '@' + str(asic_inst)) for asic_inst in range(device_info.get_num_npus()) if self.is_multi_npu and (all_instance or feature.has_per_asic_scope)]) + ([(feature.name + '@' + device_info.DPU_NAME_PREFIX + str(dpu_inst)) for dpu_inst in range(self.num_dpus) diff --git a/tests/featured/featured_test.py b/tests/featured/featured_test.py index 7b522b4c..94d9491a 100644 --- a/tests/featured/featured_test.py +++ b/tests/featured/featured_test.py @@ -262,7 +262,7 @@ def test_feature_config_parsing_defaults(self): @mock.patch('featured.FeatureHandler.update_systemd_config', mock.MagicMock()) @mock.patch('featured.FeatureHandler.update_feature_state', mock.MagicMock()) - @mock.patch('featured.FeatureHandler.sync_feature_asic_scope', mock.MagicMock()) + @mock.patch('featured.FeatureHandler.sync_feature_scope', mock.MagicMock()) def test_feature_resync(self): mock_db = mock.MagicMock() mock_db.get_entry = mock.MagicMock() diff --git a/tests/featured/test_vectors.py b/tests/featured/test_vectors.py index bc0d0cac..6def031f 100644 --- a/tests/featured/test_vectors.py +++ b/tests/featured/test_vectors.py @@ -1083,7 +1083,7 @@ "lldp": { "state": "enabled", "delayed": "False", - "has_global_scope": "True", + "has_global_scope": "{% if ('CHASSIS_METADATA' in DEVICE_RUNTIME_METADATA and DEVICE_RUNTIME_METADATA['CHASSIS_METADATA']['module_type'] in ['linecard']) %}False{% else %}True{% endif %}", "has_per_asic_scope": "{% if not DEVICE_RUNTIME_METADATA['ETHERNET_PORTS_PRESENT'] or ('CHASSIS_METADATA' in DEVICE_RUNTIME_METADATA and DEVICE_RUNTIME_METADATA['CHASSIS_METADATA']['module_type'] in ['supervisor']) %}False{% else %}True{% endif %}", "auto_restart": "enabled", "high_mem_alert": "disabled" @@ -1118,7 +1118,7 @@ }, "lldp": { "auto_restart": "enabled", - "has_global_scope": "True", + "has_global_scope": "False", "has_per_asic_scope": "True", "delayed": "False", "high_mem_alert": "disabled", @@ -1147,9 +1147,9 @@ call(['sudo', 'systemctl', 'unmask', 'teamd@1.service']), call(['sudo', 'systemctl', 'enable', 'teamd@1.service']), call(['sudo', 'systemctl', 'start', 'teamd@1.service']), - call(['sudo', 'systemctl', 'unmask', 'lldp.service']), - call(['sudo', 'systemctl', 'enable', 'lldp.service']), - call(['sudo', 'systemctl', 'start', 'lldp.service']), + call(['sudo', 'systemctl', 'mask', 'lldp.service']), + call(['sudo', 'systemctl', 'disable', 'lldp.service']), + call(['sudo', 'systemctl', 'stop', 'lldp.service']), call(['sudo', 'systemctl', 'unmask', 'lldp@0.service']), call(['sudo', 'systemctl', 'enable', 'lldp@0.service']), call(['sudo', 'systemctl', 'start', 'lldp@0.service']),