Salt: [REGRESSION] --output-diff doesn't display changes anymore with test=True since v2019.2.0
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
tomlaredo
All 44 comments
@saltstack/team-core Thoughts?
garethgreenaway
on 4 Mar 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.
thatch45
on 5 Mar 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.
garethgreenaway
on 5 Mar 2019
Agreed, and when we get it back, lets add a fat comment in there to keep it :)
thatch45
on 5 Mar 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.
tomlaredo
on 5 Mar 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.
waynew
on 5 Mar 2019
I like it, maybe say:
Note: No changes made, actual changes may be different due to other states.
thatch45
on 5 Mar 2019
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
fedepires
on 5 Mar 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
on 5 Mar 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
rhavenn
on 6 Mar 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 :)
waynew
on 6 Mar 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,
tomlaredo
on 6 Mar 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 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
cjsin
on 19 Mar 2019
The @cjsin 's patch works perfectly for me. Thanks!
madrisan
on 21 Mar 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.
dpkirchner
on 5 Apr 2019
@dpkirchner From what I can tell this should be landing in 2019.2.1, but @garethgreenaway can correct me if I'm wrong.
waynew
on 5 Apr 2019
Correct, #51992 is labeled to be merged for the 2019.2.1 release.
garethgreenaway
on 6 Apr 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.
amendlik
on 29 Apr 2019
Should we go ahead and close this issue then?
waynew
on 30 Apr 2019
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
ewenmcneill
on 8 May 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
on 17 May 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.
psy0rz
on 17 May 2019
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
ewenmcneill
on 17 May 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.
psy0rz
on 17 May 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.
zhukovra
on 19 Jun 2019
2019.2.1 has just been released but this is still open. I guess it means it's not fixed yet, is it?
rsuarezsoto
on 26 Sep 2019
@rsuarezsoto as I can see - commit 6c0dcf2 is merged in 2019.2.1 branch, so @garethgreenaway can close the issue.
zhukovra
on 26 Sep 2019
Thank you @zhukovra , I've just checked and it's indeed working again.
rsuarezsoto
on 26 Sep 2019
This may be fixed for file states, but there are others that don't return changes during test, like zabbix_host.
OrangeDog
on 18 Oct 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.
amendlik
on 18 Oct 2019
Just noting that this also seems to be an issue with 2018.3.4. I know it's unlikely to be fixed at this point.
xcorvis
on 22 Nov 2019
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.
sidharrell
on 29 Jan 2020
this was a crucial part of test=true, seeing actual changes to files
sigh...
perfecto25
on 6 May 2020
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 :-)
wes-novack
on 9 May 2020
the fix posted by psy0rz works, its just hacky though.
perfecto25
on 9 May 2020
Somehow I mistakenly added the next release label to this issue, merely a mistake. It is working in master and will be released in Sodium you can find the RC for Sodium in these places:
GitHub Release tag: https://github.com/saltstack/salt/tree/v3001rc1
PyPi: https://pypi.org/project/salt/3001rc1/
The RC packages can be found here: https://repo.saltstack.com/salt_rc/
Installation instructions: https://docs.saltstack.com/en/master/topics/releases/releasecandidate.html
sagetherage
on 4 Jun 2020
Hi,
this issue seems to pop out again with version 2019.2.5
marceliq
on 5 Jun 2020
@marceliq can you give your set up and Salt version report, please?
sagetherage
on 9 Jun 2020
@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.
marceliq
on 18 Jun 2020
@marceliq what state are you using and where exactly are you see this happening? I want to gather as much information as I can so we can really narrow down where this is still occurring and get it corrected. Thank you!
sagetherage
on 19 Jun 2020
removing this from a release cycle until unblocked
sagetherage
on 8 Dec 2020
@sagetherage what is the blocker on this bug? I would love to see it fixed. Anything I can do to help unBlock it?
wwalker
on 16 Dec 2020
@wwalker @sagetherage I'm going to dig in here to sort out where the issue stands.
oeuftete
on 16 Dec 2020
AFAICT, this is resolved in minions ≥ 2019.2.1, as noted in https://github.com/saltstack/salt/issues/51932#issuecomment-480435427.
It's possible the issue is still being seen based on an assumption that the master version matters here. It does not, the behaviour here is fully driven by the minions. Here's an example with a 3002.2 master with a 2018.3.5 minion (old) and a 3002.2 minion (master1).
# salt '*' state.apply gh51932.test test=True
old:
----------
ID: /tmp/dest_file.txt
Function: file.managed
Result: None
Comment: The file /tmp/dest_file.txt is set to be changed
Started: 23:35:59.480332
Duration: 12.141 ms
Changes:
Summary for old
------------
Succeeded: 1 (unchanged=1)
Failed: 0
------------
Total states run: 1
Total run time: 12.141 ms
master1:
----------
ID: /tmp/dest_file.txt
Function: file.managed
Result: None
Comment: The file /tmp/dest_file.txt is set to be changed
Note: No changes made, actual changes may
be different due to other states.
Started: 23:36:00.331591
Duration: 8.46 ms
Changes:
----------
diff:
---
+++
@@ -1 +1 @@
-dest
+source
Summary for master1
------------
Succeeded: 1 (unchanged=1, changed=1)
Failed: 0
------------
Total states run: 1
Total run time: 8.460 ms
Related issues
golmaal
·
3Comments
saurabhnemade
·
3Comments
zieba88
·
3Comments
layer3switch
·
3Comments
Inveracity
·
3Comments
Most helpful comment
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.