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

service.disabled on Windows #48117

Merged
merged 1 commit into from Jun 26, 2018

Conversation

Projects
None yet
6 participants
@twangboy
Contributor

twangboy commented Jun 13, 2018

What does this PR do?

Disable services that are set to manual on Windows using service.disabled and service.dead when enable: False

What issues does this PR fix or reference?

#48026

Previous Behavior

service.dead and service.disabled would not disable a service if the start_type was set to Manual.

New Behavior

Now manual start types are properly disabled.

Tests written?

Yes

Commits signed with GPG?

Yes

@rallytime rallytime requested a review from saltstack/team-windows Jun 14, 2018

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Jun 14, 2018

Please add some comments to the if is_windows which currently do not have comments.

Not this PR fault, it would be nice if private functions where documents as well as public functions, it would help with any review.

@damon-atkins

Please add functions to the execution module instead.

if salt.utils.is_windows():
# service.disabled in Windows returns True for services that are set to
# Manual start, so we need to check specifically for Disabled
before_toggle_disable_status = __salt__['service.info'](name)['StartType'] in ['Disabled']

This comment has been minimized.

@damon-atkins

damon-atkins Jun 14, 2018

Member

Can we create a function called _salt__['service.disabled'](name) rather than call __salt__['service.info'](name)['StartType'] in ['Disabled']. Keep the interface consistent across platforms.

This comment has been minimized.

@cachedout

cachedout Jun 14, 2018

Contributor

Such a function already exists, doesn't it?

This comment has been minimized.

@twangboy

twangboy Jun 18, 2018

Contributor

There is already a service.disabled, however as noted in the code comments, services that are set to start manually are considered disabled.

if salt.utils.is_windows():
# service.enabled in Windows returns True for services that are set
# to Auto start, but services set to Manual can also be disabled
before_toggle_enable_status = __salt__['service.info'](name)['StartType'] in ['Auto', 'Manual']

This comment has been minimized.

@damon-atkins

damon-atkins Jun 14, 2018

Member

Can we create a function called _salt__['service.enabled'](name) rather than call __salt__['service.info'](name)['StartType'] in 'Auto', Manual'. Keep the interface consistent across platforms.

This comment has been minimized.

@twangboy

twangboy Jun 18, 2018

Contributor

Same issue as noted above.

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Jun 19, 2018

I think it comes down to definitions:
enable/disable Unix/POSIX is on boot starting, windows equivalent of disable is Manual
start(running)/stop(dead) is independent of enable/disable i.e. which Manual is also.

I am not convinced of this change
__salt__['service.info'](name)['StartType'] in ['Disabled']
As its currently correct in terms of the definition from Unix/POSIX system.

I think the real need is to rewrite service in more open terms and not "specific platform terms" e.g. "service.dead" is bad name.

We have:
service.dead # Ensure that the named service is dead by stopping the service if it is running
service.disable # Disable Start on Boot
service.enabled # Enable Start on Boot
service.masked # fully disable a unit, making it impossible to start it even manually. (Windows Disable)
service.unmasked undo above

I think service state should be changed to the following i.e. create the following, and place warning on the old methods. (or have both)

nginx:
 service.status
   - start_mode: '<automatic|manual|etc>'   Can add more modes for different service managers.
   - start_mode_<platform|service manager>: <overrides generic start_mode>
   - start_mode_systemd:
   - start_mode_win_service:
   - state: '<running|stopped>' 
   - check_delay: 30
   - pid_file

automatic* and enable can mean the same thing.

My thoughts are leave it like it is, close this PR and open one to introduce new state called service.status

PS Second thought.

nginx:
  service.configured
    - win_service:
      - username:
      - password:
      - command:
      - description:
      - etc
    - init.d:
    - systemd:
    - etc

The something like the above. Where the platform implementation can decide what to use e.g. systemd platform would use the systemd settings above using init.d settings. If systemd is replaced, then it will also handle the next bright idea which comes along.

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 19, 2018

I prefer the current semantics, where the states are adjectives. Much more salty.

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Jun 19, 2018

A word normally used as a noun can function as an adjective, depending on its placement. :-)
I have changed configure to configured, in the example above.

@lorengordon

This comment has been minimized.

Contributor

lorengordon commented Jun 19, 2018

Right, but configure would have been a verb, much more suited to the execution module side of things. ;)

I'd probably recommend "service.managed" to follow other salt states/conventions...

@dwoz

This comment has been minimized.

Contributor

dwoz commented Jun 26, 2018

This PR addresses an open issue #48026 and fixes some broken integration tests.

https://jenkinsci.saltstack.com/job/2017.7/view/Python3/job/salt-windows-2016-py3/70/

Lets merge it and discuss changes to the service semantics in a separate issue (not a PR).

@dwoz dwoz self-requested a review Jun 26, 2018

@dwoz

dwoz approved these changes Jun 26, 2018

@dwoz

This comment has been minimized.

Contributor

dwoz commented Jun 26, 2018

re-run py

@rallytime rallytime merged commit 9aa4687 into saltstack:2017.7 Jun 26, 2018

6 of 10 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #6055 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #20108 — FAILURE
Details
default Build started sha1 is merged.
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #26259 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17988 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23983 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #11025 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22623 — SUCCESS
Details

@twangboy twangboy deleted the twangboy:fix_48026 branch Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment