Revert "Revert "[featured] fix non existing feature start (#234)" (#273)"

This reverts commit 0206da1.

Author: zbud-msft

scripts/featured

@@ -266,11 +266,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
@@ -405,7 +407,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:
@@ -425,16 +427,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):
@@ -449,9 +452,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)

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'