Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Test mode does not propagate to __states__ when using prereq #62590

Open
OrangeDog opened this issue Aug 31, 2022 · 4 comments
Open

[BUG] Test mode does not propagate to __states__ when using prereq #62590

OrangeDog opened this issue Aug 31, 2022 · 4 comments
Assignees
Labels
Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt Requisite Issue is in Salt's Requisite system

Comments

@OrangeDog
Copy link
Contributor

OrangeDog commented Aug 31, 2022

Description
When prereq is used, the target state is first run in test mode.
If the target state calls other states via __states__, those states will not run in test mode, and will actually make changes.
Then when it runs "for real" there may be nothing left to do and it will report no changes.

Setup

# _states/test2.py
import logging
log = logging.getLogger(__name__)

def call_another(name, m_name, **kwargs):
    ret = __states__[m_name](name, **kwargs)
    log.info(f'{__opts__["test"]}: {ret}')
    return ret
# test.sls

run indirect:
  test2.call_another:
    - m_name: test.succeed_with_changes

run prereq:
  test2.call_another:
    - m_name: test.succeed_with_changes

nop:
  test.nop:
    - prereq:
      - run prereq

Steps to Reproduce the behavior

# sudo salt-call state.apply --log-level=info
[INFO] Loading fresh modules for state activity
[INFO] Fetching file from saltenv 'test', ** done ** 'test.sls'
[INFO] Running state [run indirect] at time 10:54:34.955122
[INFO] Executing state test2.call_another for [run indirect]
[INFO] False: {'name': 'run indirect', 'changes': {'testing': {'old': 'Unchanged', 'new': 'Something pretended to change'}}, 'result': True, 'comment': 'Success!'}
[INFO] Made the following changes:
'testing' changed from 'Unchanged' to 'Something pretended to change'

[INFO] Completed state [run indirect] at time 10:54:34.956321 (duration_in_ms=1.199)
[INFO] Running state [run prereq] at time 10:54:34.956821
[INFO] True: {'name': 'run prereq', 'changes': {'testing': {'old': 'Unchanged', 'new': 'Something pretended to change'}}, 'result': True, 'comment': 'Success!'}
[INFO] Running state [run prereq] at time 10:54:34.957539
[INFO] Executing state test2.call_another for [run prereq]
[INFO] False: {'name': 'run prereq', 'changes': {'testing': {'old': 'Unchanged', 'new': 'Something pretended to change'}}, 'result': True, 'comment': 'Success!'}
[INFO] Made the following changes:
'testing' changed from 'Unchanged' to 'Something pretended to change'

[INFO] Completed state [run prereq] at time 10:54:34.958245 (duration_in_ms=0.705)
local:
  Name: run indirect - Function: test2.call_another - Result: Changed Started: - 10:54:34.955122 Duration: 1.199 ms
  Name: run prereq - Function: test2.call_another - Result: Changed Started: - 10:54:34.957540 Duration: 0.705 ms

Summary for local
------------
Succeeded: 3 (changed=2)
Failed:    0
------------
Total states run:     3
Total run time:   1.908 ms

Expected behavior

[INFO] Running state [run prereq] at time 10:54:34.956821
[INFO] True: {'name': 'run prereq', 'changes': {'testing': {'old': 'Unchanged', 'new': 'Something pretended to change'}}, 'result': None, 'comment': "If we weren't testing, this would be successful with changes"}

Versions Report

salt --versions-report
Salt Version:
          Salt: 3004.2

Dependency Versions:
          cffi: 1.14.5
      cherrypy: Not Installed
      dateutil: 2.7.3
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 2.10.1
       libgit2: 1.5.0
      M2Crypto: 0.31.0
          Mako: Not Installed
       msgpack: 0.6.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: 2.20
      pycrypto: Not Installed
  pycryptodome: 3.6.1
        pygit2: 1.10.0
        Python: 3.8.10 (default, Jun 22 2022, 20:18:18)
  python-gnupg: 0.4.5
        PyYAML: 5.3.1
         PyZMQ: 18.1.1
         smmap: Not Installed
       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.4.0-125-generic
        system: Linux
       version: Ubuntu 20.04 focal

Additional context
Also tested with 3005. Same result.

@OrangeDog OrangeDog added Bug broken, incorrect, or confusing behavior needs-triage labels Aug 31, 2022
@OrangeDog OrangeDog added the Core relates to code central or existential to Salt label Aug 31, 2022
@OrangeDog
Copy link
Contributor Author

Some possible solutions from @mattp-

--- a/salt/state.py
+++ b/salt/state.py
@@ -2228,14 +2228,15 @@ class State:
             inject_globals["__env__"] = str(low["__env__"])
 
         if self.inject_globals:
             inject_globals.update(self.inject_globals)
 
         if low.get("__prereq__"):
-            test = sys.modules[self.states[cdata["full"]].__module__].__opts__["test"]
-            sys.modules[self.states[cdata["full"]].__module__].__opts__["test"] = True
+            test_opts = copy.deepcopy(self.opts)
+            test_opts["test"] = True
+            inject_globals["__opts__"] = test_opts
         try:
             # Let's get a reference to the salt environment to use within this
             # state call.
             #
             # If the state function accepts an 'env' keyword argument, it
             # allows the state to be overridden(we look for that in cdata). If
@@ -2315,17 +2316,12 @@ class State:
                 "result": False,
                 "name": name,
                 "changes": {},
                 "comment": "An exception occurred in this state: {}".format(trb),
             }
         finally:
-            if low.get("__prereq__"):
-                sys.modules[self.states[cdata["full"]].__module__].__opts__[
-                    "test"
-                ] = test
-
             self.state_con.pop("runas", None)
             self.state_con.pop("runas_password", None)
 
         if not isinstance(ret, dict):
             return ret
--- a/salt/state.py
+++ b/salt/state.py
@@ -2230,12 +2230,13 @@ class State:
         if self.inject_globals:
             inject_globals.update(self.inject_globals)
 
         if low.get("__prereq__"):
             test = sys.modules[self.states[cdata["full"]].__module__].__opts__["test"]
             sys.modules[self.states[cdata["full"]].__module__].__opts__["test"] = True
+            inject_globals["__opts__"] = sys.modules[self.states[cdata["full"]].__module__].__opts__
         try:
             # Let's get a reference to the salt environment to use within this
             # state call.
             #
             # If the state function accepts an 'env' keyword argument, it
             # allows the state to be overridden(we look for that in cdata). If

@Oloremo
Copy link
Contributor

Oloremo commented Jul 20, 2023

@dwoz would the fix for that be planned for 3006.2 since it affects the x509_v2 state?

@aarnaud
Copy link
Contributor

aarnaud commented Jul 20, 2023

I did the update to 3006.1 and importing the patched file from the PR #64269 in _states folder on our project, it work fine.

@chdeliens
Copy link

Hi everyone, I was about to submit a similar bug when I stumble upon this one, close enough from what I was experiencing on 3006.2 with this "minimal SLS to reproduce":

test_prereq:
  cmd.run:
    - name: echo 'stop'
    - prereq:
      - file: test_file

test_file:
  file.managed:
    - name: /app/license.xml
    - source: salt://licenses/{{ salt['network.get_hostname']() }}.xml
    - user: app
    - group: app
    - mode: 640

In my case, the file.managed part would return:

  • Zero changes and no prereq triggering when running in REAL mode;
  • Changes OK and prereq reported to be triggered when running in TEST mode.

I've upgraded the Minion, it's now running 3006.5 and working as expected. Kudos to the ones who helped fixed this issue, many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt Requisite Issue is in Salt's Requisite system
Projects
None yet
Development

No branches or pull requests

5 participants