From 1231475d74b152d39d890c42bb281861c40592ab Mon Sep 17 00:00:00 2001 From: Junchao-Mellanox <57339448+Junchao-Mellanox@users.noreply.github.com> Date: Thu, 27 Apr 2023 09:45:11 +0800 Subject: [PATCH] [hostcfgd] Fix issue: FeatureHandler might override user configuration (#58) **Why I did this?** hostcfgd is using a subscribe/listen way to handle configuration data. It handles feature configuration like this: 1. subscribe to CONFIG DB FEATURE table 2. read initial data from CONFIG DB FEATURE table and call FeatureHandler.sync_state_field to process the initial data 3. As FEATURE table "state" field might be a template, FeatureHandler is responsible for rendering it 4. FeatureHandler will "resync" the rendered "state" back to CONFIG DB However, if there is a user configuration occurs between step 2 and step 3, user configuration will be silently ignored and override. Here is an example: 1. subscribe to CONFIG DB FEATURE table 2. read initial FEATURE data, say sflow.state = disabled. FeatureHandler.sync_state_field get an input that "sflow.state = disabled" 3. user issued command "config feature state sflow enabled". Now sflow.state is enabled in FEATURE table. 4. FeatureHandler.sync_state_field continues running and resync "sflow.state = disabled" back to FEATURE table In this case, user configuration in step#3 will be override. The PR is a fix for this issue. **How I verified?** Added new unit test ``` ----------- coverage: platform linux, python 3.9.2-final-0 ----------- Name Stmts Miss Cover ---------------------------------------------------- host_modules/config_engine.py 35 1 97% host_modules/gcu.py 54 0 100% host_modules/host_service.py 19 3 84% host_modules/showtech.py 27 7 74% scripts/caclmgrd 608 211 65% scripts/determine-reboot-cause 144 13 91% scripts/hostcfgd 1236 261 79% scripts/procdockerstatsd 157 90 43% ---------------------------------------------------- TOTAL 2280 586 74% Coverage HTML written to dir htmlcov Coverage XML written to file coverage.xml ======================= 104 passed, 4 warnings in 6.44s ======================== ``` --- scripts/hostcfgd | 35 +++++++++++++++---- tests/hostcfgd/hostcfgd_test.py | 61 +++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/scripts/hostcfgd b/scripts/hostcfgd index c99978d4..87b0ad7d 100644 --- a/scripts/hostcfgd +++ b/scripts/hostcfgd @@ -421,7 +421,7 @@ class FeatureHandler(object): # 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', feature_config.name, {'has_per_asic_scope': str(feature_config.has_per_asic_scope)}) - + def update_systemd_config(self, feature_config): """Updates `Restart=` field in feature's systemd configuration file according to the value of `auto_restart` field in `FEATURE` table of `CONFIG_DB`. @@ -496,7 +496,7 @@ class FeatureHandler(object): unit_file_state = self.get_systemd_unit_state("{}.{}".format(feature_name, feature_suffixes[-1])) if unit_file_state == "enabled" or not unit_file_state: continue - cmds = [] + cmds = [] for suffix in feature_suffixes: cmds.append(["sudo", "systemctl", "unmask", "{}.{}".format(feature_name, suffix)]) @@ -543,11 +543,28 @@ class FeatureHandler(object): self.set_feature_state(feature, self.FEATURE_STATE_DISABLED) def resync_feature_state(self, feature): - self._config_db.mod_entry('FEATURE', feature.name, {'state': feature.state}) + current_entry = self._config_db.get_entry('FEATURE', feature.name) + current_feature_state = current_entry.get('state') if current_entry else None - # resync the feature state to CONFIG_DB in namespaces in multi-asic platform - for ns, db in self.ns_cfg_db.items(): - db.mod_entry('FEATURE', feature.name, {'state': feature.state}) + if feature.state == current_feature_state: + return + + # feature.state might be rendered from a template, so that it should resync CONFIG DB + # FEATURE table to override the template value to valid state value + # ('always_enabled', 'always_disabled', 'disabled', 'enabled'). However, we should only + # resync feature state in two cases: + # 1. the rendered feature state is always_enabled or always_disabled, it means that the + # feature state is immutable and potential state change during HostConfigDaemon.load + # in redis should be skipped; + # 2. the current feature state in DB is a template which should be replaced by rendered feature + # state + # For other cases, we should not resync feature.state to CONFIG DB to avoid overriding user configuration. + if self._feature_state_is_immutable(feature.state) or self._feature_state_is_template(current_feature_state): + self._config_db.mod_entry('FEATURE', feature.name, {'state': feature.state}) + + # resync the feature state to CONFIG_DB in namespaces in multi-asic platform + for ns, db in self.ns_cfg_db.items(): + db.mod_entry('FEATURE', feature.name, {'state': feature.state}) def set_feature_state(self, feature, state): self._feature_state_table.set(feature.name, [('state', state)]) @@ -556,6 +573,12 @@ class FeatureHandler(object): for ns, tbl in self.ns_feature_state_tbl.items(): tbl.set(feature.name, [('state', state)]) + def _feature_state_is_template(self, feature_state): + return feature_state not in ('always_enabled', 'always_disabled', 'disabled', 'enabled') + + def _feature_state_is_immutable(self, feature_state): + return feature_state in ('always_enabled', 'always_disabled') + class Iptables(object): def __init__(self): ''' diff --git a/tests/hostcfgd/hostcfgd_test.py b/tests/hostcfgd/hostcfgd_test.py index fbeecb1c..1139641b 100644 --- a/tests/hostcfgd/hostcfgd_test.py +++ b/tests/hostcfgd/hostcfgd_test.py @@ -251,6 +251,67 @@ def test_feature_config_parsing_defaults(self): assert swss_feature.has_global_scope assert not swss_feature.has_per_asic_scope + @mock.patch('hostcfgd.FeatureHandler.update_systemd_config', mock.MagicMock()) + @mock.patch('hostcfgd.FeatureHandler.update_feature_state', mock.MagicMock()) + @mock.patch('hostcfgd.FeatureHandler.sync_feature_asic_scope', mock.MagicMock()) + def test_feature_resync(self): + mock_db = mock.MagicMock() + mock_db.get_entry = mock.MagicMock() + mock_db.mod_entry = mock.MagicMock() + mock_feature_state_table = mock.MagicMock() + + feature_handler = hostcfgd.FeatureHandler(mock_db, mock_feature_state_table, {}, False) + feature_table = { + 'sflow': { + 'state': 'enabled', + 'auto_restart': 'enabled', + 'delayed': 'True', + 'has_global_scope': 'False', + 'has_per_asic_scope': 'True', + } + } + mock_db.get_entry.return_value = None + feature_handler.sync_state_field(feature_table) + mock_db.mod_entry.assert_called_with('FEATURE', 'sflow', {'state': 'enabled'}) + mock_db.mod_entry.reset_mock() + + feature_handler = hostcfgd.FeatureHandler(mock_db, mock_feature_state_table, {}, False) + mock_db.get_entry.return_value = { + 'state': 'disabled', + } + feature_handler.sync_state_field(feature_table) + mock_db.mod_entry.assert_not_called() + + feature_handler = hostcfgd.FeatureHandler(mock_db, mock_feature_state_table, {}, False) + feature_table = { + 'sflow': { + 'state': 'always_enabled', + 'auto_restart': 'enabled', + 'delayed': 'True', + 'has_global_scope': 'False', + 'has_per_asic_scope': 'True', + } + } + feature_handler.sync_state_field(feature_table) + mock_db.mod_entry.assert_called_with('FEATURE', 'sflow', {'state': 'always_enabled'}) + mock_db.mod_entry.reset_mock() + + feature_handler = hostcfgd.FeatureHandler(mock_db, mock_feature_state_table, {}, False) + mock_db.get_entry.return_value = { + 'state': 'some template', + } + feature_table = { + 'sflow': { + 'state': 'enabled', + 'auto_restart': 'enabled', + 'delayed': 'True', + 'has_global_scope': 'False', + 'has_per_asic_scope': 'True', + } + } + feature_handler.sync_state_field(feature_table) + mock_db.mod_entry.assert_called_with('FEATURE', 'sflow', {'state': 'enabled'}) + class TesNtpCfgd(TestCase): """