saltstack / saltstack/salt

[BUG] Race-condition on SaltMinion leads to job being reported as succeeded when in reality it failed

Open
#63,117 7 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

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

Description

Description
SaltMinion receives 2 jobs from the master. The first one includes a state.apply and the other one can be any job. When the second job executes, it will reload all the modules and recreate the context as well. The failed result of the state.apply is set to the old context so when checking for the actual result we will get a false positive because the context has been reset. The important thing to mention is that multiprocessing is set to False in the minion config. Setting it to True seems to fix this issue because the jobs are running in different processes. However, I guess this should work even if multiprocessing is set to False (clients without enough RAM cannot enable multiprocessing).

Setup

  • on-prem machine
  • VM (VMWare)
  • used bootstrap to install

Steps to Reproduce the behavior
Use this simple function as a module(I've added it in the test.py salt module)

def test_fail():
    time.sleep(6)
    raise CommandExecutionError(
        'Problem encountered'
    )

Then add a simple test.sls under /srv/salt:

test.test_fail:
  module.run

Then just send a job from the master to the minion:

salt osboxes state.apply,test.ping test, --out json

The result will be a success, validated with salt-run state.event as well. If we change the time.sleep(6) to time.sleep(3) it will be reported as failed. Why? Because 6 is greater than 5 which I guess is the default timeout for the master running a saltutil.find_job job.
So we have a state.apply that takes 6 seconds to fail. And a find_job that runs and completes before the state.apply to finish. I've attached some logs that could help. The place to look for the issue is line 14 vs line 35, the state.apply will report the failure but in the old context. This can also be seen by checking the value for ContextObjectId between the 2 use cases.
SaltRaceConditionLogs.txt

Expected behavior
Since running the same job when no other jobs are running reports a failure, I expect to return the same thing even if other jobs are running in the same time. Think about the first one being a state.apply that installs software (could take some time) and the other one being just an internal job that gets the current state or the saltutil.find_job that is running on this setup. This is causing false-positive results.

Screenshots
If applicable, add screenshots to help explain your problem.

Versions Report
Salt Version:
Salt: 3005.1

Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: 2.7.3
docker-py: Not Installed
gitdb: 2.0.6
gitpython: 3.0.7
Jinja2: 2.10.1
libgit2: Not Installed
M2Crypto: Not Installed
Mako: 1.1.0
msgpack: 0.6.2
msgpack-pure: Not Installed
mysql-python: Not Installed
pycparser: Not Installed
pycrypto: Not Installed
pycryptodome: 3.6.1
pygit2: Not Installed
Python: 3.8.10 (default, Jun 22 2022, 20:18:18)
python-gnupg: 0.4.5
PyYAML: 5.3.1
PyZMQ: 20.0.0
smmap: 2.0.5
timelib: Not Installed
Tornado: 4.5.3
ZMQ: 4.3.2

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

I've reproduced this initially on a Windows10 client having salt 3000.2 installed and then tried with the latest version.

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 by reproducing the race with the test_fail function in the test.py Salt module, the test.sls state, and the provided salt command using multiprocessing=False. Compare the state.apply and saltutil.find_job paths, including ContextObjectId and SaltRaceConditionLogs.txt. Done means a concurrent internal job no longer causes a long-running state.apply failure to be reported as success.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
distributed-systems, infrastructure
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.