Inconsistent state return when test=True

[bewing]

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 cbd9590, 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.

[gtmanfred]

@whiteinge and @terminalmage do you have any thoughts on this?

Thanks,
Daniel

[whiteinge]

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.

[bewing]

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.

[whiteinge]

All great suggestions. I agree. I'll point the Core team to this conversation. Some of these are more work than others but all are things we should get to eventually.

[timwsuqld]

Having also observed this issue, it would be good to see it fixed.
Currently, a number of modules/states use comment for showing what will change. (I'm working in acme.cert at the moment). What is the correct way (based on the above discussions) to change this? Should it instead be putting the "comments" in pchanges if it thinks a change would be made when rerun with test=False?

        comment = 'Certificate {0} '.format(name)
        if not __salt__['acme.has'](name):
            comment += 'would have been obtained'
        elif __salt__['acme.needs_renewal'](name, window):
            comment += 'would have been renewed'
        else:
            comment += 'would not have been touched'
        ret['comment'] = comment
        return ret

Would become something like

        pchanges = 'Certificate {0} '.format(name)
        if not __salt__['acme.has'](name):
            pchanges += 'would have been obtained'
        elif __salt__['acme.needs_renewal'](name, window):
            pchanges += 'would have been renewed'
        else:
            pchanges = None
        ret['pchanges'] = pchanges
        return ret

(The acme.cert block has the nasty issue at the moment, where it makes a comment regardless of what's happening, hence it "always" appears to change when test=True)

[whiteinge]

@timwsuqld good addition. Below is a quick checklist. (We should get these in the docs.)

• pchanges should be the same "shape" that changes is in so that they can be meaningfully compared with each other.
• The comment field can and should vary between dry-run and not.
• The changes dict must be empty for dry-run.
• The result field must be None for dry-run.

[mirceaulinic]

Hi - thanks all for the inputs, that really helps me clarify and I've bookmarked this issue and revisit it and go through the checklist when I develop a new state, just to make sure I'm doing it properly.

Regarding:

The result field must be None for dry-run.

Would result need to be None even when the state detects a potential error? On the CLI, at least, I find it useful to have the red colour for error so I'm sure I won't miss this and execute a wet run. In other words, when the state detects an error, shouldn't rather be False? But the price paid is that the highstate outputter will tell Succeeded: 0 (changed=1) and Failed: 1 which is not really accurate.
Looking forward to your thoughts. Thanks!

[whiteinge]

Yes, that has been the pattern even for failures. However the use-case is plenty valid. This same question just arose in #42553.

[mirceaulinic]

Thanks for pointing that out @whiteinge -- having presult sounds good to me.

[aneeshusa]

Overall I really like pchanges and agree with the above recommendations, but I'd like to propose a slight change. On non-dry-runs I think it would be more clear if pchanges is missing or an empty dict - if things have actually changed (and are in changes) it doesn't make sense to me that they are also predicted to changes. To be concrete, for states I've written recently I've been writing change info to pchanges in test mode, and changes in non-test mode.

[oarmstrong]

Sorry to bring this up after a while of silence. I've just submitted the PR #44603 which is a quick fix for the state always returning changes on test=True for acme.cert even when the cert would not be touched. I got the feeling this issue was more for a larger refactor including that state but I didn't delve too deeply into it.

@timwsuqld not sure if you got anywhere with working on acme.cert as you stated above.

Feel free to undo what I've done in any of your work to correct the issue in a nicer way.

[sidharrell]

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.

[angrydead]

@sidharrell I'm guessing that wasn't intentionally removed?

[nokernel]

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.

Yes I also experience the same and the issue is this commit :

98c61ae

in file.py they stopped to populate ret['changes']['diff'] assuming that the output module will display ret['pchanges']['diff']

As a test I reverted that commit, and now I see the changes while test=True for file.managed.

[nokernel]

It's related to this #51932

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[dead10ck]

Was this issue ever resolved?