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

manage systemd unit files with camptocamp/systemd #90

Merged
merged 1 commit into from
Oct 24, 2017
Merged

manage systemd unit files with camptocamp/systemd #90

merged 1 commit into from
Oct 24, 2017

Conversation

bastelfreak
Copy link
Member

No description provided.

@bastelfreak bastelfreak added the enhancement New feature or request label Oct 24, 2017
@bastelfreak
Copy link
Member Author

This should be rebased after #86 got merged

@wyardley wyardley merged commit 6c04300 into voxpupuli:master Oct 24, 2017
@bastelfreak bastelfreak deleted the systemd branch October 24, 2017 17:43
@juliantaylor
Copy link

juliantaylor commented Mar 6, 2018

this effectively reverted #58
as there are many systemd modules out there this limits where we can use the prometheus module.
Please manage the systemd without using a module with such poor namespacing.

@bastelfreak
Copy link
Member Author

I am not sure how we should proceed here. One the one side I think it is a good idea to aim for 'only one module for one job' and the camp2camp-systemd module is the most used one, has a good feature set and is heavily tested. On the other side this is a bit anoying because we don't have proper namespacing on the puppetserver side, so we can't easily use other modules with the same name. My personal opinion is that we should stick with the camptocamp module. I would like to get feedback from the other contributors. CC @voxpupuli/collaborators

@hunner @eputnam maybe this should be discussed during the module triage (sadly I won't be able to attend :( )? How is this handled in official Puppet(labs) modules?

@bastelfreak bastelfreak added the needs-feedback Further information is requested label Mar 6, 2018
@jhoblitt
Copy link
Member

jhoblitt commented Mar 6, 2018

I'm not a contrib on this module but I agree that camptocamp/systemd seems to be the defacto standard at this point.

@juliantaylor
Copy link

juliantaylor commented Mar 6, 2018

I disagree on it being a defacto standard, the one being used by me is eyp/systemd which also has a significant user base and active development (though they are a bit lacking in actually pushing their releases to the forge)
The last forge release of camptocamp being in 2015 does not give a good impression.

@bastelfreak
Copy link
Member Author

https://forge.puppet.com/camptocamp/systemd last release is from november 2017

@juliantaylor
Copy link

oh sorry, google search sent me to a tagged version ...

still it only has 5 times more downloads which is imo not significant enough to call it a defacto standard.

@juliantaylor
Copy link

juliantaylor commented Mar 6, 2018

are it still only these two places where the module is used? after a quick grep it seems that way.
In that case I could probably live with creating a fork of the prometheus module, though I would prefer if you could live with managing these two service files without another dependency. I can help out with code if it is more than just what is in this PR.

@bastelfreak
Copy link
Member Author

The thing here is that we once discussed in the IRC to manage all units in all of our repos with the camptocamp module. For consistency I would prefer it if all services in our modules are managed the same way.

@wyardley
Copy link
Contributor

wyardley commented Mar 6, 2018

I'll admit that I'm still not wild about the camptocamp module, though seems like some improvements have been made recently. FWIW, though, when there was discussion about this, the Puppet party line was that it's not Puppet's job to handle it internally (beyond what's already supported in terms of the service resource) and IIRC, it was suggested to use the camptocamp module. Hunter did do some work in terms of seeing whether it was practical (and could perform well enough) to have Puppet detect changed unit files internally.

There's some discussion in, e.g., https://tickets.puppetlabs.com/browse/PUP-3483

I'm not using Puppet either for work or play right now, so haven't been doing any work on this stuff, so won't be in a position to improve the camptocamp module personally, but I do think it makes sense to focus on changing / improving that module vs. other solutions. That said, pulling in the module as a dependency when it's literally just doing one exec or managing one template does seem a little heavy-handed when it's not really adding a ton of value.

@alexjfisher
Copy link
Member

How about making use of https://github.com/puppetlabs/puppetlabs-stdlib#load_module_metadata ?

It should be possible to use camptocamp/systemd only if it's on the modulepath and otherwise just use an exec.

@alexjfisher
Copy link
Member

Completely untested, but my proposal to satisfy both camps is something a bit like #169

@juniorsysadmin
Copy link
Member

It should be possible to use camptocamp/systemd only if it's on the modulepath and otherwise just use an exec.

Maybe I haven't read the docs correctly but modulepath is not the same as metadata.json listing

@juniorsysadmin
Copy link
Member

There is sort of a precedent for something similar in the case of puppet-archive vs puppet-staging ($use_archive vs $use_staging). From where I have seen that used, no catch-all case was performed with tar execs if both were set to false.

Also, it is important to distinguish between just the unit service reload and management of the unit file itself. I would prefer being dependent on something else for the reload only (don't use ::systemd::unit_file)

Ideally, I would like something like:

# Assume camptocamp/systemd is available, don't use execs to account
# for no systemd reload module being used
$systemd_reload_class = 'systemd::systemctl::daemon_reload'
...
# File resource notifies $systemd_reload_class
... 

This might not be possible.

@alexjfisher
Copy link
Member

@juniorsysadmin load_module_metadata loads the metadata.json of the module you ask it to, not the metadata of the module you call it from. See https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/lib/puppet/parser/functions/load_module_metadata.rb#L12

@vinzent
Copy link
Contributor

vinzent commented Mar 7, 2018

Personally I don't like modules that manage systemd unit files. Providing a unit fiile and adding it to systemd should be done by a package. A module should only care about enabling+starting a service. modifying/overriding unit file defaults then should happen at the profile level.

I know that packaging is not fancy anymore but it really solves some problems.

@ekohl
Copy link
Member

ekohl commented Mar 7, 2018

@vinzent I'm with you that actual unit files should live in packaging. That said, there is a use case for overrides (drop ins). For example to manage limits.

@alexjfisher
Copy link
Member

and unfortunately, some stuff comes unpackaged. eg just a tarball :(

@alexjfisher
Copy link
Member

@juniorsysadmin I did find an issue with load_module_metadata. It expects some version of the module to be present. If you don't have any systemd module installed, it will still raise an error. Boo!

Not to worry. I wrote a new function we could use instead. :) voxpupuli/puppet-extlib#92

@juliantaylor
Copy link

juliantaylor commented Mar 7, 2018

Adding conditional use of the module is not a good idea.
You then have to now test that two code paths work instead of one and if both code paths are present, work and do the same thing you might as well delete one of them.

@alexjfisher
Copy link
Member

@juliantaylor The paths only have a couple of resources each. If this allows you to continue using the module, I'm a bit surprised you're against it.

@juliantaylor
Copy link

juliantaylor commented Mar 7, 2018

I am not against it and to save me time I should probably just be silent.
But is not a good idea from your perspective. You would be maintaining two code paths which do the same thing for no benefit.
For example, now someone discovers the systemd reload you still have in your code is not needed anymore because the module is supposed to handle that and removes it. Now one of the code paths is broken.
As both code paths are the same thing you are much better of just having one to maintain.

I would like that one to be the no-module one, but that is in the end up to you.

@jhoblitt
Copy link
Member

jhoblitt commented Mar 7, 2018

The thought of trying to support multiple implementations of a module with the same name makes me uneasy and feels like a bad precedent to be setting. I'd rather that voxpupuli as a whole trying to pick a preferred implementation in these situations and try to normalize its usage across the entire "portfolio".

@alexjfisher
Copy link
Member

I don’t think we’d want to go as far as supporting multiple systemd modules. We should pick one. But I don’t think we should mandate it either. I don’t think it’s fair on our users or on the authors of eyp/systemd especially. (I think there’d be some tipping point if we increased our usage of camptocamp/systemd after which other systemd modules couldn’t continue). Much like many modules have a ‘manage_java’ parameter, I think we want some flag for people who don’t want to or can’t use camptocamp/systemd.

The function I added to extlib would allow us to do if has_module(‘camptocamp/systemd’) { ... but perhaps that’s more magic than we need and a normal module parameter would suffice.

There probably aren’t too many other cases where I’d see us wanting to use has_module. Maybe for saz/sudo (especially if it becomes puppet/sudo)??

@hunner
Copy link
Member

hunner commented Mar 9, 2018

@igalic
Copy link

igalic commented Mar 11, 2018

remember what i did for puppet/archive and camptocamp/archive?
if that's an option that's not too painful, someone might consider going that way
if not, then, what @hunner said.

Rovanion pushed a commit to Rovanion/puppet-prometheus that referenced this pull request May 5, 2021
manage systemd unit files with camptocamp/systemd
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-feedback Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants