Remove unused __orchestration__ ret key

[max-arnold]

What does this PR do?

Remove obsolete __orchestration__ key from return data.

It is set in salt.runner and salt.wheel orchestration states and was supposed to be used in the highstate outputter: #35290

However, it is no longer used in the codebase after the highstate outputter became recursive: #46022

CC: @mattp-

Merge requirements satisfied?

• Docs (N/A)
• Changelog - https://docs.saltstack.com/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

No

[garethgreenaway]

This feels like something we should follow the normal deprecation path for. Add a warning that it will be removed in 2 releases and then delete it then.

[max-arnold]

Does it mean a warning during each orchestration run? Seems to be too noisy for the majority of users who have no idea what this field means

[garethgreenaway]

@max-arnold If it's not actively used anywhere in the code we can probably just have a note there and something in the release notes saying it will be removed in the future. I'd just hate to remove something that someone might be relying on in some capacity without adequate warning.

[max-arnold]

It is not used anywhere in the Salt codebase (see the PRs linked in my first message)

@mattp- Are your Salt installs depend on it in any way?

[mattp-]

@max-arnold nope, no use that I am aware of

[garethgreenaway]

@max-arnold @mattp- If we're confident that no one would be using this information from the return data then I'm comfortable merging this PR.

[dmurphy18]

I have to second @garethgreenaway initial concerns about deprecating this over a couple of releases. Just because we don't see a use for it and don't make use of it, and don't think anyone is using it, doesn't mean that an existing user is not relying on it.

Prefer deprecation

[waynew]

Just echoing above, and as far as mechanisms go, I'd say something like this would be effective:

if not __opts__.get("skip_ret_orchestration"):
    log.warn("'__orchestration__' will be set in ret, but will be removed after XXX. To enable future behavior and suppress this warning, set `skip_ret_orchestration: True` in YYY config.")
    ret["__orchestration__"] = True

That would give them the ability:

1. To be warned, if that's something they rely on now
2. opt-in to the future behavior, not unlike Python's from __future__ import print_function

[mattp-]

@waynew i really dont think that is necessary for something im 99.9% sure noone uses, and adds more churn to an already churned set of config options. why not just document the deprecation and then slate the removal for N releases away

[max-arnold]

I do not like the log warning, because it will add unnecessary noise for the majority of Salt users who have no idea what this obscure field means and who will be forced to look into the new setting. How do you all feel about the following idea (a little bit more aggressive deprecation):

1. Remove the field
2. Add a changelog note ("The field was removed ...")
3. No warning in the log
4. An opt-in setting to get the field back (i.e. absent by default)
5. A deprecation warning in the log if the setting is enabled (to not forget to remove it from Salt codebase)

This means smooth sailing with no distractions for 99.99% of Salt users, a minor inconvenience (enable the opt-in setting) for those from 0.01% who rely on the field AND read the changelog before upgrading to a new major Salt version, and a fixable problem for those who do not read the changelog

[waynew]

@max-arnold I'm OK with that approach. (And after writing the below, I actually really like that approach)

I think you're right that probably nobody is using it. But I'm also pretty sure that because there's always someone relying on some sort of weird edge case for some reason, removing it without remedy will require folks to downgrade.

Like. there's probably some unmanned server that's been boarded up in a wall in a basement somewhere that's relying on this key being in the dictionary, and removing it will cause a meltdown in the financial systems of some township or something.

But your approach - completely remove it, with a note in the log, and allow them to add a flag to re-enable (and then warn if they do that it will be completely removed following our regular deprecation cycle) - is I think the right blend of pragmatism and dogmatic adherence to our deprecation policy.

👍

[dmurphy18]

Well @terminalmage add this back in the 2015 days

Add __orchestration__ key to orch returns for runner/wheel funcs
This gets passed through to the highstate outputter to suppress the
error in the output when the changes param is not a dict.

And I am wondering if this still applies but seeing nothing to check and suppress an error, unused internally as it said.

Fine with it as @waynew state above

[max-arnold]

@waynew I implemented the proposed change. The only strange thing is that I do not see any warnings from warn_until in my 3005.1 onedir install (with the patch applied) on Ubuntu 22.04.

Is warnings.warn logs at all to the master log or terminal? Could you please check whether it is a problem for you as well?

To trigger the warning, add the following snippet to /etc/salt/master.d/opts.conf and restart the master:

features:
  enable_deprecated_orchestration_flag: true

Then run the following orch using salt-run state.orch test_orch --out json:

# test_orch.sls
Runner job:
  salt.runner:
    - name: config.get
    - arg:
        - features

Wheel job:
  salt.wheel:
    - name: key.list
    - match: accepted

[waynew]

🚀

[waynew]

@max-arnold I think there was something that happened recently that mucked with some warnings, but I haven't had time to track it down

[waynew]

☝️ see the above mentions. I had some time this morning to track it down and add a fix 🎉