salt: Inconsistent state return when test=True

While working on a change to fix up the return of some network configuration diffs when test=True or when no changes are made, I ran into a lot of inconsistencies in the code base and documentation regarding the correct way to return potential changes when testing.

From the addition of ret['pchanges'] in https://github.com/saltstack/salt/commit/cbd9590dcb3668f33c123d1ab44608b74cab61b2, it appears that there was a desire to differentiate when changes are ACTUALLY made by salt, and when changes WOULD be made, but weren’t because we were testing. This is kind of reflected in the highstate outputter, as the summary counts the # of states that have ret['changes'] not empty as changed and # of states with ret['result'] == None as unchanged. This can still be seen as states such as file.directory will use ret['pchanges'] to populate information in ret['comments'] to indicate what would happen if test == False

Currently, this results in confusing output, as modules like file.managed will set ret['changes']['diff''] = ret['pchanges']['diff'], resulting in the summary stating that it was both changed and unchanged at the same time.

I think this was due to some regressions where the file diffs were getting lost, as the highstate outputter does not show ret['pchanges'] at all. Perhaps we should update the highstate outputter to display ret['pchanges'] instead of ret['changes'] when ret['result'] == None, and update documentation to indicate that ret['changes'] should only be populated when a change was actually made.

I’d hope that this prompts a discussion to create something definitive on how states return and display potential data. I apologize if this has already been brought up – my search for issues didn’t turn up anything directly relevant.

About this issue

Original URL
State: closed
Created 7 years ago
Reactions: 1
Comments: 17 (11 by maintainers)

Commits related to this issue

Refactor processing return data from load_config Move default_ret and loaded_ret into salt.utils.napalm, and update the dictionary returned based on discussions in #40208. Diffs are copied into comme... — committed to bewing/salt by bewing 7 years ago
Fix acme state to correctly return on test Previously, the acme.cert state would return that a change was pending during a test run even in the case that the certificate would not have been touched. ... — committed to rallytime/salt by oarmstrong 7 years ago

Most upvoted comments

Thanks much for the quick response. It appears that I was on the right track with my assumptions. Below are my thoughts as a new contributor to saltstack.

IMHO (with which $2 will get you a coffee), the pchanges enhancement is a great idea that should be advanced more aggressively. My suggestion would be:

Update documentation to promote storing predictive changes in new/updated state modules (it is not clear from the single line mentioning pchanges today)
Also state that wherever possible, changes should be the actual changes made on the minion, instead of copying pchanges and hoping nothing underlying went wrong
Recommend copying pchanges to comments in existing modules utilizing pchanges if result is None in the interim, as a bridge
Revisit existing modules utilizing pchanges to enforce above (states.file.managed, etc)
Update unit tests to enforce changes == {} if result is None IF pchanges is present (in effect grandfathering in state modules that do not utilize it)
Update highstate outputter to sub pchanges for changes if result is None (it appears to be the only one that references changes directly – the rest walk recursive dictionaries)
Update documentation to remove pchanges to comments recommendation
Put pressure on PRs for existing modules to meet the new standard

To my knowledge, we haven’t yet added pchanges support to any of the outputters so this field should only be visible when looking at the raw data. I’d like to see more module adopt usage (and gain more consistent formatting).

From my quick survey, it’s really only highstate that directly references/calls out changes directly. In terms of seeing modules adopt pchanges, I think the documentation, teaching, enforcement approach makes sense. I’ve never mananged an open source project thought, so I’m prepared to be told otherwise.

bewing on Mar 21, 2017

2019.2 release lacks diff output when running salt ‘*’ state.apply test=True saltenv=“production” I ran the minion in debug mode, the diff output is there. I reverted to the 2018.3 release, diff output in test mode returned.

sidharrell on Feb 26, 2019

Your sleuthing is commendable. This is an in-progress feature addition and I don’t think we ever documented or explained it.

The history behind this is pchanges was added to give a home for structured data regardless if the module is run in dry-run mode or not. The official recommendation for dry-run returns is that changes must be empty and that result must be None. This lead to the problem of stuffing useful data (like a file diff) inside of the comment field. But, as you pointed out, not all modules adhere to this recommendation and we don’t have anything in place at the moment to enforce it.

pchanges is a shorthand for “predictive changes” because we can make guesses about how a system will be changed but there are some things we can’t (easily) know until after the change is actually made. A simple example is default groups when adding a user or the default umask for a file if a non-standard one has been configured on the system and the state didn’t specify one. So pchanges and changes should usually match for non-dry-runs, but sometimes changes should have more information.

To my knowledge, we haven’t yet added pchanges support to any of the outputters so this field should only be visible when looking at the raw data. I’d like to see more module adopt usage (and gain more consistent formatting).

Does that answer your questions? I’m interested to hear your thoughts.

whiteinge on Mar 21, 2017