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

Added systemd based service-runner in python #1025

Merged
merged 4 commits into from Sep 22, 2018

Conversation

Projects
None yet
6 participants
@greeebs
Copy link
Contributor

greeebs commented Sep 21, 2018

Added a new python based service-runner and .service file to enable it to run as a service under systemd.
This should replace the service-runner bash script included in other repos that require service-runner.

Previously these changes were in a PR to https://github.com/openenergymonitor/emonpi but part of that discussion suggested moving this function to emoncms. For the previous discussions, see openenergymonitor/emonpi#67 and openenergymonitor/emonpi#66

For this PR, I have created a new services directory under scripts with a service-runner directory underneath containing the actual .service and .py files.
I left the documentation file for it in the scripts/services/ directory, but can move it down one level if people think that makes more sense.

I have not yet tested it on Debian jessie, but strongly suggest a merge of this waits until that testing has been completed.

I'm also not sure that running a service with a script file that lives under /var/www is entirely appropriate, but can probably be convinced otherwise.

If this gets merged in, we'll need to make sure the other service-runner scripts in the openenergymonitor/emonpi and emoncms/backup (and possibly other places too?) get cleaned up.

@pb66, @glynhudson , @Paul-Reed, @borpin , I have incorporated most of the changes previously discussed but appreciate if you could have a quick look and provide feedback.

greeebs added some commits Sep 21, 2018

Added systemd based service-runner in python
Added a new python based service-runner and .service file to enable it to run as
a service under systemd.
This should replace the service-runner.sh bash script included in other repos
that require service-runner.
@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Sep 21, 2018

I'm also not sure that running a service with a script file that lives under /var/www is entirely appropriate, but can probably be convinced otherwise.

I am with you on that. There have been some other discussions about that and I think it may be a good idea to take that forward. [edit] #794 has been created on this [\edit]

https://community.openenergymonitor.org/t/move-data-settings-file-to-a-common-location/6464/5

As part of this PR, any documentation relating to service-runner should be updated, old files deleted and a method to migrate users to the new script created (not much then) 😄.

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Sep 21, 2018

Not completely familiar with Git, but my feeling is this needs a dev branch to properly test it (remove old stuff etc).

@TrystanLea

This comment has been minimized.

Copy link
Member

TrystanLea commented Sep 22, 2018

Thanks @greeebs this looks great! I will try and test it this week

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Sep 22, 2018

Could someone tell me how to pull just the PR down (on top of a fresh master branch) and test it please?

@Paul-Reed Paul-Reed changed the base branch from master to systmdRunner Sep 22, 2018

@Paul-Reed Paul-Reed merged commit 316c5a9 into emoncms:systmdRunner Sep 22, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Paul-Reed

This comment has been minimized.

Copy link
Member

Paul-Reed commented Sep 22, 2018

@borpin
Brian, try the emoncms:systmdRunner branch

Paul

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Sep 25, 2018

@greeebs suddenly occurred to me. Which version of Python or is it compatible with both?

@greeebs

This comment has been minimized.

Copy link
Contributor

greeebs commented Sep 25, 2018

@borpin, I only tested it with v2.7

I don't think I used anything that was incompatible with python3 though.

@glynhudson

This comment has been minimized.

Copy link
Member

glynhudson commented Oct 3, 2018

Thanks @greeebs

I've just tested on Raspbian Stretch and it works great 👍
I will ensure this new service runner in the new location is used on the new Stretch emonSD.

However, I've just tested on two separate Jessie systems and get the error Failed to execute operation: No such file or directory when I try and enable the service.

Has anyone else been able to test on Jessie? Maybe the systemd service file needs to be in a different location for Jessie?

However, not working on Jessie is not a major issue since Jessie does not work on the new Pi3B+ therefore everyone will be moving to Stretch in the near future. Existing users can continue to use the old bash service runner in the emonPi repo which will continue to work fine.

Once this is merged into Emoncms core master I will add note on the emonPi repo to indicate the existence of this new service runner.

@greeebs

This comment has been minimized.

Copy link
Contributor

greeebs commented Oct 3, 2018

@glynhudson, I also had no success on jessie and after extensive reading of the man pages for systemd installed on my jessie systems, came to the conclusion that it was a systemd bug in the version that comes with jessie. It really should work the same way it does on stretch.

The way around it on jessie is to link the service file into /etc/systemd/system instead of /lib/systemd/system.
The down-side of that is that if you subsequently do a systemctl disable service-runner.service it will remove the symlink and the original file that it linked to (at least it did in my testing)

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Oct 3, 2018

The way around it on jessie is to link the service file into /etc/systemd/system instead of /lib/systemd/system.

My reading of the docs, is that the /lib/ location is for packaged installs and /etc/ for local installs or locally modified service files. I think /etc/ is the better location.

@greeebs

This comment has been minimized.

Copy link
Contributor

greeebs commented Oct 3, 2018

@borpin, I don't disgaree with that intention, but I urge you to test the difference between symlinking a file into /etc/systemd/system and then using systemctl enable/systemctl disable vs doing the same with a symlink into /lib/systemd/system.
In the testing I did on stretch, everything worked exactly the same with the symlink into /lib or /etc until I did the systemctl disable, at which point the original file (the one in /var/www/... that I had symlinked into /etc) got deleted along with the symlink in /etc/systemd/system.

I think the takeaway for me was that if you are going to put a file into /etc/systemd/system, copy the file in there, don't symlink it. That also means it needs to be copied in every time an update occurs. Either that or you make sure users never run sudo systemctl disable service-runner.service :)

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Oct 4, 2018

Ah yes I see. It was raised as a bug but shut down as it is by design to delete the link. Not sure why it should delete the original file - perhaps that is a bug?

I think a possible solution is to add the emoncms service files path to $SYSTEMD_UNIT_PATH (if it ends with an empty component (":"), the usual unit load path will be appended to the contents of the variable) as part of the emoncms install process. This might be the best option as it is then persistent and any future service files could be automatically found (though we will need to drop the service-runner sub folder). The advantage of this is that any updates would (I think) just require a sudo systemctl daemon-reload to be effective.

Another option would be to not create a symlink first but rather use a full path for the enable which creates the links. The disadvantage is that any subsequent enable will also need the path.

sudo systemctl enable /var/www/emoncms/scripts/services/service-runner/service-runner.service

or

sudo systemctl link /var/www/emoncms/scripts/services/service-runner/service-runner.service
sudo systemctl enable service-runner.service

However, I cannot see the point of the link command as it is not persistent across a disable.

Final option, as you say, is to copy the service file to the /etc folder (which is suggested in the man file). But this has the disadvantage for updating.

I think the general confusion has occurred from a misunderstanding as to what systemctl actually does when you call enable. Ideally, all service files should be outside the /etc/ path and then the enable call creates symlinks of services found in the specified set of paths. Differences between different Debian versions does not help 😦.

@greeebs

This comment has been minimized.

Copy link
Contributor

greeebs commented Oct 4, 2018

According to the jessie manpage https://manpages.debian.org/jessie/systemd/systemd.unit.5.en.html $SYSTEMD_UNIT_PATH is only available when systemd is running in user mode... The stretch version of that man page no longer includes the user mode restriction, so we're in the same boat regarding compatibility between versions :(

Which leaves us with either copying the file into /etc/systemd/system or as you suggested above,

sudo systemctl enable /var/www/emoncms/scripts/services/service-runner/service-runner.service

Do you know how that behaves if you do a subsequent sudo systemctl disable?

Maybe we just have two sets of instructions, one for jessie and one for stretch :)

Personally, I still think /lib/systemd/system is the right place for the symlink... Referring to Table 1 in that manpage I linked above, the reality for most users is that emoncms is just another "package" with one or more "vendor provided" .service unit file(s) (where OEM is the vendor). I would consider "local configuration" versions to be individual modifications each user may wish to make to the OEM provided versions...

It's just a shame /lib/systemd/system bugs out under jessie!

@greeebs

This comment has been minimized.

Copy link
Contributor

greeebs commented Oct 4, 2018

I've just been reading some more forum posts around the place on systemd and systemctl enable/disable... I'll try and play around a bit more with jessie and see if I can work out why it bugs out when the .service is in /lib/systemd/system... Other packages use that directory so it really should work - maybe there's something missing from the .service file which is no longer required in stretch.

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Oct 4, 2018

Do you know how that behaves if you do a subsequent sudo systemctl disable?

I think you need to use the full path command to enable again.

How about a hybrid solution:

  • For Jessie, the instruction is to use the full path in the enable command.
  • For Stretch, we modify the $SYSTEMD_UNIT_PATH (which future proofs further service files). You could still use the full path in the enable command on Jessie, but don't have to.

If we decide at a later time to move these types of files off the /var/www/ path altogether, the update required is to change the command instruction and/or the environment variable.

@pb66

This comment has been minimized.

Copy link
Collaborator

pb66 commented Oct 4, 2018

Have you tried using a hard symlink (ln without the -s arg), systemd shouldn't see it as a symlink and might not delete it, therefore the install instructions would be to just link and enable, then the systemctl enable/disable might work as usual, as might updating the unit file via emoncms/git.

@greeebs

This comment has been minimized.

Copy link
Contributor

greeebs commented Oct 4, 2018

@pb66, pretty sure that only works if both source and destination are on the same mount point, which may not always be the case?

@pb66

This comment has been minimized.

Copy link
Collaborator

pb66 commented Oct 4, 2018

which may not always be the case?

I think that the user base that have emoncms core and the OS on separate mountpoints might be quite small and can be catered for with a footnote, those users with split filesystems will have a little experience of this and be able to cope.

IMO it's better to have a simple solution for the masses than trying to accommodate all use cases, the next best alternative is to just set it up for "stretch" users and have special instructions for "jessie" users as it would be daft to be stuck with jessie considerations in the stretch code long after jessie has faded out. Having said that /lib/systemd/system is supposed to be for package managed files and isn't really the place for any emoncms unit files but do I understand your preference for a location where links are not deleted. Given there are fewer users that might fall foul of a multi-mountpoint FS than Jessie users, and the /etc path might be more appropriate to both jessie and stretch it might be the lesser evil, it's certainly the more KISS approach, if it works, this maybe irrelevant if it doesn't do the job in any case.

I just thought I would throw another option your way, it's a sad thing that possibly the easiest option is to write an init script as that would most likely work perfectly across both jessie and stretch (and wheezy too) you can see why the uptake of systemd is slow and meets resistance, it's just too complex for it's own good some of the time.

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Oct 4, 2018

$SYSTEMD_UNIT_PATH is only available when systemd is running in user mode.

@greeebs Ah yes - sorry.

the next best alternative is to just set it up for "stretch" users and have special instructions for "jessie" users

@pb66 I'd concur with this sentiment but add that if we can make it work for other future service units, that would be good.

Have you looked at using /etc/systemd/user described as for 'Local configuration' which should be in the standard path searched?

@glynhudson

This comment has been minimized.

Copy link
Member

glynhudson commented Oct 4, 2018

the next best alternative is to just set it up for "stretch" users and have special instructions for "jessie" users

This would be my preference. As I mentioned earlier I don't think many jessie users will make the effort to switch to this new service runner. It's now in place for Stretch going forward. We will not be releasing a Jessie image that includes this service runner.

@glynhudson

This comment has been minimized.

Copy link
Member

glynhudson commented Oct 4, 2018

I'm happy for this branch to be merged into Master as soon as your ready. Thanks again for your help 👍

@borpin

This comment has been minimized.

Copy link
Contributor

borpin commented Oct 12, 2018

I have just installed this on a DietPi VM - stretch. I decided to copy the file to /usr/lib/systemd/system/. and then systemctl enable service-runner.service.

Seems to run fine.

However, could you add in a comment in the readme that it runs as a user not as system so if not running on a Pi, the user name will need to be changed.

Actually looking at the MQTT service file, it does not have a user entry at all.

@pb66

This comment has been minimized.

Copy link
Collaborator

pb66 commented Oct 13, 2018

Actually looking at the MQTT service file, it does not have a user entry at all.

I think it would be bad practice to use "User=pi", If you do not want to use "User=root" (probably the default) then perhaps create a specific user for emoncms (and emonhub etc?). I always create a emoncms user for each instance and the original emonhub install guide adds a "emonhub" system user for emonhub. That has been carried forward to the emonpi variant and used in that service file too.

https://github.com/openenergymonitor/emonhub/blob/emon-pi/service/emonhub.service#L10

However, could you add in a comment in the readme that it runs as a user not as system so if not running on a Pi, the user name will need to be changed.

https://github.com/emoncms/emoncms/pull/1025/files#diff-b89a17ccfc5ff5bfcc42d3509fd56a50R15

@greeebs

This comment has been minimized.

Copy link
Contributor

greeebs commented Oct 14, 2018

As pb66 indicated above, the readme already says this:

If you are not running EmonCMS on Raspbian, modify the .service file to run the service
as an appropriate user. The service is configured to run as the user 'pi' by default.

Remember that this was a replacement for the emonPi service-runner which ran from the pi user's crontab. I have no idea if it needs to run as pi, I just made it an exact replacement for what was already there :)

@pb66

This comment has been minimized.

Copy link
Collaborator

pb66 commented Oct 15, 2018

I just made it an exact replacement for what was already there :)

My comment wasn't aimed at you, just my 2 cents worth on using "pi", it is a really bad position across most if not all the OEM projects that the user "pi" and more often the /home/pi folder are used as if they are globally there on all (Linux) os's.

glynhudson added a commit that referenced this pull request Oct 15, 2018

Merge pull request #1068 from emoncms/systmdRunner
Added systemd based service-runner in python (#1025)
@glynhudson

This comment has been minimized.

Copy link
Member

glynhudson commented Oct 15, 2018

This has now been merged into master

@greeebs

This comment has been minimized.

Copy link
Contributor

greeebs commented Oct 15, 2018

My comment wasn't aimed at you

I didn't take it that way and I agree with your sentiment entirely... it was more an explanation of why this service unit file has user=pi when @borpin didn't find a user= entry in the MQTT service unit file. A missing user= entry simply implies user=root which may be correct in MQTTs case.

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