Upstart module only finds legacy services #1915

Closed
mitsuhiko opened this Issue Aug 30, 2012 · 8 comments

2 participants

@mitsuhiko

The upstart module only looks for files in /etc/init.d which however are only dummy files. If you install a service to /etc/init only salt thinks the service does not exist and will fail. I only noticed this with the current master because salt is now unable to bring any of my services online with the running state.

@mitsuhiko

I think the regression might be there now because of 3c48f22 which introduced a check to see if a service is there.

@thatch45
Salt Stack member

Thanks for the find.

@mitsuhiko

The simple fix is to extend the glob to find files in /etc/init.d and /etc/init.

@mitsuhiko

Proposed fix:

diff --git a/salt/modules/upstart.py b/salt/modules/upstart.py
index 8005a7a..82f4708 100644
--- a/salt/modules/upstart.py
+++ b/salt/modules/upstart.py
@@ -122,6 +122,19 @@ def _sysv_is_enabled(name):
     return not _sysv_is_disabled(name)


+def _iter_service_names():
+    found = set()
+    for line in glob.glob('/etc/init.d/*'):
+        name = os.path.basename(line)
+        found.add(name)
+        yield name
+    for line in glob.glob('/etc/init/*.conf'):
+        name = os.path.basename(line)[:-5]
+        if name in found:
+            continue
+        yield name
+
+
 def get_enabled():
     '''
     Return the enabled services
@@ -131,8 +144,7 @@ def get_enabled():
         salt '*' service.get_enabled
     '''
     ret = set()
-    for line in glob.glob('/etc/init.d/*'):
-        name = os.path.basename(line)
+    for name in _iter_service_names():
         if _service_is_upstart(name):
             if _upstart_is_enabled(name):
                 ret.add(name)
@@ -152,8 +164,7 @@ def get_disabled():
         salt '*' service.get_disabled
     '''
     ret = set()
-    for line in glob.glob('/etc/init.d/*'):
-        name = os.path.basename(line)
+    for name in _iter_service_names():
         if _service_is_upstart(name):
             if _upstart_is_disabled(name):
                 ret.add(name)
@mitsuhiko

I think the whole idea of detecting sysvinit and upstart is not correct. There are valid cases for init scripts being symlinks. Instead I would do this on top of the above patch:

diff --git a/salt/modules/upstart.py b/salt/modules/upstart.py
index 82f4708..f1837ce 100644
--- a/salt/modules/upstart.py
+++ b/salt/modules/upstart.py
@@ -100,9 +100,7 @@ def _service_is_sysv(name):
     executable, like README or skeleton.
     '''
     script = '/etc/init.d/{0}'.format(name)
-    if not _is_symlink(script):
-        return os.access(script, os.X_OK)
-    return False
+    return not _service_is_upstart(name) and os.access(script, os.X_OK)
@thatch45
Salt Stack member

Thanks! I will get this in!

@thatch45 thatch45 closed this in 396d404 Aug 30, 2012
@mitsuhiko

I just noticed the patch was wrongly indented. The second for loop has to be on the same level as the first.

@thatch45
Salt Stack member

Thanks, I should have caught that, fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment