[Feature Request] supervisord.running state should check for retcode

[Oloremo]

Description of Issue/Question

Look like supervisord.running state is not checking the return codes of the supervisordctl.
It's probably ok since the current (3.x) version of the supervisord is always returning the 0 exit code. But the version from master (future 4.x) already has Supervisor/supervisor#668 merged which adds the correct exit codes and we use supervisord with this patch to enforce the exit codes.

Not sure what is the Salt policy for such situations.

Steps to Reproduce Issue

some_name:
  supervisord.running:
    - name: non-existent

The example above will always be successful and will always try to apply state on each run.

Versions Report

Salt Version:
           Salt: 2018.3.3

Dependency Versions:
           cffi: 1.6.0
       cherrypy: Not Installed
       dateutil: 2.7.5
      docker-py: 1.10.6
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: 1.2.5
      pycparser: 2.14
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.5 (default, Jul 13 2018, 13:06:57)
   python-gnupg: Not Installed
         PyYAML: 3.10
          PyZMQ: 17.1.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 5.1.1
            ZMQ: 4.2.5

System Versions:
           dist: centos 7.5.1804 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-862.3.2.el7.x86_64
         system: Linux
        version: CentOS Linux 7.5.1804 Core

[garethgreenaway]

@Oloremo Thanks for the report. we should definitely account for the retcode is supervisord is going to start returning them in future versions. Marking this as a feature request for a future feature addition. Thanks!

[redbaron4]

@garethgreenaway We are running Salt-2019.2.0 and supervisor 4.0.4. Since supervisor returns non-zero exit codes for supervisorctl status, it breaks salt states that depend on return of a 0 code for successful execution. Maybe as a short-term fix, the return code can be discarded in salt's supervisor module. There is a discussion about this feature in the supervisor issues page too here

[Oloremo]

we're using this local patch to workaround this:

+++ states/_modules/supervisord.py	2019-03-12 14:51:28.465844095 +0000
@@ -62,13 +62,6 @@
     return ret


-def _get_return(ret):
-    if ret['retcode'] == 0:
-        return ret['stdout']
-    else:
-        return ''
-
-
 def start(name='all', user=None, conf_file=None, bin_env=None):
     '''
     Start the named service.
@@ -96,7 +89,7 @@
         runas=user,
         python_shell=False,
     )
-    return _get_return(ret)
+    return ret['stdout']


 def restart(name='all', user=None, conf_file=None, bin_env=None):
@@ -126,7 +119,7 @@
         runas=user,
         python_shell=False,
     )
-    return _get_return(ret)
+    return ret['stdout']


 def stop(name='all', user=None, conf_file=None, bin_env=None):
@@ -156,7 +149,7 @@
         runas=user,
         python_shell=False,
     )
-    return _get_return(ret)
+    return ret['stdout']


 def add(name, user=None, conf_file=None, bin_env=None):
@@ -186,7 +179,7 @@
         runas=user,
         python_shell=False,
     )
-    return _get_return(ret)
+    return ret['stdout']


 def remove(name, user=None, conf_file=None, bin_env=None):
@@ -216,7 +209,7 @@
         runas=user,
         python_shell=False,
     )
-    return _get_return(ret)
+    return ret['stdout']


 def reread(user=None, conf_file=None, bin_env=None):
@@ -242,7 +235,7 @@
         runas=user,
         python_shell=False,
     )
-    return _get_return(ret)
+    return ret['stdout']


 def update(user=None, conf_file=None, bin_env=None, name=None):
@@ -278,7 +271,7 @@
         runas=user,
         python_shell=False,
     )
-    return _get_return(ret)
+    return ret['stdout']


 def status(name=None, user=None, conf_file=None, bin_env=None):
@@ -331,8 +324,9 @@
         _ctl_cmd('status', name, conf_file, bin_env),
         runas=user,
         python_shell=False,
+        ignore_retcode=True,
     )
-    return _get_return(ret)
+    return ret['stdout']


 def custom(command, user=None, conf_file=None, bin_env=None):
@@ -358,7 +352,7 @@
         runas=user,
         python_shell=False,
     )
-    return _get_return(ret)
+    return ret['stdout']

But a proper fix would be appreciated.

[redbaron4]

@Oloremo A smaller patch can be to pass success_returncodes=range(256) to all cmd.run_all calls in the module.

[Oloremo]

Well, I want to catch errors. :-) It's not about avoiding the error states it's about to handle it properly.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[Oloremo]

Supervisord 4.x released long time ago with the support of proper exit codes yet for Salt it's still always 0.

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.