This repository has been archived by the owner. It is now read-only.

first draft systemd service plugin #3660

Merged
merged 2 commits into from May 25, 2016

Conversation

Projects
None yet
@bcoca
Copy link
Member

bcoca commented May 16, 2016

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

systemd

ANSIBLE VERSION
2.2
SUMMARY

New systemd module to manage systemd services.

@bcoca bcoca added the new_plugin label May 16, 2016

@bcoca bcoca added this to the 2.2.0 milestone May 16, 2016

@jimi-c jimi-c added the in progress label May 16, 2016

@mscherer

This comment has been minimized.

Copy link
Contributor

mscherer commented May 16, 2016

Should masked be a state rather that a specific verb ?

@bcoca bcoca removed the needs_revision label May 16, 2016

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented May 16, 2016

no, as you can also want to stop it at the same time:

systemd: name=httpd3 masked=yes state=stopped

@bcoca bcoca force-pushed the bcoca:systemd branch 3 times, most recently from c255183 to c11fe69 May 16, 2016

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented May 16, 2016

Thanks @bcoca for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

@nemesisdesign

This comment has been minimized.

Copy link

nemesisdesign commented May 16, 2016

Hi, is there a way to try this new module?

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented May 16, 2016

like all modules, download into a 'library/' dir adjacent to your play or inside a role, be aware that this will only work with Ansible >=2.1. Also, based on feedback, I'll possibly be changing it soon to use shared code which will bump that up to 'current devel'.


# check systemctl result or if it is a init script
initscript = '/etc/init.d/' + unit
if rc == 0 or (os.access(initscript, os.X_OK) and bool(glob.glob('/etc/rc?.d/S??' + unit))):

This comment has been minimized.

@evverx

evverx May 16, 2016

Contributor

This doesn't work properly.
See #1909 (comment)

This comment has been minimized.

@bcoca

bcoca May 16, 2016

Member

it 'works as expected' as it will fail to enable masked services, this module has a specific option do deal with masking.

This is now an explicit option, so the fix for the service module does not really carry over.

This comment has been minimized.

@evverx

evverx May 16, 2016

Contributor

it will fail to enable masked services

$ systemctl is-enabled ssh
masked

$ ls -l /etc/init.d/ssh
-rwxr-xr-x 1 root root 4077 Mar 21 12:06 /etc/init.d/ssh

$ find /etc/rc[0-9]* -name '*ssh*'
/etc/rc2.d/S02ssh
/etc/rc3.d/S02ssh
/etc/rc4.d/S02ssh
/etc/rc5.d/S02ssh

So, if rc == 0 or (os.access(initscript, os.X_OK) and bool(glob.glob('/etc/rc?.d/S??' + unit))): returns True. No?

@nemesisdesign

This comment has been minimized.

Copy link

nemesisdesign commented May 16, 2016

Does this have any relation with the service module?

Do I have to alter my playbooks/roles in order to try this? Or is the daemon-reload case handled automatically?

@evverx

This comment has been minimized.

Copy link
Contributor

evverx commented May 16, 2016

is the daemon-reload case handled automatically?

    daemon_reload:
        required: false
        default: no
        choices: [ "yes", "no" ]
        description:
            - run daemon-reload before doing any other operations, to make sure systemd has read any changes.
        aliases: ['daemon-reload']

I think default: no is the good choice. See https://youtu.be/wVk-NWtiIZY?t=6m23s

@abadger

This comment has been minimized.

Copy link
Member

abadger commented May 16, 2016

We should be doing as little as possible at the toplevel of a module. The reason is that when we start to unittest modules, the toplevel will be nearly impossible to test. We can test main(), although not well (it's almost equivalent to treating the module as a blackbox. But we're able to import the code and monkey patch/mock it if needed). Even better is to push each unit of functionality into its own function.

Doing that lets us test, for instance, enabling and disabling a service separate from parsing the parameters for the module.

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented May 17, 2016

Thanks @bcoca for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

@@ -0,0 +1,329 @@
#!/usr/bin/python

This comment has been minimized.

@alikins

alikins May 17, 2016

Contributor

^^^
I'd rather not use the 'systemd' name space yet. (Maybe... 'systemctl' or 'systemctl_unit' or 'systemd_service' or 'systemd_unit')

This comment has been minimized.

@bcoca

bcoca May 17, 2016

Member

it needs to match the service_mgr fact, which already is systemd, we might still have systemd/systemctl_properties and other modules, but the main perception of systemd is still as a service manager.

default: null
description:
- Whether the unit should be masked or not, a masked unit is impossible to start.
daemon_reload:

This comment has been minimized.

@alikins

alikins May 17, 2016

Contributor

Could daemon_reload be a separate module? Seems like it could potentially be needed by itself as a task (and/or, for use in a notify handler)

This comment has been minimized.

@bcoca

bcoca May 17, 2016

Member

I don't think it merits a module as it would only ever do 'the one thing', it is here for the case in which the units have been modified (template/copy/etc tasks) and you want to execute the service action on the updated (reloaded) config and it seems to be a logical fit.

# Example action to reload service httpd, in all cases
- systemd: name=httpd state=reloaded
# Example action to enable service httpd
- systemd: name=httpd enabled=yes

This comment has been minimized.

@alikins

alikins May 17, 2016

Contributor

Might as well make the examples parallel the systemctl command, ie 'systemd: state=started name=httpd'.
(The systemctl cli ordering is still weird to me, but probably worth matching it for the examples).

This comment has been minimized.

@bcoca

bcoca May 17, 2016

Member

I really have no opinion on this, I should also probably add pure YAML invocations as others really don't like the k=v syntax ...

description:
- Controls systemd services on remote hosts.
options:
name:

This comment has been minimized.

@alikins

alikins May 17, 2016

Contributor

add alias for 'unit' ?

This comment has been minimized.

@bcoca

bcoca May 17, 2016

Member

yes, will add an alias as 'unit', name= is what we use generically in the service module.

}

#TODO: check if service exists
(rc, out, err) = module.run_command("%s show '%s'" % (systemctl, unit))

This comment has been minimized.

@alikins

alikins May 17, 2016

Contributor

If we are running the cli tools, it may be useful to add a systemd aware 'module.run_command' analog.

  • could be useful to be able to grab the journal entries related to running of the command/unit/pid to potentially include in the return code or debugging info
  • single place to specify host/machine/user for --user/--host/--machine options

This comment has been minimized.

@bcoca

bcoca May 17, 2016

Member

I'll leave that to future refactoring, I don't really see a use for other than --user.

@bcoca bcoca added core_review and removed needs_revision labels May 17, 2016

@bcoca bcoca force-pushed the bcoca:systemd branch from 4afcdb0 to 1e705a4 May 17, 2016

@gregdek gregdek added needs_revision and removed core_review labels May 17, 2016

@grahamrhay

This comment has been minimized.

Copy link
Contributor

grahamrhay commented May 20, 2016

@bcoca @evverx while I don't want daemon-reload to run more often than necessary, I would also prefer not to have to explicitly ask for it either. systemd does tell you when it's necessary:

Warning: Unit file changed on disk, 'systemctl daemon-reload' recommended.

@bcoca bcoca removed the needs_revision label May 20, 2016

@bcoca

This comment has been minimized.

Copy link
Member

bcoca commented May 20, 2016

@grahamrhay yes, and I even had code to do that, but it is still far from optimal. There are a few scenarios in which you want to avoid this, specially if you are changing multiple unit files and don't want to incur in the performance penalty of constantly reloading.

Other CM tools had this by default and have been backtracking as not only is there a performance cost, the system is unresponsive to events while reloading. Check the video above, even if they fix many of the issues at the systemd level, we will keep this default as not all distros will be able upgrade to a version of systemd that mitigates these issues.

@grahamrhay

This comment has been minimized.

Copy link
Contributor

grahamrhay commented May 20, 2016

hmm. you may be right, but without that you're not really giving me anything that I can't do already with a handler (i.e. the current workaround). I still have to keep track of which unit files I have changed myself.

@bcoca bcoca force-pushed the bcoca:systemd branch from 1e705a4 to 68a8881 May 20, 2016

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented May 20, 2016

Thanks @bcoca for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

@bcoca bcoca merged commit fb77ab4 into ansible:devel May 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jimi-c jimi-c removed the in progress label May 25, 2016

@xrow

This comment has been minimized.

Copy link

xrow commented Jun 20, 2016

Hi,

can someone test if this test case inside docker will work for 2.2.0 Milestone? We had issues with earlier versions.

https://gitlab.com/dennisxrow/ansible-service-testcase

@runcom

This comment has been minimized.

Copy link
Contributor

runcom commented Jul 18, 2016

@bocca there's no way to get the systemd module to work with "systemd --user" services,unit,timers etc etc

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