salt.states.service: detect that service failed to start/stop

salt/states/service.py

@@ -62,6 +62,9 @@ service, then set the reload value to True:
 from __future__ import absolute_import
 import time
 
+# Import Salt libs
+from salt.exceptions import CommandExecutionError
+
 __virtualname__ = 'service'
 
 
@@ -98,9 +101,17 @@ def _enable(name, started, result=True, **kwargs):
     ret = {}
 
     # is service available?
-    if not _available(name, ret):
+    try:
+        if not _available(name, ret):
+            return ret
+    except CommandExecutionError as exc:
+        ret['result'] = False
+        ret['comment'] = exc.strerror
         return ret
 
+    # Set default expected result
+    ret['result'] = result
+
     # Check to see if this minion supports enable
     if 'service.enable' not in __salt__ or 'service.enabled' not in __salt__:
         if started is True:
@@ -139,10 +150,9 @@ def _enable(name, started, result=True, **kwargs):
         return ret
 
     if __salt__['service.enable'](name, **kwargs):
-        after_toggle_enable_status = __salt__['service.enabled'](name,
-                                                                 **kwargs)
         # Service has been enabled
         ret['changes'] = {}
+        after_toggle_enable_status = __salt__['service.enabled'](name, **kwargs)
         # on upstart, certain services like apparmor will always return
         # False, even if correctly activated
         # do not trigger a change
@@ -160,16 +170,14 @@ def _enable(name, started, result=True, **kwargs):
         return ret
 
     # Service failed to be enabled
+    ret['result'] = False
     if started is True:
-        ret['result'] = False
         ret['comment'] = ('Failed when setting service {0} to start at boot,'
                           ' but the service is running').format(name)
     elif started is None:
-        ret['result'] = False
         ret['comment'] = ('Failed when setting service {0} to start at boot,'
                           ' but the service was already running').format(name)
     else:
-        ret['result'] = False
         ret['comment'] = ('Failed when setting service {0} to start at boot,'
                           ' and the service is dead').format(name)
     return ret
@@ -182,10 +190,18 @@ def _disable(name, started, result=True, **kwargs):
     ret = {}
 
     # is service available?
-    if not _available(name, ret):
+    try:
-        ret['result'] = True
+        if not _available(name, ret):
+            ret['result'] = True
+            return ret
+    except CommandExecutionError as exc:
+        ret['result'] = False
+        ret['comment'] = exc.strerror
         return ret
 
+    # Set default expected result
+    ret['result'] = result
+
     # is enable/disable available?
     if 'service.disable' not in __salt__ or 'service.disabled' not in __salt__:
         if started is True:
@@ -200,8 +216,8 @@ def _disable(name, started, result=True, **kwargs):
                               ' service {0} is dead').format(name)
         return ret
 
-    before_toggle_disable_status = __salt__['service.disabled'](name)
     # Service can be disabled
+    before_toggle_disable_status = __salt__['service.disabled'](name)
     if before_toggle_disable_status:
         # Service is disabled
         if started is True:
@@ -227,8 +243,6 @@ def _disable(name, started, result=True, **kwargs):
         # Service has been disabled
         ret['changes'] = {}
         after_toggle_disable_status = __salt__['service.disabled'](name)
-        # Service has been disabled
-        ret['changes'] = {}
         # on upstart, certain services like apparmor will always return
         # False, even if correctly activated
         # do not trigger a change
@@ -254,7 +268,6 @@ def _disable(name, started, result=True, **kwargs):
         ret['comment'] = ('Failed when setting service {0} to not start'
                           ' at boot, but the service was already running'
                           ).format(name)
-        return ret
     else:
         ret['comment'] = ('Failed when setting service {0} to not start'
                           ' at boot, and the service is dead').format(name)
@@ -315,13 +328,21 @@ def running(name, enable=None, sig=None, init_delay=None, **kwargs):
         return _enabled_used_error(ret)
 
     # Check if the service is available
-    if not _available(name, ret):
+    try:
+        if not _available(name, ret):
+            return ret
+    except CommandExecutionError as exc:
+        ret['result'] = False
+        ret['comment'] = exc.strerror
         return ret
 
     # lot of custom init script won't or mis implement the status
     # command, so it is just an indicator but can not be fully trusted
     before_toggle_status = __salt__['service.status'](name, sig)
-    before_toggle_enable_status = __salt__['service.enabled'](name)
+    if 'service.enabled' in __salt__:
+        before_toggle_enable_status = __salt__['service.enabled'](name)
+    else:
+        before_toggle_enable_status = True
 
     # See if the service is already running
     if before_toggle_status:
@@ -347,28 +368,39 @@ def running(name, enable=None, sig=None, init_delay=None, **kwargs):
             ret.update(_enable(name, False, result=False, **kwargs))
         elif enable is False:
             ret.update(_disable(name, False, result=False, **kwargs))
-    else:
+        return ret
-        ret['comment'] = 'Started Service {0}'.format(name)
-        if enable is True:
-            ret.update(_enable(name, True, **kwargs))
-        elif enable is False:
-            ret.update(_disable(name, True, **kwargs))
-
-    # only force a change state if we have explicitly detected them
-    after_toggle_status = __salt__['service.status'](name)
-    after_toggle_enable_status = __salt__['service.enabled'](name)
-    if (
-        (before_toggle_enable_status != after_toggle_enable_status) or
-        (before_toggle_status != after_toggle_status)
-    ) and not ret.get('changes', {}):
-        ret['changes'][name] = func_ret
 
     if init_delay:
         time.sleep(init_delay)
+
+    # only force a change state if we have explicitly detected them
+    after_toggle_status = __salt__['service.status'](name)
+    if 'service.enabled' in __salt__:
+        after_toggle_enable_status = __salt__['service.enabled'](name)
+    else:
+        after_toggle_enable_status = True
+    if (
+            (before_toggle_enable_status != after_toggle_enable_status) or
+            (before_toggle_status != after_toggle_status)
+    ) and not ret.get('changes', {}):
+        ret['changes'][name] = after_toggle_status
+
+    if after_toggle_status:
+        ret['comment'] = 'Started Service {0}'.format(name)
+    else:
+        ret['comment'] = 'Service {0} failed to start'.format(name)
+
+    if enable is True:
+        ret.update(_enable(name, after_toggle_status, result=after_toggle_status, **kwargs))
+    elif enable is False:
+        ret.update(_disable(name, after_toggle_status, result=after_toggle_status, **kwargs))
+
+    if init_delay:
         ret['comment'] = (
             '{0}\nDelayed return for {1} seconds'
             .format(ret['comment'], init_delay)
         )
+
     return ret
 
 
@@ -397,14 +429,23 @@ def dead(name, enable=None, sig=None, **kwargs):
         return _enabled_used_error(ret)
 
     # Check if the service is available
-    if not _available(name, ret):
+    try:
-        ret['result'] = True
+        if not _available(name, ret):
+            return ret
+    except CommandExecutionError as exc:
+        ret['result'] = False
+        ret['comment'] = exc.strerror
         return ret
 
     # lot of custom init script won't or mis implement the status
     # command, so it is just an indicator but can not be fully trusted
     before_toggle_status = __salt__['service.status'](name, sig)
-    before_toggle_enable_status = __salt__['service.enabled'](name)
+    if 'service.enabled' in __salt__:
+        before_toggle_enable_status = __salt__['service.enabled'](name)
+    else:
+        before_toggle_enable_status = True
+
+    # See if the service is already dead
     if not before_toggle_status:
         ret['comment'] = 'The service {0} is already dead'.format(name)
         if enable is True and not before_toggle_enable_status:
@@ -413,12 +454,12 @@ def dead(name, enable=None, sig=None, **kwargs):
             ret.update(_disable(name, None, **kwargs))
         return ret
 
+    # Run the tests
     if __opts__['test']:
         ret['result'] = None
         ret['comment'] = 'Service {0} is set to be killed'.format(name)
         return ret
 
-    # be sure to stop, in case we mis detected in the check
     func_ret = __salt__['service.stop'](name)
     if not func_ret:
         ret['result'] = False
@@ -427,20 +468,32 @@ def dead(name, enable=None, sig=None, **kwargs):
             ret.update(_enable(name, True, result=False, **kwargs))
         elif enable is False:
             ret.update(_disable(name, True, result=False, **kwargs))
-    else:
+        return ret
-        ret['comment'] = 'Service {0} was killed'.format(name)
+
-        if enable is True:
-            ret.update(_enable(name, False, **kwargs))
-        elif enable is False:
-            ret.update(_disable(name, False, **kwargs))
     # only force a change state if we have explicitly detected them
     after_toggle_status = __salt__['service.status'](name)
-    after_toggle_enable_status = __salt__['service.enabled'](name)
+    if 'service.enabled' in __salt__:
+        after_toggle_enable_status = __salt__['service.enabled'](name)
+    else:
+        after_toggle_enable_status = True
     if (
-        (before_toggle_enable_status != after_toggle_enable_status) or
+            (before_toggle_enable_status != after_toggle_enable_status) or
-        (before_toggle_status != after_toggle_status)
+            (before_toggle_status != after_toggle_status)
     ) and not ret.get('changes', {}):
-        ret['changes'][name] = func_ret
+        ret['changes'][name] = after_toggle_status
+
+    # be sure to stop, in case we mis detected in the check
+    if after_toggle_status:
+        ret['result'] = False
+        ret['comment'] = 'Service {0} failed to die'.format(name)
+    else:
+        ret['comment'] = 'Service {0} was killed'.format(name)
+
+    if enable is True:
+        ret.update(_enable(name, after_toggle_status, result=not after_toggle_status, **kwargs))
+    elif enable is False:
+        ret.update(_disable(name, after_toggle_status, result=not after_toggle_status, **kwargs))
+
     return ret
 
 
@@ -502,6 +555,19 @@ def mod_watch(name,
 
     sig
         The string to search for when looking for the service process with ps
+
+    reload
+        Use reload instead of the default restart (exclusive option with full_restart,
+        defaults to reload if both are used)
+
+    full_restart
+        Use service.full_restart instead of restart (exclusive option with reload)
+
+    force
+        Use service.force_reload instead of reload (needs reload to be set to True)
+
+    init_delay
+        Add a sleep command (in seconds) before the service is restarted/reloaded
     '''
     ret = {'name': name,
            'changes': {},

tests/unit/states/service_test.py

@@ -58,7 +58,10 @@ class ServiceTestCase(TestCase):
                 'result': True},
                {'changes': {},
                 'comment': 'The service salt is already running',
-                'name': 'salt', 'result': True}]
+                'name': 'salt', 'result': True},
+               {'changes': 'saltstack',
+                'comment': 'Service salt failed to start', 'name': 'salt',
+                'result': True}]
 
         tmock = MagicMock(return_value=True)
         fmock = MagicMock(return_value=False)
@@ -134,6 +137,22 @@ class ServiceTestCase(TestCase):
                 ):
                     self.assertDictEqual(service.running("salt", True), ret[4])
 
+                with contextlib.nested(
+                    patch.dict(service.__opts__, {'test': False}),
+                    patch.dict(
+                        service.__salt__, {
+                            'service.status':
+                            MagicMock(side_effect=[False, False]),
+                            'service.enabled':
+                            MagicMock(side_effect=[True, True]),
+                            'service.start':
+                            MagicMock(return_value="stack")}),
+                    patch.object(
+                        service, '_enable',
+                        MagicMock(return_value={'changes': 'saltstack'}))
+                ):
+                    self.assertDictEqual(service.running("salt", True), ret[6])
+
     def test_dead(self):
         '''
             Test to ensure that the named service is dead
@@ -149,8 +168,8 @@ class ServiceTestCase(TestCase):
                 'comment': 'Service salt was killed', 'name': 'salt',
                 'result': True},
                {'changes': {},
-                'comment': 'Service salt was killed', 'name': 'salt',
+                'comment': 'Service salt failed to die', 'name': 'salt',
-                'result': True},
+                'result': False},
                {'changes': 'saltstack',
                 'comment': 'The service salt is already dead', 'name': 'salt',
                 'result': True}]
@@ -176,6 +195,7 @@ class ServiceTestCase(TestCase):
                 patch.object(service, '_enable', mock)
             ):
                 self.assertDictEqual(service.dead("salt", True), ret[5])
+
             with contextlib.nested(
                 patch.dict(service.__opts__, {'test': False}),
                 patch.dict(
@@ -203,7 +223,7 @@ class ServiceTestCase(TestCase):
                     {'service.enabled':
                      MagicMock(side_effect=[True, True, False]),
                      'service.status':
-                     MagicMock(side_effect=[True, True, False]),
+                     MagicMock(side_effect=[True, False, False]),
                      'service.stop': MagicMock(return_value="stack")}),
                 patch.object(
                     service, '_enable',