salt: [REGRESSION] --output-diff doesn't display changes anymore with test=True since v2019.2.0

Brief discussion between @thatch45 @waynew and myself, we concluded that the diff should be shown and we’ll look to see what changed from 2018.3.x to 2019.2.x to disable this.

Thanks for those informations and for your reactivity.

Even if test=True isn’t perfect and could miss some changes (generally due to triggers like watch / watch_in etc.), it is a huge help for the one who needs to check some particular changes instead of blindly applying states and just hope changes will be good. A note could be displayed on test=True telling the user that changes are juste there for information and can be wrong in certain cases.

To add a last thought to the @rhavenn 's comment, currently, if you misspell “test=True”, the state will be executed without any error or message telling that we passed an unknown argument, which is very dangerous and confusing.

Then, replacing test=True by a special state.test (or maybe state.dryrun) would be a good thing I think,

Hello,

I agree that this is a terrible regression, it makes it impossible to actually see what changes are going to be made in test mode, almost defeating the entire purpose of the test mode (If you can’t see potential changes, then the test mode becomes useful only as a syntax test, to check for errors in the states). We use test mode to capture differences in a nightly audit also which has now become useless without the details of the differences.

Although the change described by @fedepires and tested by @rhavenn supposedly fixes it, there is another alternative fix, potentially more suitable one, because the fix below restores the behaviour for handling all pchanges data from different states, whereas the fix above, only restores the changes displayed within the ‘file’ module. That fix also introduces the ‘mixing’ of ‘changes’ and ‘pchanges’, which people are concerned with above. However the fix below only affects the output produced (choosing to display pchanges if it is available).

The code in highstate.py, used to print ‘pchanges’ if it was present and there were no ‘changes’ but there are ‘pchanges’.

This handling was removed in commit 98cfa1ff464c1008e5c08f02bd50836cd33de87c “recursion for orchestration” (file salt/output/highstate.py, line 226), and may have simply been an oversight.

this fix is as follows (simply restoring the removed logic) from that commit

Around line 246 of salt/output/highstate.py, in salt version 2019.2.0, within function _format_host :

    schanged, ctext = _format_changes(ret['changes'])
    + if not ctext and 'pchanges' in ret:
    +     schanged, ctext = _format_changes(ret['pchanges'])
    nchanges += 1 if schanged else 0

I would only add that a) This is a major feature when trying to merge a Salt state into systems that have not previously been managed by Salt and have years of manual changes in them. One never knows what setting you’re going to be accidentally removing. b) If test=true isn’t the grammitically correct way this is envisioned then adding a state.test that does the same thing is perhaps more grammatically correct?

Agreed, and when we get it back, lets add a fat comment in there to keep it 😃

Can we please either reopen this or https://github.com/saltstack/salt/issues/40208 dry-run diff output is extremely important to us, and we’re having to patch every salt-minion.

Here’s my way to rollout this patch to all salt minions:

put this in a salt sls file:

########################### salt 2019.2.0 diff fix
{% if grains.saltversion=="2019.2.0" and not grains.patched_testdiff|default(false)  %}

{{ grains.saltpath }}/output/highstate.py:
    file.patch:
        - source: salt://patches/2019.2.0.testdiff.patch
        - require:
            - pkg: patch

patched_testdiff:
        grains.present:
        - value: true
        - require:
            - {{ grains.saltpath }}/output/highstate.py
{% endif %}

add @cjsin 's patch in /srv/salt/patches/2019.2.0.testdiff.patch:

--- highstate.py.orig   2019-05-17 11:33:44.442310662 +0200
+++ highstate.py        2019-05-17 11:33:33.250925936 +0200
@@ -244,6 +244,8 @@
                 nchanges += 1
             else:
                 schanged, ctext = _format_changes(ret['changes'])
+                if not ctext and 'pchanges' in ret:
+                   schanged, ctext = _format_changes(ret['pchanges'])
                 nchanges += 1 if schanged else 0
 
             # Skip this state if it was successful & diff output was requested

edit: use saltpath from grains, since salt is not always in the same location.

In case it helps anyone else, I’ve slightly tweaked the patch in https://github.com/saltstack/salt/issues/51932#issuecomment-474213938 for my own version, which results output like:

     Comment: The file ${FILE} is set to be changed
[...]
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -25,9 +25,11 @@
[...]

To do this I’ve changed the source (again around line 246 in 2019.2.0, salt/output/highstate.py):

            else:
                schanged, ctext = _format_changes(ret['changes'])
                # Edited 2019-05-09, to fix regression
                # See: https://github.com/saltstack/salt/issues/51932#issuecomment-474213938
                if not ctext and 'pchanges' in ret:
                    _, ctext = _format_changes(ret['pchanges'])
                nchanges += 1 if schanged else 0

Which avoids updating schanged, and only picks up the ctext value from pchanges (sticking the unwanted value into _ to discard it).

Possibly this is more logical output in test mode?

(I definitely want to see the potential diff that will be applied in test mode. That’s about 90% of why I run a highstate in test mode.)

Ewen

I like it, maybe say: Note: No changes made, actual changes may be different due to other states.

Based on this comment thread, it sounds like this was patched/fixed in 2019.2.1. We just upgraded to 3000.2 and we see that it is broken/missing again. Should the file module show a DIFF for test=True executions in 3000.2 ?

Edit: I tried this with --output-diff and it still didn’t show the file changes.

This must be a reintroduction of the bug in the 3000.x versions. We would love it if this could be fixed 😃

Correct, #51992 is labeled to be merged for the 2019.2.1 release.

@sagetherage sorry for late answer, here it is:

Salt is installed from pip.

salt-call --versions-report
Salt Version:
           Salt: 2019.2.5
 
Dependency Versions:
           cffi: 1.13.2
       cherrypy: 17.4.0
       dateutil: 2.8.1
      docker-py: 1.10.6
          gitdb: 2.0.6
      gitpython: 2.1.15
          ioflo: Not Installed
         Jinja2: 2.10.3
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.32.0
           Mako: 1.1.0
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: 2.19
       pycrypto: 3.9.4
   pycryptodome: 3.9.4
         pygit2: Not Installed
         Python: 2.7.17 (default, May 14 2020, 06:37:18)
   python-gnupg: 0.4.6
         PyYAML: 5.2
          PyZMQ: 18.1.1
           RAET: Not Installed
          smmap: 3.0.4
        timelib: 0.2.4
        Tornado: 5.1.1
            ZMQ: 4.3.2
 
System Versions:
           dist: centos 7.7.1908 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-1062.el7.x86_64
         system: Linux
        version: CentOS Linux 7.7.1908 Core

Note to mention, that file.serialize shows diffs, but file.manage dont.

this was a crucial part of test=true, seeing actual changes to files

sigh…

Re: https://github.com/saltstack/salt/issues/51932#issuecomment-493394790

Here’s my way to rollout this patch to all salt minions:

I think the output formatting is done in the UI layer on the salt master, rather than the minions. At least I’ve only patched this (https://github.com/saltstack/salt/issues/51932#issuecomment-490664079) on my salt master, and do seem to get sensible results from testing against various salt minions (not just the one that’s also the salt master).

But @psy0rz that’s still a very cool trick for doing self-patching of Salt, just once, that I’ll keep in mind for future use 😃

Ewen

If it’s of any help, from our tests, looks like this commit might be the culprit:

https://github.com/saltstack/salt/commit/98c61ae22ef108adcf9101d0ca9505f6babb5e13

#44353

We have gone back and forth on this one a lot over the years. The core argument has been that the changes dict should only have changes in it if changes actually happened. Thats why it keeps getting pulled out. The main argument is that this is the expected behavior.

The counter argument is that this is some very valuable information to have from a test=True run and that it should be made available.

The counter/counter to that is that we don’t have the ability to get predictive changes out of all states, therefore the output becomes inconsistent.

Finally, it can also be argued that if the result is None then we should know for sure that we did not make changes, so any changes in the changes dict can be seen as predictive.