possible [BUG]? 3000.9 cmd.run with test=true does not trigger watch/watch_in

[onmeac]

Description
A cmd.run during test=true that "would have been executed" does not have any changes, obviously becuase the cmd hasn't run yet.

Because cmd.run has no changes, service.running that has a watch on that cmd.run during test=true does not mention "Service is set to be restarted".

There is logic already present for cmd.run, e.g. unless/onlyif, which helps to determine if cmd.run is going to something.

Setup
foo.sls (simplified example)

cmd_test:
  cmd.run:
    - name: echo test > /test
    - unless: grep test /test

service_test:
  service.running:
    - name: postfix
    - watch:
        - cmd: cmd_test

Steps to Reproduce the behavior

• echo something > /test
• salt-call state.apply foo test=true service_test not mentioning going to restart
• salt-call state.apply foo service_test restarted

Expected behavior
service_test should mention: "Service is set to be restarted" during test=true

Versions Report

salt --versions-report

Salt Version:
           Salt: 3000.9
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.11.1
        libgit2: Not Installed
       M2Crypto: 0.35.2
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.6.2
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.6.8 (default, Nov 16 2020, 16:55:22)
   python-gnupg: Not Installed
         PyYAML: 3.13
          PyZMQ: 15.3.0
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.1.4
 
System Versions:
           dist: centos 7.9.2009 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-1160.24.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.9.2009 Core

[dmurphy18]

@onmeac Believe this issue is similar #60120 and fixed by PR #60150
Text is a little different but similar to what you state

Comment: The service nginx is set to restart

Closing since duplicate, please re-open with details if you believe this issue is not addressed by the PR above.

[onmeac]

@dmurphy18 looks similar, but when I test PR #60150 using test=true a service.running will always report The service <name> is set to restart, without test=true, service does not actually restart.

[dmurphy18]

@onmeac Just clarifying, with test=True and PR #60150 you now get an indication that the service will be restarted. However without test=True, that is, commands should be executed, the service is not actually restarted ?

[dmurphy18]

For adjusted foo.sls where /dog/kat already exists

root@Unknown:/srv/salt# cat foo.sls 
cmd_test:
  cmd.run:
    - name: echo test > /dog/kat
    - unless: grep test /dog/kat
##    - name: echo test > /test
##    - unless: grep test /test

service_test:
  service.running:
    - name: postfix
    - watch:
        - cmd: cmd_test
root@Unknown:/srv/salt# 

root@Unknown:/srv/salt# salt server1 state.apply foo test=True
server1:
----------
          ID: cmd_test
    Function: cmd.run
        Name: echo test > /dog/kat
      Result: True
     Comment: unless condition is true
     Started: 10:42:27.312729
    Duration: 1556.403 ms
     Changes:   
----------
          ID: service_test
    Function: service.running
        Name: postfix
      Result: None
     Comment: The service postfix is set to restart
     Started: 10:42:28.870120
    Duration: 28.171 ms
     Changes:   

Summary for server1
------------
Succeeded: 2 (unchanged=1)
Failed:    0
------------
Total states run:     2
Total run time:   1.585 s
root@Unknown:/srv/salt# salt server1 state.apply foo
server1:
----------
          ID: cmd_test
    Function: cmd.run
        Name: echo test > /dog/kat
      Result: True
     Comment: unless condition is true
     Started: 10:42:46.739622
    Duration: 1570.26 ms
     Changes:   
----------
          ID: service_test
    Function: service.running
        Name: postfix
      Result: True
     Comment: The service postfix is already running
     Started: 10:42:48.310443
    Duration: 29.452 ms
     Changes:   

Summary for server1
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time:   1.600 s
root@Unknown:/srv/salt# 

and PID's before and after

[root@Unknown ~]# ps -ef | grep postfix
root      1297     1  0 10:23 ?        00:00:00 /usr/libexec/postfix/master -w
postfix   1304  1297  0 10:23 ?        00:00:00 pickup -l -t unix -u
postfix   1305  1297  0 10:23 ?        00:00:00 qmgr -l -t unix -u
root      3190  2417  0 10:42 pts/1    00:00:00 grep --color=auto postfix
[root@Unknown ~]# ps -ef | grep postfix
root      1297     1  0 10:23 ?        00:00:00 /usr/libexec/postfix/master -w
postfix   1304  1297  0 10:23 ?        00:00:00 pickup -l -t unix -u
postfix   1305  1297  0 10:23 ?        00:00:00 qmgr -l -t unix -u
root      3216  2417  0 10:42 pts/1    00:00:00 grep --color=auto postfix
[root@Unknown ~]# 

The service may or may not be restarted, depending on the state of systemd and other items in the system.
Perhaps the text form PR #60150 should be adjusted from

The service <name> is set to restart

to

The service <name> may be restarted

[dmurphy18]

From Slack conversion with @onmeac

#60120 is fixed with PR #60150 so that that part is currently reporting correctly 
for that case. In your case, the same text is not appropriate since the service is
not going to restart, however deducing what will and will not restart due to 
systemd requirements will be interesting to develop. Perhaps the cheap 
wiggle room text may not be the best solution. I shall have to dig into this more 
in depth, to see if there a better solution to reporting this.

[dmurphy18]

The PR's #61019 and #61019, back to square one.

With Salt 3004 for Master and Minion and using the provided test case, the following is produced.

root@c7mstr:/srv/salt# salt tc7 state.apply foo test=True
tc7:
----------
          ID: cmd_test
    Function: cmd.run
        Name: echo test > /test
      Result: None
     Comment: Command "echo test > /test" would have been executed
     Started: 11:27:44.658372
    Duration: 1629.769 ms
     Changes:   
----------
          ID: service_test
    Function: service.running
        Name: postfix
      Result: True
     Comment: The service postfix is already running
     Started: 11:27:46.288656
    Duration: 36.251 ms
     Changes:   

Summary for tc7
------------
Succeeded: 2 (unchanged=1)
Failed:    0
------------
Total states run:     2
Total run time:   1.666 s
root@c7mstr:/srv/salt# 

And with Master and Minion off the current Master branch(2022-02-14 and pip installed)

root@c7mstr:/home/david/dev1# salt tc7 state.apply foo test=True
tc7:
----------
          ID: cmd_test
    Function: cmd.run
        Name: echo test > /test
      Result: True
     Comment: unless condition is true
     Started: 12:32:57.423932
    Duration: 3531.115 ms
     Changes:   
----------
          ID: service_test
    Function: service.running
        Name: postfix
      Result: True
     Comment: The service postfix is already running
     Started: 12:33:00.955611
    Duration: 38.813 ms
     Changes:   

Summary for tc7
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time:   3.570 s
root@c7mstr:/home/david/dev1# salt tc7 state.apply foo
tc7:
----------
          ID: cmd_test
    Function: cmd.run
        Name: echo test > /test
      Result: True
     Comment: unless condition is true
     Started: 12:33:10.039586
    Duration: 1394.157 ms
     Changes:   
----------
          ID: service_test
    Function: service.running
        Name: postfix
      Result: True
     Comment: The service postfix is already running
     Started: 12:33:11.434367
    Duration: 33.455 ms
     Changes:   

Summary for tc7
------------
Succeeded: 2
Failed:    0
------------
Total states run:     2
Total run time:   1.428 s
root@c7mstr:/home/david/dev1#

Closing this issue since the output for the postfix has been address from what it was. Given the long arguments in the threads for the above PR's and their associated issues, and not wanting to rehash here, as to what should be reflected with test=True, going to close this issue, as believe it has been adequately addressed by the other PR's. Please reopen if you feel this is unsatisfactory but provide knew information as to why the above PR's do not adequately respond to your concerns.

Note: The above changes should be available with the next major release of Salt, Phosphorous.