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

add rc service module and sysrc utils #26251

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@thnee

thnee commented Jun 29, 2017

SUMMARY

Adds a new module rc_service. This follows along the lines of separating modules from service.py into new service modules, as previously done with the systemd module.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME
  • modules/system/rc_service
  • module_utils/sysrc
ANSIBLE VERSION
ansible 2.4.0 (rc_service e00e6fa58d) last updated 2017/06/29 16:50:44 (GMT +200)
  config file = /path/to/project/playbooks/ansible.cfg
  configured module search path = [u'/path/to/project/library']
  ansible python module location = /path/to/ansible/lib/ansible
  executable location = /path/to/ansible/bin/ansible
  python version = 2.7.13 (default, Jun 26 2017, 13:38:23) [GCC 4.2.1 Compatible FreeBSD Clang 3.8.0 (tags/RELEASE_380/final 262564)]
ADDITIONAL INFORMATION
@thnee

This comment has been minimized.

thnee commented Jun 29, 2017

@bcoca Pinging you for an initial review as this is based on your previous work. This is an initial version, hoping it can be a starting point for a discussion. I called it rc_service for now as that is what you suggested. But the way I see it, this should be named specifically after FreeBSD as the other BSD's work in different ways. However, I see in module_utils/facts/system/service_mgr.py:114 that there is no differentiation at all between the different BSD's at this point. Shall we make it part of this pull request to fix that too?

@mattclay

This comment has been minimized.

Member

mattclay commented Jun 29, 2017

Closing and re-opening to trigger CI.

@mattclay mattclay closed this Jun 29, 2017

@mattclay mattclay reopened this Jun 29, 2017

@mattclay

This comment has been minimized.

Member

mattclay commented Jun 29, 2017

@thnee Can you include integration tests in your PR so we have test coverage for this?

@ansibot

This comment has been minimized.

Contributor

ansibot commented Jun 30, 2017

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/system/rc_service.py:0:0: E312 No RETURN provided

click here for bot help

@bcoca

This comment has been minimized.

Member

bcoca commented Jun 30, 2017

@mattclay actually, the 'service' tests on BSD will already cover this as the action plugin should select this module automatically

@bcoca

This comment has been minimized.

Member

bcoca commented Jun 30, 2017

@thnee if the name is the issue we have 2 options, port the rest of 'bsd classes' that the old service module uses or alter the facts make the init system BSD flavor specific. I've engaged BUGs in the past about this topic and it seems pretty split with a slight majority in favor of the latter.

def run_service(module, args):
bin_path = module.get_bin_path('service', required=True)

This comment has been minimized.

@bcoca

bcoca Jun 30, 2017

Member

not all versions of FreeBSD have the 'service' command, we should fallback to the rc script itself

This comment has been minimized.

@thnee

thnee Jul 1, 2017

Which versions are you thinking of specifically? The service command was added in 7.3, which is not supported any more. Falling back to calling the rc script directly would mean that we would have to re-implement and maintain duplicated logic for finding local rc scripts in various directories.

This comment has been minimized.

@bcoca

bcoca Jul 5, 2017

Member

I've had 8 and 9 installs w/o it ... but that was mostly cause previous admin avoided it as a 'linuxism', we do have the code for rc, but we can always add later of someone protests.

so ignore this point for now.

default: null
choices: ['yes', 'no']
description:
- Whether the service should be enabled using sysrc.

This comment has been minimized.

@bcoca

bcoca Jun 30, 2017

Member

some services take extra arguments, should we implement that field here?

This comment has been minimized.

@thnee

thnee Jul 1, 2017

I would like to suggest a different approach: We remove support for enabled from this service module, and do not attempt to set any sysrc variables at all. Instead we add another module called sysrc letting the user set any configuration they like, as sysrc is only used on FreeBSD. This will simplify the modules and separate the concerns. This is what I am doing locally now, as this makes the most sense for me. What do you think?

In contrast: on OpenBSD, there is the rcctl utility, which does support enable and disable actions, if someone makes a module for that, it would make more sense to support enabled argument in that module.

This comment has been minimized.

@bcoca

bcoca Jul 5, 2017

Member

this would make it incompatible with the service interface

@mattclay

This comment has been minimized.

Member

mattclay commented Jul 4, 2017

@bcoca Unfortunately we're not running the service tests on Shippable for FreeBSD. They weren't passing last time I checked, so they've been marked skip/freebsd.

@bcoca

This comment has been minimized.

Member

bcoca commented Jul 5, 2017

@mattclay well, we need those to work if we want BSD test coverage.

@mattclay

This comment has been minimized.

Member

mattclay commented Jul 18, 2017

The service integration test only has support for sysv, systemd and upstart currently.

@thnee Could you update the test to support bsd-init as part of your PR so we can test in CI? Here's where the init system specific setup occurs:

https://github.com/ansible/ansible/blob/devel/test/integration/targets/service/tasks/main.yml#L13

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