#39891 makes "legacy style" module.run states incompatible with future versions

[dkfsalt]

Description of Issue

GH-39891 creates a "legacy style XOR new style" situation where all old style states in the wild will no longer work, This means that there is a lot of community content that will simply fail to work once sodium is shipped.

This creates unnecessary work for anyone with states that work (if it ain't broke, don't fix it). It also means that anyone that's already chosen to use "new style" syntax can't use old style states without modifying them.

This is just a barrier to entry/usage of the system and I can foresee a situation where some people's production workloads will just stop working because of this when folks upgrade to sodium.

re: the warning that this is happening in Sodium - I was using the product for five months before I'd hit this issue - and if I'd not decided to "opt in" to the new syntax, I wouldn't have seen the issue until I'd upgraded to sodium. And it fails in a non-obvious way.

The module.run should be changed such that it can parse both the old and new style states.

Steps to Reproduce Issue

Run a new-style on a default pre-sodium system or run a legacy style state on a minion with the module.run set to superceded.

[thatch45]

Thanks, @dkfsalt, I completely agree.

[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.

[stale]

Thank you for updating this issue. It is no longer marked as stale.

[cr1st1p]

I also recommend keeping old behavior always ON.
I have new style activated because I like it more, but I also depend on some external 3rd party formulas that are using the old style.

Also, I have to do more checks to find the real cause, but one of the formulas did NOT gave me any error but also did not do ANYTHING. After a couple of hours trying to figure out what was the issue, when I changed the formula to use the new module.run call style, it worked :-(

[xeacott]

@cr1st1p So I'll be handling these changes and I've been updating myself on all the issues/PR's open for this... and want to make sure we're on the same page (please correct me if my thinking is wrong 😄).

1. So we want to keep the old style enabled by default, with setting the following in the minion/master config files to actually use the new style by default.

use_superseded 
  - module.run

2. As well as removing the deprecation warning from the state, updating the docs surrounding the configs, and finally...
3. Adding logic to the state file to handle both of these situations (old by default, otherwise parse as new style).

Sound like a plan?

[cr1st1p]

First, strategically speaking, you guys have to decide what you want long term with the way of
using module.run - new style only or both?
If you decide you want to do it at some point, then you really need to:

• decide to do that in some kind of 'major' version change (2021.0 ?)
• clearly notify people very early, so they get a chance to change things
• clearly notify people that the new upcoming 2021.0? version will deprecate old style
• if possible, do a search on github for code that uses the old syntax and raise an issue in those projects

A. Anyway, you need to support BOTH ways of calling module.run. Either indefinitely, or until you deprecate it, based on
decision above.

Because you need to support both, as per A, automatically, there is no reason for the existence of the setting superseded: module.run

Task wise, then:
1 remove documentation references regarding superseded: module.run
2 remove code regarding that setting

3 wrap the current 2 versions of module.run in a separate function, that will do the detection of which implementation to run.
Independent of any setting, because you need to support BOTH modes regardless of any possible setting (which, as
said above, you should remove)
Implementation must be good, because it is important to correctly choose the implementation to run.
Also, it needs to take into consideration that user might have some mistakes in its calls.
4 update module.run documentation to say you support two modes; that preferably you should use the new one.
And, based on decision from the beginning of this text, that old style will be removed with version X
5 if decision is to deprecate old style at some point, add some deprecation warning, in code, on the old style implementation

I think you could start with 2 and 3 and maybe even release, until you get the decision in place.

Regards.

[max-arnold]

IMO, the new module.run style is better. While breaking compatibility is bad, we still need to make the transition happen (otherwise nothing will change).

There are open-source project examples (e.g., Django) that manage deprecations fairly well (again, IMO).

It is worth placing some nudges in the right direction:

1. Emphasize the new style across the docs
2. De-emphasize the old style (although it should be findable, place it in a footnote and add a warning note)
3. Add more examples of how to convert states from old to new style (I always struggle to remember how to place args/kwargs)
4. Return back the warning to remind people to convert the states. One idea is to inject a warning into the old-style states (via the ret dictionary) so they are visible in the higstate output (and not just in a minion log). Maybe even add the automatically converted syntax into the warning message

Finally, choose the deprecation date and deprecate the old style.

[xeacott]

Thanks for the detailed response guys - the current view point from the core team is to not deprecate the old style at all. This is because we don't wish to break any states.

I agree, @max-arnold that we'll probably emphasis in the docs the new style so it becomes more apparent which style is favored, while also having the old style present.
@cr1st1p I like the idea of having a wrapper around the function to do some decision making first which style to parse, probably will go that route unless a better idea comes forth.

I will probably swing this by the core team and see what is the most appropriate moving forward.

[xeacott]

I wanted to ask you @max-arnold and @cr1st1p for a little bit of clarity on this, as I may be missing something here. So I have a state file like so...

# Legacy style
run_my_state:
  module.run:
  - name: foo.bar
  - m_name: some name
  - m_names:
    - such names
    - very wow
  - m_state: Arkansas
  - m_fun: Such fun
  - m_saltenv: Salty

# New style
run_new_state:
  module.run:
    - foo.bar:
      - name: hello
      - names: Another one
      - state: Arkansas
      - fun: No fun
      - saltenv: Hi

And inside /srv/salt/_modules/foo.py I've written a function called bar that looks like so..

def bar(name, names, fun, state, saltenv):
    ret = {}
    ret = {
        'name': name,
        'names': names,
        'fun': fun,
        'state': state,
        'saltenv': saltenv,
    }
    return ret

and upon applying the state file, without having my minion configured with use_superseded both of these states run and return fine. I looked through all of module.run and it doesn't appear to consume the flag nor does it read __opts__ and look for use_superseded. Unless I am missing something critical, it doesn't appear that a wrapper function needs to be written, and the helper functions inside module.py map the resevered m_ prefix args already...

[cr1st1p]

The fact that your simple module worked for you...
a) maybe you did not correctly set up the superseded option
b) maybe it is too simple.
I encountered a state file written for the old style and that did not error and neither did anything at all, when superseded mode was activated. Took me hours to figure out what was the cause.

Code wise, the flag is consumed via a decorator.
A quick look, at salt 2019.2.3 on my machine, file states/module.py, around line 222,
there is

    221 @with_deprecated(globals(), "Sodium", policy=with_deprecated.OPT_IN)
    222 def run(**kwargs):

This is the new style function.
The decorator will take the function's name - i.e. 'run' and will run, when in old style, the name '_' + 'run', and you can find '_run' around line 368

[max-arnold]

upon applying the state file, without having my minion configured with use_superseded both of these states run and return fine. I looked through all of module.run and it doesn't appear to consume the flag nor does it read opts and look for use_superseded. Unless I am missing something critical, it doesn't appear that a wrapper function needs to be written, and the helper functions inside module.py map the resevered m_ prefix args already...

@xeacott See the @with_deprecated decorator source code (this is where use_superseded is handled):

https://github.com/saltstack/salt/blob/master/salt/utils/decorators/__init__.py#L536-L537

This is the new-style module.run implementation:

https://github.com/saltstack/salt/blob/master/salt/states/module.py#L349

And this is the old-style implementation:

https://github.com/saltstack/salt/blob/master/salt/states/module.py#L459

One of the differences is how args/kwargs are passed:

old_style:
  module.run:
    - name: test.arg
    - args:
      - arg1
      - arg2
    - kwargs:
        kw1: kwarg1
        kw2: kwarg2

new_style:
  module.run:
    - test.arg:
        - arg1
        - arg2
        - kw1: kwarg1
        - kw2: kwarg2

The expected return should be:

    Function: module.run
      Result: True
     Comment: test.arg: Success
     Started: 04:48:09.621554
    Duration: 25.697 ms
     Changes:
              ----------
              test.arg:
                  ----------
                  args:
                      - arg1
                      - arg2
                  kwargs:
                      ----------
                      kw1:
                          kwarg1
                      kw2:
                          kwarg2

[max-arnold]

See also: #43130 #50348

[prometheanfire]

{%- if ( 'module.run' in salt['config.get']('use_superseded', default=[]) ) or ( grains.saltversioninfo[0] >= 3001 ) %} seems will be needed unless both versions can be supported at the same time. I think, right now, the main thing that's needed is a particular version the cut over will happen and migration path for any consumers of the module.

[OrangeDog]

The best way to get people to migrate away from deprecated constructions is to provide migration tools that do it for them. See e.g. Python (though unfortunately once you start using six it's too late to use 2to3, I don't see a 6to3 but it would be very useful for salt right now), Java (provided by third parties like JetBrains), Angular.

It should be easy to script migration of plain yaml, and that could even be used as an sls filter (i.e. #!jinja|salt-deprecation|yaml) if it's too hard to migrate jinja sources.

[max-arnold]

A module that can automatically handle the differences between the module.run variants already exists: #57919 (comment)

[dwoz]

Resolved by #58763