saltstack / saltstack/salt

Some states need concurrent when running salt-minion as a non-root user - includes possible solution

Open
#50,651 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug severity-medium
Dominant language
Python
Stars
15.7k
Forks
5.6k
Avg merge
2d 44m
Merged PRs (30d)
80

Description

Description of Issue/Question

When salt-minion is configured to run as a non-root user with sudo privileges, some states require concurrent to be set to True, as the minion starts another process to actually execute the state. When using a sudo setup, some states are executed by issuing a sudo command to salt-call, which then detects that another state is already running. This result is then propagated up the chain, and the result of the state is that another state is already running. Unfortunate, as the actual work should have been performed by the child process.

The workaround/solution to this issue is to force concurrent to True for the states that behaves in this manner.

So far, the states state.apply, state.sls, state.highstate has been identified and a workaround has been implemented in executors/sudo.py (https://github.com/saltstack/salt/pull/42021).

I have run into a few other states that need the fix as well, and are unsure whether this is now a conclusive list of all states needing concurrent to run in a sudo setup:

state.show_highstate
state.show_sls

Setup and steps to Reproduce Issue

Setup and reproduction of errors has been fully described in https://github.com/saltstack/salt/issues/25842

Versions Report
Salt Version:
           Salt: 2018.3.0

Dependency Versions:
           cffi: 1.5.2
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: 2.10
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.13 (default, Jan 11 2017, 10:56:06) [GCC]
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 14.0.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.4

System Versions:
           dist: SuSE 12 x86_64
         locale: UTF-8
        machine: x86_64
        release: 4.4.143-94.47-default
         system: Linux
        version: SUSE Linux Enterprise Server  12 x86_64

I've also seen the issue on a 2018.3.2 on Debian 9.5, so not unique to the above version.

Suggested workaround/solution

Instead of making a list of states that need to have concurrent = True hardcoded in sudo.py, it might be an idea to check if the process found to be a concurrent instance is actually an ancestor of the state reporting the concurrency issue. This seem to be logically placed in modules/states.py, and seem to me to be a bit more dynamic in it's working. If a process is a direct decendant of the process being reported, then ignore that process.

The following patch seem to work in that way, but probably need someone to sanity check it before spreading it widely.

--- state.py.new        2018-11-26 14:53:26.306978300 +0100
+++ state.py.new        2018-11-26 14:56:43.554939700 +0100
@@ -21,6 +21,7 @@
 import tarfile
 import tempfile
 import time
+import psutil

 # Import salt libs
 import salt.config
@@ -375,7 +376,23 @@
     if concurrent:
         return ret
     active = __salt__['saltutil.is_running']('state.*')
+    ancestors = []
+    pid = os.getpid()
+    cur_pid = pid
+    init_found = False
+    # Build list of ancestor processors.
+    while not init_found:
+        cur_ppid = psutil.Process(cur_pid).ppid
+        ancestors.append(cur_ppid)
+        cur_pid = cur_ppid
+        if cur_pid in [0, 1]:
+            init_found = True
+
     for data in active:
+        # If the pid that is concurrent is one of the ancestores,
+        # then ignore this process, as we are probably running a sudo setup.
+        if data['pid'] in ancestors:
+            continue
         err = (
             'The function "{0}" is running as PID {1} and was started at '
             '{2} with jid {3}'

Of note, the psutil is overridden by psutil_compat, so it is my guess that it should work on most systems.

I'm unsure as to whether this module is executed on other than Unix/Linux like platforms and if it would work on those.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with executors/sudo.py and modules/states.py, then review the reproduction described in issue 25842. Check how state.apply, state.sls, state.highstate, state.show_highstate, and state.show_sls handle concurrent execution and how the proposed ancestor-process behavior works across supported platforms. Done means the affected states no longer report a false concurrent run in the sudo setup, with the complete affected list determined.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
devops, infrastructure
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
32/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.