Add systemd support (in 3.0.0-rewrite)

[miguelaferreira]

This PR adds a nginx service systemd provider.

I've tested the provider on a Centos 7 box. In order to make it work I've had to converge the box with yum-epelon it, change the site root dir in the example recipe (tmp wouldn't work) and always set the run_user attribute of nginx_service to nginx.

I've ran all the kitchen tests for the Centos 7 platform. All but the service_single_upstream test case passed. The failing text can be fixed by checking the platform and on Centos 7 include the yum-epel recipe instead of adding the apt repository.

[miketheman]

@miguelaferreira Thanks! This is great stuff. I've got some notes:

I think that in general, I wanted to keep the example.rb recipe as simple as possible, and have the provider make the "default" decisions unless overridden by a variable in the resource, as well as when running the integration tests. There should be very little decision-making in the code at

The reason I used /tmp as the site root so as to not have to create more directories and convey that this is absolutely not-prod-use case.

There are some syntax failures that prevent the rest of the tests from being run (I'm not certain why the circleci test kitchen suite didn't run either).

I had added a reminder in the resource - I think this needs to be handled semi-automatically when on a given platform, but also needs to be flexible enough to allow a user to override it.

This should have some accompanying examples in the spec testing, that can make iteration slightly speedier, see https://github.com/miketheman/nginx/blob/3.0.0-rewrite/spec/shared_examples/service.rb and for how I did sysvinit and upstart examples, and then use them in a platform-specific spec test: https://github.com/miketheman/nginx/blob/3.0.0-rewrite/spec/libraries_specs/resource/nginx_service/create/ubuntu_1404_spec.rb
(These aren't comprehensive coverage yet, but it's a start.)

Let me know if this makes sense to you, thanks again for working on it!

[miguelaferreira]

@miketheman glad I can help. Let me provide some context with respect to your comments.

For some reason, using the opcode-centos-7.1 box, I can't serve a website out of /tmp. I've tried to make the nginxuser own the entire thing, opening up all the file permissions, and still couldn't serve the page. Then I tried enabling autoindex on /tmp and as a result I could list files by hitting http://localhost but the list contained only the .. entry. Then I tried serving it from another place, and it worked. So, I understand and agree with you, but I've reached the limits of my knowledge in respect to another way of making that test pass. I'm willing to rever the code to the original example recipe, but that will mean that the kitchen test will fail. Let me know and I'll act accordingly.

I'll fix the style issues as well (I think that's what you mean by syntax failures, right?).

I agree with determining the user automatically depending on the platform. For that I can create a helper method and then use it in the resource definition. Does it sound good to you?

I'll update the spec test as well. I'll start with the detection of the user name based on the platform, and then copy whatever you did for sysvinit and upstart.

I'll update the PR soonish (most likely this week).

[miketheman]

For some reason, using the opcode-centos-7.1 box, I can't serve a website
out of /tmp. I've tried to make the nginxuser own the entire thing, opening
up all the file permissions, and still couldn't serve the page. Then I
tried enabling autoindex on /tmp and as a result I could list files by
hitting http://localhost but the list contained only the .. entry. Then I
tried serving it from another place, and it worked. So, I understand and
agree with you, but I've reached the limits of my knowledge in respect to
another way of making that test pass. I'm willing to rever the code to the
original example recipe, but that will mean that the kitchen test will
fail. Let me know and I'll act accordingly.

Not certain why /tmp would behave differently just yet - I'll try to take a
pass at it sometime this week. Maybe leave this to the very last part, and
hopefully we'll figure it out together. :)

I'll fix the style issues as well (I think that's what you mean by syntax
failures, right?).

Yes. Once those pass, the spec tests should be executed. You can run these
locally with rake and it'll run all desired tests in order.

I agree with determining the user automatically depending on the
platform. For that I can create a helper method and then use it in the
resource definition. Does it sound good to you?

I think it does - there's already a helper module to add methods in
helpers.rb - so that's probably the right place to put the logic.

I'll update the spec test as well. I'll start with the detection of the
user name based on the platform, and then copy whatever you did for
sysvinit and upstart.

Sounds good!

-M
On Mon, Oct 12, 2015 at 6:58 AM, Miguel Ferreira notifications@github.com
wrote:

@miketheman https://github.com/miketheman glad I can help. Let me
provide some context with respect to your comments.

For some reason, using the opcode-centos-7.1 box, I can't serve a website
out of /tmp. I've tried to make the nginxuser own the entire thing,
opening up all the file permissions, and still couldn't serve the page.
Then I tried enabling autoindex on /tmp and as a result I could list
files by hitting http://localhost but the list contained only the ..
entry. Then I tried serving it from another place, and it worked. So, I
understand and agree with you, but I've reached the limits of my knowledge
in respect to another way of making that test pass. I'm willing to rever
the code to the original example recipe, but that will mean that the
kitchen test will fail. Let me know and I'll act accordingly.

I'll fix the style issues as well (I think that's what you mean by syntax
failures, right?).

I agree with determining the user automatically depending on the platform.
For that I can create a helper method and then use it in the resource
definition. Does it sound good to you?

I'll update the spec test as well. I'll start with the detection of the
user name based on the platform, and then copy whatever you did for
sysvinit and upstart.

I'll update the PR soonish (most likely this week).

—
Reply to this email directly or view it on GitHub
#384 (comment).

[nogweii]

For some reason, using the opcode-centos-7.1 box, I can't serve a website out of /tmp

SELinux is probably the cause of this. nginx will run in a limited http context, which I think blocks access to /tmp.

[miguelaferreira]

@evaryont It doesn't seem to be SELinux. It's in permissive mode, and I don't find any denied action in the audit log.

[miguelaferreira]

@miketheman I've just pushed a new commit to this PR.

I've reverted the changes in the example recipe to keep is simple, as you request. The consequence is that I can't run the respective kitchen test on centos 7.

I've added a helper method to determine the user based on the platform. That method is used in the resource definition, and tested in the helpers_sepc.rb. However, the way I managed to do it produces a RuboCop violation. If follow RuboCop's recommendation, then several rspec tests start failing.

I've added ChefSpec tests for the centos platform, just like what you had already for debian. The tests for debian were already failing, and so are the ones for centos.

Without my changes rake reports this:

Finished in 3 minutes 49.7 seconds (files took 2.7 seconds to load)
39 examples, 23 failures, 1 pending

With my changes the numbers are higher, but the asserts that fail for the tests I added are the same as the existing ones:

Finished in 3 minutes 52.3 seconds (files took 2.63 seconds to load)
57 examples, 34 failures, 2 pending

[miguelaferreira]

@miketheman any progress on this?

[genslein]

can you fix the rubocop with the following?:

lazy { user_for_platform }

[miguelaferreira]

Sure

[chewi]

Just a thought, wouldn't it be nicer to use systemd's own instance support? You would install the actual .service file once and create symlinks like nginx@foo.service or whatever.

[Unit]
Description=The nginx HTTP and reverse proxy server for %i

[Service]
PIDFile=/run/%i.pid
ExecStart=/usr/sbin/nginx -c /etc/%i/nginx.conf

Please also provide a way to disable this entirely. systemd, nginx, and keys with passphrases do not play well together AT ALL. Trust me, I've tried really hard. With v2, I've had to use chef-rewind to force the service to :nothing and install my own script to start nginx manually.

[miketheman]

Reviewed 16 of 17 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

.kitchen.yml, line 16 [r2] (raw file):
See notes in the nginx_repo changeset for what I think should be done here.

---

.rubocop.yml, line 3 [r2] (raw file):
What don't the cops like in the helpers that this needs to be disabled?

---

metadata.rb, line 10 [r2] (raw file):
I've removed the other suggests recommendations in accordance with FC052.
We should probably note the tested versions via Berksfile/Readme since we are not restricting/requiring the user to any specific version.

---

libraries/helpers.rb, line 21 [r2] (raw file):
How ubiquitous is this? Does it change based on the package installed?

---

libraries/helpers.rb, line 31 [r2] (raw file):
Can you explain how/why this better than having the resource opt-in via include?
I haven't seen much of this type of pattern before.

---

libraries/provider_nginx_service_systemd.rb, line 15 [r2] (raw file):
Is this always the right path? For some reason, I thought it was in /lib/systemd/system - are these related?

---

spec/libraries_specs/resource/nginx_service/create/centos_7_spec.rb, line 18 [r2] (raw file):
This should probably be #systemd.

---

templates/default/systemd/nginx.erb, line 9 [r2] (raw file):
The default we have in the base config is /var/run/... - I'm thinking that one needs to be changed to be /run/..., since I've seen many an issue with nginx, pidfile handling, and would like to get some degree of sanity done here. Thanks for surfacing this!

---

templates/default/systemd/nginx.erb, line 10 [r2] (raw file):
Does the test flag need to have the correct config file as well?

---

test/fixtures/cookbooks/nginx_service_test/recipes/nginx_repo.rb, line 6 [r2] (raw file):
As a general approach, this resource was already guarded with an only_if in the resource a few lines later.

I find that these guards are great for a single platform-specific resource, and the case...when is better in the event of multiple platform-specific resources.

---

test/fixtures/cookbooks/nginx_service_test/recipes/nginx_repo.rb, line 16 [r2] (raw file):
The intent of this recipe is to set an upstream nginx repo (i.e. the nginx.org one above).
I think that if you're looking for the "default" nginx package from the yum::epel repo, that should be part of either the suite's direct run list, or another recipe.

It would be good to change this include_recipe to an actual yum_repository statement pointing at the nginx.org yum repo.

---

Comments from the review on Reviewable.io

[miketheman]

@miguelaferreira I tried using reviewable.io for this changest - hope it makes sense. It was pretty cool and reduced the "17 emails, one per change" problem.
In general, I think this needs a rebase & conflict resolution, address the outstanding questions/issues and a tests (which CI should now provide decent feedback for).
Thanks!

[chewi]

Is this always the right path? For some reason, I thought it was in /lib/systemd/system - are these related?

You can drop .service files under /etc but you won't be able to disable them on boot without deleting them entirely. Generally speaking, you should put them under /lib/systemd/system and create symlinks under /etc. systemctl enable creates symlinks in the same way. Also be aware that you should use /lib/systemd and not /usr/lib/systemd because the latter doesn't exist on Debian.

[miguelaferreira]

@chewi When I implemented this I didn't know systemd supported something like parametric service definitions. I agree that it would indeed be a nice implementation. However, I don't really think that's an issue with the PR. I mean, it could be a better implementation but the current implementation isn't bad. Could this be something to refactor afterwards?

---

Review status: 12 of 17 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

.rubocop.yml, line 3 [r2] (raw file):
Don't remember. Do you want me to remove this?

---

metadata.rb, line 10 [r2] (raw file):
I've replaced the suggests by depends, but if you prefer I can just remove them. Let me know

---

libraries/helpers.rb, line 21 [r2] (raw file):
As far as I know, that's where the user name on the centos platform. I don't know if it changes or not depending on the package.

---

libraries/helpers.rb, line 31 [r2] (raw file):
It's a matter of taste, I guess. I've picked that up from the chef-sugar cookbook and have bene using it ever since. I like the way it extends the resource/provider/recipe DSL.

---

libraries/provider_nginx_service_systemd.rb, line 15 [r2] (raw file):
As far as I know, the path is that one. The packages, when installed, place an example path under /lib or /usr and then those should either be copied or linked to in /etc/systemd/...

---

spec/libraries_specs/resource/nginx_service/create/centos_7_spec.rb, line 18 [r2] (raw file):
Did you test that? Or are you suggesting?
I wrote this code a while ago, and I guess I did have reason to do it like that. However, if you do test it and you tell me it works otherwise, I'll be happy to change it.

---

templates/default/systemd/nginx.erb, line 10 [r2] (raw file):
Don't know

---

test/fixtures/cookbooks/nginx_service_test/recipes/nginx_repo.rb, line 6 [r2] (raw file):
Ok. Anything you want done here? Or was this just a note?

---

test/fixtures/cookbooks/nginx_service_test/recipes/nginx_repo.rb, line 16 [r2] (raw file):
Sure.

---

Comments from the review on Reviewable.io

[miguelaferreira]

spec/libraries_specs/resource/nginx_service/create/centos_7_spec.rb, line 18 [r2] (raw file):
I gess you are right on that one.

---

Comments from the review on Reviewable.io

[miketheman]

Reviewed 3 of 17 files at r1, 1 of 3 files at r3, 3 of 3 files at r4.
Review status: 15 of 17 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

.rubocop.yml, line 3 [r2] (raw file):
Please. If removing locally and running the tests pass, then we know it's fine.

---

metadata.rb, line 10 [r2] (raw file):
I think removal is safest for maximum compat - we leave it up to the user of the cookbook to implement whichever package strategy they prefer.

---

libraries/provider_nginx_service_systemd.rb, line 15 [r2] (raw file):
Referencing the RH manual:

/etc/systemd/system/ Systemd units created and managed by the system administrator. This directory takes precedence over the directory with runtime units.

---

libraries/provider_nginx_service_systemd.rb, line 23 [r4] (raw file):
Noting this for myself later on: chef/chef#2839

---

templates/default/systemd/nginx.erb, line 10 [r2] (raw file):
I think if this executes against a malformed nginx-, it should fail/stop the reload/execution, so it should point to the custom config.

---

test/fixtures/cookbooks/nginx_service_test/recipes/nginx_repo.rb, line 6 [r2] (raw file):
It was more of a "we should not double-guard a resource" - so if the resources are going to be in a case statement, remove the only_if.

---

test/fixtures/cookbooks/nginx_service_test/recipes/nginx_repo.rb, line 16 [r2] (raw file):
I think this change missed the mark - instead of pointing at epel, point at the nginx.org package repo: http://nginx.org/en/linux_packages.html#stable

---

Comments from the review on Reviewable.io

[chewi]

@miguelaferreira I don't think it'd be a massive change but it's cool. I wasn't sure how much of a hurry you were in to get this merged.

[miguelaferreira]

@chewi this PR has bene open for too long, and I'm not keen in maintaining long lived branches. A change like what you propose would require me to start testing everything from top to bottom, all over again. I simply don't have the time/focus to do that anymore.

[coveralls]

Coverage decreased (-3.9%) to 94.969% when pulling d584ece on miguelaferreira:3.0.0-rewrite/add-systemd-support into a921767 on miketheman:3.0.0-rewrite.

[miguelaferreira]

@miketheman At this point, I'm happy with my contribution. All tests RSpec and ServerSpec are passing.
I will now push a few more commits to fulfil what you've asked in the review. There is one change that might break things (adding the test command to the config file in the systemd unit), feel free to address that if it happens.

[miguelaferreira]

Review status: 10 of 21 files reviewed at latest revision, 9 unresolved discussions.

---

.rubocop.yml, line 3 [r2] (raw file):
Done

---

metadata.rb, line 10 [r2] (raw file):
Done

---

templates/default/systemd/nginx.erb, line 10 [r2] (raw file):
Sure, I'll add the flag and I'll leave the testing (and any potential fixing to you).

---

test/fixtures/cookbooks/nginx_service_test/recipes/nginx_repo.rb, line 6 [r2] (raw file):
Actually, the case statement and the guard are not checking the exact same thing.
Furthermore, I've actually tested this, and removing this guard will break the RSpec test run. No resources will be found without it. So I'm keeping it in.

---

test/fixtures/cookbooks/nginx_service_test/recipes/nginx_repo.rb, line 16 [r2] (raw file):
Done

---

Comments from the review on Reviewable.io

[miketheman]

@miguelaferreira Thanks, and sorry for the delay - I've been attempting to figure out how to get systemd properly working in the docker-based CI, and spent far too much time going down rabbit holes with no resolution.
Apparently running systemd inside a docker container that runs on an LXC-based container CI service is not as simple as one might hope.
For now, I guess systemd-based tests will have to be excluded from that CI, and done manually when needed, until that can be figured out.

I have a few more commits to add to this branch before merging, and then will be very happy to add the hard work you've put in here to the codebase.

---

Reviewed 1 of 17 files at r1, 10 of 11 files at r5, 1 of 1 files at r6, 3 of 3 files at r7.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.