diff --git a/scripts/featured b/scripts/featured index c470be87..05285a7b 100644 --- a/scripts/featured +++ b/scripts/featured @@ -274,11 +274,13 @@ class FeatureHandler(object): return True if enable: - self.enable_feature(feature) + if not self.enable_feature(feature): + return False syslog.syslog(syslog.LOG_INFO, "Feature {} is enabled and started".format(feature.name)) if disable: - self.disable_feature(feature) + if not self.disable_feature(feature): + return False syslog.syslog(syslog.LOG_INFO, "Feature {} is stopped and disabled".format(feature.name)) return True @@ -413,7 +415,7 @@ class FeatureHandler(object): for feature_name in feature_names: # Check if it is already enabled, if yes skip the system call unit_file_state = self.get_systemd_unit_state("{}.{}".format(feature_name, feature_suffixes[-1])) - if unit_file_state == "enabled" or not unit_file_state: + if unit_file_state == "enabled": continue cmds = [] for suffix in feature_suffixes: @@ -433,16 +435,17 @@ class FeatureHandler(object): syslog.syslog(syslog.LOG_ERR, "Feature '{}.{}' failed to be enabled and started" .format(feature.name, feature_suffixes[-1])) self.set_feature_state(feature, self.FEATURE_STATE_FAILED) - return + return False self.set_feature_state(feature, self.FEATURE_STATE_ENABLED) + return True def disable_feature(self, feature): feature_names, feature_suffixes = self.get_multiasic_feature_instances(feature) for feature_name in feature_names: # Check if it is already disabled, if yes skip the system call unit_file_state = self.get_systemd_unit_state("{}.{}".format(feature_name, feature_suffixes[-1])) - if unit_file_state in ("disabled", "masked") or not unit_file_state: + if unit_file_state in ("disabled", "masked"): continue cmds = [] for suffix in reversed(feature_suffixes): @@ -457,9 +460,10 @@ class FeatureHandler(object): syslog.syslog(syslog.LOG_ERR, "Feature '{}.{}' failed to be stopped and disabled" .format(feature.name, feature_suffixes[-1])) self.set_feature_state(feature, self.FEATURE_STATE_FAILED) - return + return False self.set_feature_state(feature, self.FEATURE_STATE_DISABLED) + return True def resync_feature_state(self, feature): current_entry = self._config_db.get_entry('FEATURE', feature.name) diff --git a/tests/featured/featured_test.py b/tests/featured/featured_test.py index a42d2bb8..d893f68a 100644 --- a/tests/featured/featured_test.py +++ b/tests/featured/featured_test.py @@ -493,3 +493,53 @@ def test_portinit_timeout(self, mock_syslog, get_runtime): call(['sudo', 'systemctl', 'enable', 'telemetry.service'], capture_output=True, check=True, text=True), call(['sudo', 'systemctl', 'start', 'telemetry.service'], capture_output=True, check=True, text=True)] mocked_subprocess.run.assert_has_calls(expected, any_order=True) + + def test_systemctl_command_failure(self, mock_syslog, get_runtime): + """Test that when systemctl commands fail: + 1. The feature state is not cached + 2. The feature state is set to FAILED + 3. The update_feature_state returns False + """ + mock_db = mock.MagicMock() + mock_feature_state_table = mock.MagicMock() + + feature_handler = featured.FeatureHandler(mock_db, mock_feature_state_table, {}, False) + feature_handler.is_delayed_enabled = True + + # Create a feature that should be enabled + feature_name = 'test_feature' + feature_cfg = { + 'state': 'enabled', + 'auto_restart': 'enabled', + 'delayed': 'False', + 'has_global_scope': 'True', + 'has_per_asic_scope': 'False' + } + + # Initialize the feature in cached_config using the same pattern as in featured + feature = featured.Feature(feature_name, feature_cfg) + feature_handler._cached_config.setdefault(feature_name, featured.Feature(feature_name, {})) + + # Mock subprocess.run and Popen to simulate command failure + with mock.patch('featured.subprocess') as mocked_subprocess: + # Mock Popen for get_systemd_unit_state + popen_mock = mock.Mock() + popen_mock.communicate.return_value = ('enabled', '') + popen_mock.returncode = 1 + mocked_subprocess.Popen.return_value = popen_mock + + # Mock run_cmd to raise an exception + with mock.patch('featured.run_cmd') as mocked_run_cmd: + mocked_run_cmd.side_effect = Exception("Command failed") + + # Try to update feature state + result = feature_handler.update_feature_state(feature) + + # Verify the result is False + assert result is False + + # Verify the feature state was set to FAILED + mock_feature_state_table.set.assert_called_with('test_feature', [('state', 'failed')]) + + # Verify the feature state was not enabled in the cache + assert feature_handler._cached_config[feature.name].state != 'enabled'