Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce "module.xrun" #39891

Merged
merged 65 commits into from Mar 21, 2017
Merged

Conversation

isbm
Copy link
Collaborator

@isbm isbm commented Mar 8, 2017

What does this PR do?

Adds a feature: module.xrun for states.

Motivation

Current way of calling Salt modules with module.run has pretty cumbersome interface along with the fact it has also reserved words. This leads to a strange states, something like:

show_friends:
  module.run:
    - name: facebook.list_friends
    - m_name: thatch

Or even worse:

list_states:
  module.run:
    - name: united.states
    - m_states: Florida, Arizona, California
    - m_names: Tallahassee, Phoenix, Sacramento
    - m_fun: Bowling
    - kwargs: {
      'drink_beer': True,
      'visit': [
        'Florida State Capitol',
        'Arizona Science Center',
        'California Railroad State Museum'
      ],
      'call': thatch
    }

This led to the reasoning make it easier and simpler to use, throw away m_ prefix (this just sucks, sorry). Also write better unit tests. 馃槃 So the above can be done something like this:

show_friends:
  module.xrun:
    facebook.list_friends:
      - name: thatch

Look, Ma! No m_ prefix! How about another example:

list_states:
  module.xrun:
    united.states:
      - states: Florida, Arizona, California
      - names: Tallahassee, Phoenix, Sacramento
      - fun: Bowling
      - drink_beer: True
      - visit:
        - Florida State Capitol
        - Arizona Science Center
        - California Railroad State Museum
      - call: thatch

One more thing you can do (optionally, of course, you don't have to):

this_calls_it_at_once:
  module.xrun:
    some.function:
      - foo
      - bar
    another.function:
      - wow
      - that's handy

馃槑

Tests written?

Yes

Copy link
Member

@thatch45 thatch45 left a comment

This looks great, I never liked the module.run interface. I think this is significantly cleaner

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 8, 2017

@thatch45 thanks! :) Before @cachedout pointed me to the Lint, I am already fixing it!!

Copy link
Contributor

@gtmanfred gtmanfred left a comment

馃憤

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 8, 2017

@cachedout I do not understand Lint error of W1699 against zip(... and on top of the source. It works just fine in Python3 and Python2. What exactly is wrong and how to fix it?

Copy link
Contributor

@rallytime rallytime left a comment

Very nice! This will be great to have.

I only have one small request: Can you add this new option to the Nitrogen Release Notes? I think people will find this useful and will want to know about it!

Copy link
Contributor

@whiteinge whiteinge left a comment

Might be worth adding a deprecation notice to module.run for this release or the next.

Given how heavily used it is we might not want to actually put it on a deprecation path but rather just add a notice to that docstring -- the 'ol file.sed treatment. 馃槈

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 9, 2017

@rallytime I added release notes and hopefully it is correct.

@whiteinge I added decorator-based deprecation (and also fixed it is now working for state modules as well). Now it will simply be using new module.run, but you can always restore an old behaviour with the use_deprecated mechanism (and have warning log messages about it every time you calling that). I mentioned all that in examples within the release notes. Complete implementation removal of old module.run is targeting Oxygen. We still got lots of time for it.

Hint for packagers: create a module_run_deprecation.conf file with the configuration mentioned in the release notes and deploy it in your minion package (RPM or deb etc) as a post-install script (so the file doesn't really belong to the package). Then later, when your infrastructure has been migrated, simply delete that config everywhere and restart minions.

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 9, 2017

@cachedout by now all the issues were addressed. If you think those two lints are just misbehaviour of custom scripts (at my side it doesn't hurt for Python 3 actually), then we are clean to merge.

@whiteinge
Copy link
Contributor

@whiteinge whiteinge commented Mar 9, 2017

@isbm sorry my comment above wasn't very clear.

I strongly think that we should not deprecate the old module.run. It's been around for far too long and is used in the wild far too much. The "file.sed treatment" is adding a note to the docstring of the old function that says, "Hey, you should use other function instead," but keeping the old one in the codebase more or less indefinitely.

Thoughts from the Core team? Agree/disagree?

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 9, 2017

@whiteinge But this is what exactly is happening. Add to your minion config:

use_deprecated:
  - module.run

And you will have the old one. All is needed to be done is to package it properly. E.g. we will ship something like /etc/salt/minion.d/module_run_deprecated.conf with the content above. Then you will see those messages in log and you can just "fix" the syntax eventually. Because having both that does the same is kind of pollution (in my eyes). Having that renamed is to lose "run" name, which nothing can be better.

So let's just slowly migrate the world. Two years should be enough to write - foo: bar instead of {foo: 'bar'}. 馃槃

@thatch45
Copy link
Member

@thatch45 thatch45 commented Mar 9, 2017

Both good points, I think that slowly moving the world would be a good idea, we could easily make sure that the comment returned from module.run specifies that it is going to be deprecated...

@whiteinge
Copy link
Contributor

@whiteinge whiteinge commented Mar 9, 2017

I can't speak directly to pros/cons of relying on package upgrades to put that config file in place. Personally, I'd prefer an opt-in rather than an opt-out for such a core module.

That said, I'll defer to the Core team since they will be on the front lines of any user upgrade problems.

@thatch45
Copy link
Member

@thatch45 thatch45 commented Mar 9, 2017

I would also prefer for opt-in, we do not have the luxury that SUSE has of dictating how the platform is going to be used unfortunately :(

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 9, 2017

@whiteinge @thatch45 no problems for opt-in. I will change that in the entire mechanism, so all the deprecation will work always on opt-in with "nag messages" to the log. Good point! But now is 1:00AM in Germany, so let me fix that tomorrow CEST. :-)

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 10, 2017

@thatch45 @whiteinge specially for your preference! Have a nice weekend, guys.

P.S. Hmm, nice deprecation mechanism we've just got, BTW...

ret['result'] = False
elif __opts__['test']:
ret['comment'] = "Module function '{0}' is set to execute".format(func)
ret['result'] = True
Copy link
Contributor

@ScoreUnder ScoreUnder Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could potentially overwrite the ret['comment'] & ret['result'] from a previous loop iteration where it fails, depending on the content of the SLS file and the order of the dict.

This would mask a failure under some circumstances when test=True.

Copy link
Collaborator Author

@isbm isbm Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, knapp, you're right. And I forgot to update docstring. Give me a sec...

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 13, 2017

@ScoreUnder done!

@isbm isbm force-pushed the isbm-translate-varargs branch from 15bdcd6 to 55ab664 Compare Mar 13, 2017
Copy link
Collaborator

@terminalmage terminalmage left a comment

I think the config options can be better retrieved using config.get. There are also a number of things I noticed in the RST that we can clean up.

@@ -61,6 +61,49 @@ State Module Changes
start/stop the service using the ``--no-block`` flag in the ``systemctl``
command. On non-systemd minions, a warning will be issued.

- The :py:func:`module.run <salt.states.module.run>` state dropped its previous
Copy link
Collaborator

@terminalmage terminalmage Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "has dropped".

- The :py:func:`module.run <salt.states.module.run>` state dropped its previous
syntax with ``m_`` prefix for reserved keywords. Additionally, it allows
running several functions in a batch.
**NOTE: you need explicitly turn on new behaviour (see below how)**
Copy link
Collaborator

@terminalmage terminalmage Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be expressed better in RST using:

.. note::
    It is necessary to explicitly turn on the new behavior (see below)

- do_stuff: True

- Previous behaviour of the function :py:func:`module.run <salt.states.module.run>` is
still kept by default and can be bypassed in case you want to use behaviour above.
Copy link
Collaborator

@terminalmage terminalmage Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thatch45 Did we ever make a firm decision on whether to use US or international spelling for words ending in -or/-our?

Copy link
Collaborator Author

@isbm isbm Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not american here, sry.

Copy link
Collaborator

@terminalmage terminalmage Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't care which we use, but I think that we should choose one for consistency across the docs.

Copy link
Contributor

@cachedout cachedout Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are mixed in the docs. I agree we should pick a style but since we haven't, I see no reason to block on this.

Copy link
Collaborator

@terminalmage terminalmage Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, yes I definitely agree.


- Previous behaviour of the function :py:func:`module.run <salt.states.module.run>` is
still kept by default and can be bypassed in case you want to use behaviour above.
Please keep in mind that old implementation will be entirely removed in version "Oxygen".
Copy link
Collaborator

@terminalmage terminalmage Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be

Please keep in mind that the old syntax will no longer be supported in the ``Oxygen`` release of Salt.

still kept by default and can be bypassed in case you want to use behaviour above.
Please keep in mind that old implementation will be entirely removed in version "Oxygen".
In order to access new function behaviour, please add the following configuration to the
minion setup. Note that the configuration below you can also deploy separately to all
Copy link
Collaborator

@terminalmage terminalmage Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note here could also be put in a .. note:: RST block.

I don't understand this note though, to be honest. Are you suggesting that all minions should have their config files edited? Have you considered using config.get to get these configuration options? It checks grains and pillar as well as the minion config file. This would allow for this option to be turned on simply by editing the pillar data (or using grains.setval to set the value in the minion's /etc/salt/grains). Both of these options seem less intrusive IMO than editing all minions' config files.

Copy link
Collaborator Author

@isbm isbm Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@terminalmage good point on grains. I will extend that, but with a different PR, since I would like to have a different tests for it.

- Previous behaviour of the function :py:func:`module.run <salt.states.module.run>` is
still kept by default and can be bypassed in case you want to use behaviour above.
Please keep in mind that old implementation will be entirely removed in version "Oxygen".
In order to access new function behaviour, please add the following configuration to the
Copy link
Collaborator

@terminalmage terminalmage Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would read better as:

To enable the new behavior, add the following to the minion config file:

@cachedout
Copy link
Contributor

@cachedout cachedout commented Mar 13, 2017

Final call for comments on this one. Otherwise, everything is looking good for a merge.

@rallytime
Copy link
Contributor

@rallytime rallytime commented Mar 14, 2017

@cachedout What do you want to do about the PY3 lint errors for the changes in module.py?

https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/9449/violations/file/salt/states/module.py/

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 14, 2017

@rallytime I already asked this 5 days ago: please advice what it does mean. Because the code is not incompatible with Python 3 as lint claims. I would think that the script/lint check is flawed, as it does not provide even the reason and proposed fix. Just complaining and complaining wrong (unless I am missing something).

Suggestions? I can fix it in 5 minutes, but if I know what...

@rallytime
Copy link
Contributor

@rallytime rallytime commented Mar 14, 2017

@isbm Right, all good points.

For the record, and so everyone doesn't have to click around and find it, the console output for the PY3 violation displays the following:

16:14:34 salt/states/module.py:183: [W1699(incompatible-py3-code), ] Incompatible Python 3 code found. Proposed fix:
16:14:34  from salt.ext.six.moves import range
16:14:34  from salt.exceptions import SaltInvocationError
16:14:34  from salt.utils.decorators import with_deprecated
16:14:34 +from salt.ext.six.moves import zip
16:14:34  
16:14:34  
16:14:34  def wait(name, **kwargs):
16:14:34 salt/states/module.py:311: [W1699(incompatible-py3-code), ] Incompatible Python 3 code found. Proposed fix:
16:14:34      :return:
16:14:34      '''
16:14:34      argspec = salt.utils.args.get_function_argspec(__salt__[name])
16:14:34 -    func_kw = dict(zip(argspec.args[-len(argspec.defaults or []):], argspec.defaults or []))
16:14:34 +    func_kw = dict(list(zip(argspec.args[-len(argspec.defaults or []):], argspec.defaults or [])))
16:14:34      func_args = []
16:14:34      for funcset in kwargs.get('func_args') or {}:
16:14:34          if isinstance(funcset, dict):

But, as you stated, this should be working in both versions. If this is something wrong with our linter, then we need to address that.

I am thinking @s0undt3ch or @cachedout should provide some direction on this.

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 14, 2017

@rallytime Hmm. I don't think this is needed fix: the only difference that zip in Python3 returns iterator instead of list. This fix is trying to replicate the behaviour in a sense that it first turns iterator to the list. I personally wouldn't do that, because iterators are better. And I still think that the message is still misleading, if by "incompatible" we normally understand "unable to compile".

@isbm isbm force-pushed the isbm-translate-varargs branch from 36070a8 to 5592b49 Compare Mar 20, 2017
@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 20, 2017

@cachedout OK, today I re-based it one more time against origin/develop for you (this morning and just now). Lint errors at least gone and unit tests works. What else I should do?

@cachedout
Copy link
Contributor

@cachedout cachedout commented Mar 20, 2017

@isbm That's all I think we need. If the tests pass, I'll merge it.

@isbm
Copy link
Collaborator Author

@isbm isbm commented Mar 21, 2017

@cachedout it is all @rallytime 's fault. 馃槅 (no, not really: thanks for adding unit tests for with_deprecated which supposed to be mine technical debt!). I fixed them + added few more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants