saltstack / saltstack/salt

[BUG] salt.states.process.absent erroneously says that processes are killed without checking

Open
#63,603 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

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

Description

Description
salt.states.process.absent erroneously prints that processes are killed even if they aren't. There are no checks done to validate that processes are killed, only that a signal was sent to a process. These false positives make it hard to determine if a process is actually running or not.

Setup

AWS instance - Ubuntu 20.04
Salt master installed with

sudo apt-get install -y salt-master

Relevant state file:

# /srv/salt/kill_wumbo.sls

kill_wumbo:
  process.absent:
    - name: wumbo

Steps to Reproduce the behavior
Run the following command from the salt-master:

sudo salt '*' state.apply kill_wumbo

Here is the message that is printed out:

ip-10-1-1-164:
----------
          ID: kill_wumbo
    Function: process.absent
        Name: wumbo
      Result: True
     Comment: Killed 2 processes
     Started: 19:31:47.858554
    Duration: 13.623 ms
     Changes:
              ----------
              killed:
                  - 13847
                  - 13849

Summary for ip-10-1-1-164
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  13.623 ms

This expects that the wumbo process successfully accepted the SIGTERM, which is not the case and can never be expected. Not all processes accept signals in the desired manner so it cannot be assumed that the process(es) were killed correctly.

In this case, wumbo ignores SIGTERM and continues to run.

Expected behavior
I expect the module to check whether or not the process was actually killed, not that a signal was sent. If the desired behavior is to only send a signal it should say that in the logs, not that it "successfully killed X processes"

Versions Report

Salt Version:
          Salt: 3005.1

Dependency Versions:
          cffi: 1.14.6
      cherrypy: unknown
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.0
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: 2.21
      pycrypto: Not Installed
  pycryptodome: 3.9.8
        pygit2: Not Installed
        Python: 3.9.16 (main, Dec 14 2022, 15:47:31)
  python-gnupg: 0.4.8
        PyYAML: 5.4.1
         PyZMQ: 23.2.0
         smmap: Not Installed
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: 4.3.4

System Versions:
          dist: ubuntu 20.04 focal
        locale: utf-8
       machine: x86_64
       release: 5.15.0-1026-aws
        system: Linux
       version: Ubuntu 20.04 focal

Additional context
N/A

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 at the salt.states.process.absent entry point and reproduce the behavior with the kill_wumbo state and sudo salt '*' state.apply kill_wumbo. Trace how the state reports processes as killed after sending SIGTERM. Done means the result and log wording accurately reflect whether processes were terminated, with tests covering a process that ignores SIGTERM.

Written by the indexing model from the issue text.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.