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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate some state requisites #55974

Merged
merged 29 commits into from
May 7, 2020
Merged

Consolidate some state requisites #55974

merged 29 commits into from
May 7, 2020

Conversation

mchugh19
Copy link
Contributor

@mchugh19 mchugh19 commented Jan 25, 2020

What does this PR do?

Attempts to unify some of the mod_run_check state implementations. Global requisites onlyif and unless now support use of salt modules as well as slots, and custom implementations were falling behind (especially the commonly used cmd.run state).

As this adjusts some specific state behavior, review is encouraged.

  • states.cmd
    Done
    Included unique creates feature. Shorthand for unless: test -f <filename> or list of names.
    PR implements creates as a top level state requisite and remove special mod_run_check from cmd. (Left undocumented as the few states using it have mention it individually)

  • states.docker_container
    Done
    Also supported unless, onlyif, and creates.
    Removes special mod_run_check in favor of global creates.

  • states.git
    Done
    I might have missed something, but this didn't appear to implement anything other than unless and onlyif. Removed special mod_run_check.

  • states.macpackage
    Allows passing user runas to onlyif and unless command. Kept the unique implementation.
    See comment Consolidate some state requisites #55974 (comment)
    I think the macpackage implementation is broken and should be removed along with the local references to unless, onlyif, and user.

  • states.file
    Runs command specified in check_cmd but also allows for additional tmp_dir and tmp_ext. Kept the unique implementation.
    Nope, file just has a mod_run_check_cmd

What issues does this PR fix or reference?

Fixes: #55938
#51846 (comment)
Fixes: #56329

Previous Behavior

Unique implementations of onlyif and unless act differently for different states

New Behavior

cmd, docker_container, and git state behavior is now more consistent.

Tests written?

Yes - with high likelihood others need adjustment

Commits signed with GPG?

No

@waynew @max-arnold thoughts?

@mchugh19 mchugh19 requested a review from a team as a code owner January 25, 2020 18:29
@ghost ghost requested a review from xeacott January 25, 2020 18:29
@max-arnold
Copy link
Contributor

Neat!

I will review the refactored implementation more thoroughly in a couple of days.

  • Am I correct in that unless/onlyif should work as expected (support module calls) with file state too?
  • Could/should this change be included in Neon?

@max-arnold
Copy link
Contributor

max-arnold commented Jan 25, 2020

@mchugh19
Copy link
Contributor Author

I left the file and macpackage custom implementations, as they looked somewhat state specific. If it was decided to aim for the full superset, those features could be ported too.

@codecov
Copy link

codecov bot commented Jan 27, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@6460874). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #55974   +/-   ##
=========================================
  Coverage          ?    38.9%           
=========================================
  Files             ?     1496           
  Lines             ?   262820           
  Branches          ?    55551           
=========================================
  Hits              ?   102232           
  Misses            ?   149031           
  Partials          ?    11557
Flag Coverage Δ
#amazon2 38.9% <0%> (?)
#py2 38.9% <0%> (?)
#runtests 38.9% <0%> (?)
#zeromq 38.9% <0%> (?)

@max-arnold
Copy link
Contributor

@mchugh19

I finally had some time to review/test the patch, and so far, it looks quite good! There are minor differences (for example, how exceptions in states.git.mod_run_check were handled), but I believe there is no sense to port them to the global implementation.

I still think it makes sense to add the creates keyword to the STATE_RUNTIME_KEYWORDS list and document it as a global requisite: https://github.com/saltstack/salt/blob/master/salt/state.py#L92-L127. Otherwise, it won't be filtered here https://github.com/saltstack/salt/blob/master/salt/utils/args.py#L467-L485 and will be passed down to any state that accepts **kwargs.

I left the file and macpackage custom implementations, as they looked somewhat state specific. If it was decided to aim for the full superset, those features could be ported too.

I believe that states.file already supports global unless/onlyif options (it has a mod_run_check_cmd implementation and no mod_run_check one)

As for macpackage, the code seems to be quite similar, so it might be possible to drop the module-specific implementation if you have the energy to do so.

And finally, it makes sense to mention this in the release notes, because there is a small risk that the change is not 100% compatible.

@mchugh19
Copy link
Contributor Author

mchugh19 commented Feb 9, 2020

Thanks for the review @max-arnold! I think I've taken care of the STATE_RUNTIME_KEYWORDS adding and some of the needed doc updates.

You're right on the file state. So that's an easy one off the list.

However, while the macpackage state's mod_run_check is simplistic, the fanciness is in that it accepts as user argument under which to run the only_if and unless requisites.

run_check_cmd_kwargs = {'runas': user, 'python_shell': True}

Since the global only_if and unless can do things other than just run shell commands, I'm not sure if it would make the most sense to attempt to port this to the global version, as it may introduce some confusion.

Thoughts, and did I catch all the needed docs?

@max-arnold
Copy link
Contributor

max-arnold commented Feb 9, 2020

Found an interesting cheatsheet that states the following:

This logic is specific to the cmd state and may not apply to others in the same way or at all since salt.states.cmd implements its own onlyif and unless requisites.

https://gist.github.com/jfindlay/331a4394281e778a1ae9217edb5728b7

Not sure if the statement is true, but maybe it worth to double check the cmd.run invariants by manually running the linked state twice (before and after the patch is applied).

Other than that, I think it is ready to merge if the tests pass.

@mchugh19
Copy link
Contributor Author

mchugh19 commented Feb 9, 2020

Nice! With the patch, those states still execute as described. (Seems like the expected behavior anyway)

Currently this still leaves macpackage as the only state with specialized unless/onlyif and missing out on other requisite capabilities.

@max-arnold
Copy link
Contributor

max-arnold commented Feb 9, 2020

Currently this still leaves macpackage as the only state with specialized unless/onlyif and missing out on other requisite capabilities.

If you really want to fix the macpackage state, it could be done in the opposite way - by calling these advanced unless/onlyif features from macpackage._mod_run_check.

Not sure if it is worth the trouble, though. And it is still possible that someone will introduce new state-specific mod_run_check functions in the future. Unless you remove this possibility altogether 😎

@mchugh19
Copy link
Contributor Author

mchugh19 commented Feb 9, 2020

Zowie! On further investigation, it appears macpackage doesn't use its local version at all.

Here we go:
The macpackage function name is _mod_run_check with a leading underscore.

def _mod_run_check(cmd_kwargs, onlyif, unless):

As such, the state specific version doesn't cause state.py to not run its own global versions, since it doesn't match the name mod_run_check https://github.com/saltstack/salt/pull/55974/files#diff-e5cf22153489a6c40864babf3ca18002R1974
and since this global requisite check occurs before the macpackage state is actually executed, it seems any onlyif or unless checks will have already occured. Should they meet whatever conditions are being tested, then the state will never be run and the local _mod_run_check will never execute.

The only way I foresee the local version ever functioning is in the very special circumstance where user is specified on the state, but the unless (onlyif by nature would be even more difficult to work this way) check command is done in such a way where it fails if run by the global implementation, but later succeeds when run by the user specified in the state.

This seems a bit far fetched, and as it looks like there aren't any tests for the macpackage state's _mod_run_check, I suspect it has simply never been called and the user functionality is just broken.

Can anyone from the MacOS working group confirm? If unused, we might as well remove the confusing code and documentation.

For this PR, I think that wraps up the outstanding bits.

@weswhet
Copy link
Contributor

weswhet commented Feb 9, 2020

I can take a look at the macpackage state tomorrow. cc @sheagcraig

@weswhet
Copy link
Contributor

weswhet commented Feb 14, 2020

@mchugh19 per The only way I foresee the local version ever functioning is in the very special circumstance where user is specified on the state,

macOS packages can only be installed as root and attempting to do so as a user will fail. So with that being said, I'm okay with removing the code and the documentation for the macpackage state.

@weswhet
Copy link
Contributor

weswhet commented Feb 18, 2020

Actually, after discussion with the macOS workgroup, the change to the macpackage state would break this formula. https://github.com/saltstack-formulas/salt-formula/blob/efc5d7c632834fa7e14f1c2410645a4a6594df6f/salt/minion.sls#L69-L70

@garethgreenaway recommends adding a deprecation notice.

@Ch3LL Ch3LL removed the request for review from a team April 15, 2020 14:27
@mchugh19
Copy link
Contributor Author

mchugh19 commented May 4, 2020

re-run pr-ubuntu2004-py3
re-run pr-fedora32-py3

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

Successfully merging this pull request may close these issues.

Requisites 'onlyif'/'unless' ignored in some cases Refactoring onlyif/unless - generalized with fallback?
7 participants