Outputters mutate data which can be a problem for Runners and perhaps other things

[whiteinge]

Description of Issue/Question

The highstate outputter mutates data on purpose (https://github.com/saltstack/salt/blob/547ada9418442eaa9e87e380c411ac5e0d9a52e1/salt/output/highstate.py#L135-L141) which causes two problems:

1. If the outputter is invoked before the result is saved to the job cache then we lose part of the return.
2. If the outputter fails and falls back to another outputter we lose part of the return.

Steps to Reproduce Issue

# Add to the test runner:
def datatest():
    return {
        'data': 'dataishere',
        'notdata': 'otherthingsarehere',
        'outputter': 'highstate',  # remove this and 'data' _is_ written to the job cache
    }

1. Run salt-run test.datatest.
2. The highstate outputter is invoked because we told it to but fails and falls back to the nested outputter.
3. Observe the data key is now missing.

and

1. Run salt-run test.datatest --async.
2. Grab the JID.
3. Run salt-run jobs.lookup_jid <jid> and observe that data is missing.

Versions Report

At least 2016.3 and onward through develop.

[whiteinge]

@cachedout follow-up to our earlier conversation. There's an additional problem when one outputter fails and falls back to another. The PR above should side-step the immediate 🔥 but I think whether outputters should mutate, and if they do how to test/control for that should be revisited at some point in the future.

[whiteinge]

Oh, and thank you for your help in tracking this down.

[bstevenson]

@whiteinge @cachedout I've run into this issue too. I made a comments in the original change which pops the data: https://github.com/eyj/salt/commit/6f26237574247c3431e75062dcce39c038074f9d

My use case is trying to return orchestration output to devs via the rest API. I grab the job_id and then pull for the return data. The return data used to show the results of orchestration via a salt-run jobs.lookup_jid <job_id> but now that data isn't returned. My monkey patch works

if 'return' in data:
    data = data['return'].pop('data')

but not convinced it's the right path or if it'll break anything else.

[stale[bot]]

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.