-
-
Notifications
You must be signed in to change notification settings - Fork 499
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
Fix Debian 8 support: configure for systemd #683
Conversation
I'm not sure what's going on with those Arch Linux test failures...that isn't a part of the code that I changed. |
manifests/config.pp
Outdated
@@ -193,28 +193,22 @@ | |||
} | |||
|
|||
case $facts['os']['family'] { | |||
'Archlinux': { | |||
$has_systemd = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we perhaps make use of the systemd fact in camptopcamp/systemd module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about that fact.. if it works accurately it would be cool to use. I can add the module as a dependency? It would also be useful for the systemctl daemon reload stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you can add it as a dependency to the metadata.json and use the fact
FWIW, I'm seeing those Arch test failures on current master as well. Paging Archlinux evangelist @bastelfreak to the white courtesy phone. |
yes this is related to the new facts |
@JayH5 I will have a look at the spec failures related to archlinux and fix them. I maybe already need to add the dependency to the systemd module. You can hold on with further development here until I fixed the current issues. |
Did a quick rebase but there are errors now... I'm a little swamped with other things at the moment and might only get back to this next week. |
I've tried to make use of the systemd module. I hit a few bumps and it has taken me a while to get this working. I've run out of time to spend on this for now and the tests still aren't passing 😢. I've done the bulk of the work here and would be grateful if somebody could take over. Otherwise, I will only be able to revisit again sometime in the future (1 week to 1 month's time). |
Currently, LimitNOFILE can still only be integer: I'll see if I can submit a PR to fix this; until then, I don't think we can use this module to manage the systemd service limits. Even then, it would not be ideal, because folks would have to use the newest version.
I personally suggest making a smaller incremental fix to keep this working on Debian 8 with the existing tooling, and then switch to systemd module if / when it becomes more practical. |
voxpupuli/puppet-systemd#76 |
manifests/config.pp
Outdated
default: { } | ||
} | ||
|
||
if $::facts['systemd'] { # systemd fact provided by systemd module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use topscope anymore. Please use $facts['systemd']
@@ -1425,6 +1413,13 @@ | |||
} | |||
end | |||
|
|||
context 'on systems with systemd', if: facts[:systemd] do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, I always forget that we can set conditions within a context block
manifests/service.pp
Outdated
@@ -32,6 +32,10 @@ | |||
hasrestart => true, | |||
name => $service_name, | |||
} | |||
|
|||
if $::facts['systemd'] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use $facts['systemd']
Rebased and updated as suggested. I'm still stumped by the Arch Linux test failures...if anybody has any ideas... Once again, don't have a heck of a lot of time to work on this so may be slow to update and happy for others to take over. |
@JayH5 the errors are not archlinux specific:
|
@bastelfreak thanks.. I must've been misreading 😓 |
@JayH5 I don't think this can work with the current systemd module. |
@wyardley I am happy to fix this in the systemd module. I would like to merge voxpupuli/puppet-systemd#81 first and afterwards add support for |
I am going to close this in favour of #715 |
Rebase of #683 / mock systemd fact properly
} | ||
|
||
if $facts['systemd'] { # systemd fact provided by systemd module | ||
systemd::service_limits { 'rabbitmq.service': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this use $::rabbitmq::service_name?
On Debian Stretch, the Service is called "rabbitmq-server.service" - but with "rabbitmq.service", the override does not get applied.
Debian 8 is listed as a supported platform in
metadata.json
, but the logic to detect systemd configuration seems to expect only Ubuntu within the Debian family. This checks for vanilla Debian vs Ubuntu and applies systemd configuration as necessary.