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

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

Closed
tomlaredo opened this issue Mar 1, 2019 · 30 comments

Comments

@tomlaredo
Copy link

commented Mar 1, 2019

Description of Issue/Question

--output-diff doesn't display changes anymore with test=True since v2019.2.0

Setup

# cat state.sls 
/tmp/dest_file.txt:
    file.managed:
        - source: /tmp/source_file.txt

Steps to Reproduce Issue

Initial setup

echo new_content > /tmp/source_file.txt
echo old_content > /tmp/dest_file.txt

With test=True

# salt-call state.apply state --output-diff test=True
[INFO    ] Got list of available master addresses: [u'salt-master', u'localhost']
[INFO    ] Loading fresh modules for state activity
[INFO    ] Running state [/tmp/dest_file.txt] at time 16:36:21.779602
[INFO    ] Executing state file.managed for [/tmp/dest_file.txt]
[INFO    ] The file /tmp/dest_file.txt is set to be changed
[INFO    ] Completed state [/tmp/dest_file.txt] at time 16:36:21.797227 (duration_in_ms=17.626)
local:
----------
          ID: /tmp/dest_file.txt
    Function: file.managed
      Result: None
     Comment: The file /tmp/dest_file.txt is set to be changed
     Started: 16:36:21.779601
    Duration: 17.626 ms
     Changes:   

Summary for local
------------
Succeeded: 1 (unchanged=1)
Failed:    0
------------
Total states run:     1
Total run time:  17.626 ms

Diff isn't displayed anymore.

Without test=True

vs-salt-master:/srv/salt# salt-call state.apply state --output-diff
[INFO    ] Got list of available master addresses: [u'salt-master', u'localhost']
[INFO    ] Loading fresh modules for state activity
[INFO    ] Running state [/tmp/dest_file.txt] at time 16:36:29.781272
[INFO    ] Executing state file.managed for [/tmp/dest_file.txt]
[INFO    ] File changed:
--- 
+++ 
@@ -1 +1 @@
-old_content
+new_content

[INFO    ] Completed state [/tmp/dest_file.txt] at time 16:36:29.800569 (duration_in_ms=19.296)
local:
----------
          ID: /tmp/dest_file.txt
    Function: file.managed
      Result: True
     Comment: File /tmp/dest_file.txt updated
     Started: 16:36:29.781273
    Duration: 19.296 ms
     Changes:   
              ----------
              diff:
                  --- 
                  +++ 
                  @@ -1 +1 @@
                  -old_content
                  +new_content

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:  19.296 ms

Diff is displayed.

Versions Report

# salt-call --versions-report
Salt Version:
           Salt: 2019.2.0
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: 3.5.0
       dateutil: 2.5.3
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.24.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.13 (default, Sep 26 2018, 18:42:22)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.4.3
            ZMQ: 4.2.1
 
System Versions:
           dist: debian 9.8 
         locale: UTF-8
        machine: x86_64
        release: 4.15.18-9-pve
         system: Linux
        version: debian 9.8 
@garethgreenaway

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@thatch45

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

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.

@garethgreenaway

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

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.

@thatch45

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

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

@garethgreenaway garethgreenaway self-assigned this Mar 5, 2019
@tomlaredo

This comment has been minimized.

Copy link
Author

commented Mar 5, 2019

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.

@waynew

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

I'm a fan of adding a note in there, maybe something like Comment: File /tmp/dest_file.txt updated. Note: Actual changes may be different due to other states.

@thatch45

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

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

@fedepires

This comment has been minimized.

Copy link

commented Mar 5, 2019

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

98c61ae

#44353

@rhavenn

This comment has been minimized.

Copy link

commented Mar 5, 2019

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?

@rhavenn

This comment has been minimized.

Copy link

commented Mar 5, 2019

I can confirm reverting the change @fedepires mentions and restarting my remote minion "fixes" the issue and shows the diffs when running state.apply with test=true

@waynew

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@fedepires Nice find :) (and helpful for those who need it.

@rhavenn At least from those involved in the minor discussion we had, we didn't have strong feelings about calling it something else. My cursory search didn't find anything to the contrary. I'd say that my personal preference would be to fix this back (with the aforementioned comment), with the idea that test=True is basically equivalent to git's dry-run flag.

Maybe if someone really feels strongly about the semantics/grammar they could submit a PR to introduce a dry-run flag and re-un-restore the behavior for test=True :)

@tomlaredo

This comment has been minimized.

Copy link
Author

commented Mar 6, 2019

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,

@cjsin

This comment has been minimized.

Copy link

commented Mar 19, 2019

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 98cfa1f "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
@madrisan

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

The @cjsin 's patch works perfectly for me. Thanks!

@dpkirchner

This comment has been minimized.

Copy link

commented Apr 5, 2019

Can confirm, this works great. Is there a plan to merge this change into master? This was hands down the most valuable feature of salt (and of configuration management tools in general) so it would be great to have it back without manually patching.

@waynew

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@dpkirchner From what I can tell this should be landing in 2019.2.1, but @garethgreenaway can correct me if I'm wrong.

@garethgreenaway

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

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

@amendlik

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

The long-running debate over pchanges was settled in #45895. There have been changes sitting on the develop branch since October that remove pchanges entirely, from every module.

@waynew

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Should we go ahead and close this issue then?

@ewenmcneill

This comment has been minimized.

Copy link

commented May 8, 2019

In case it helps anyone else, I've slightly tweaked the patch in #51932 (comment) 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

@psy0rz

This comment has been minimized.

Copy link

commented May 17, 2019

I also agree this is an essential feature. I use it all the time to see and merge the manual changes my customers did.

If its not perfect, we should indeed add a note or make something like state.test. and make it a best effort thing.

@psy0rz

This comment has been minimized.

Copy link

commented May 17, 2019

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.

@ewenmcneill

This comment has been minimized.

Copy link

commented May 17, 2019

Re: #51932 (comment)

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 (#51932 (comment)) 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

@psy0rz

This comment has been minimized.

Copy link

commented May 17, 2019

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 :-)

grains are a really awesome way to keep state like this with minimal overhead. :)

you can find them in /etc/salt/grains on every minion.

@zhukovra

This comment has been minimized.

Copy link

commented Jun 19, 2019

@ewenmcneill you are right. I've applied patch to master and now can see diff in files on minions when working with master commands.

But if you want see the diff on minions with "salt-call state.highstate test=True" patch is also need to be applied.

@rsuarezsoto

This comment has been minimized.

Copy link

commented Sep 26, 2019

2019.2.1 has just been released but this is still open. I guess it means it's not fixed yet, is it?

@zhukovra

This comment has been minimized.

Copy link

commented Sep 26, 2019

@rsuarezsoto as I can see - commit 6c0dcf2 is merged in 2019.2.1 branch, so @garethgreenaway can close the issue.

@rsuarezsoto

This comment has been minimized.

Copy link

commented Sep 26, 2019

Thank you @zhukovra , I've just checked and it's indeed working again.

@waynew waynew closed this Oct 1, 2019
@OrangeDog

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2019

This may be fixed for file states, but there are others that don't return changes during test, like zabbix_host.

@amendlik

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2019

@OrangeDog That is because there is no changes element in the returned dictionary. https://github.com/saltstack/salt/blob/2019.2.1/salt/states/zabbix_host.py#L248-L260

You should probably open a new issue on that state, and see issue #45895 for an example of why the changes element is very import for test mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.