openenergymonitor / emonpi

Raspberry Pi Based Energy Monitor. Hardware, Firmware & related software for the PI.
https://guide.openenergymonitor.org/setup
270 stars 113 forks source link

Rewrite service-runner in Python #65

Closed greeebs closed 6 years ago

greeebs commented 6 years ago

Since the switch to redis, the bash script version of service-runner causes unnecessary load on redis due to having to create a new connection every iteration of the loop. Rewriting it in Python uses a persistent connection to redis.

greeebs commented 6 years ago

Original discussion thread that started this off: https://community.openenergymonitor.org/t/rpi-3b-getting-hot-redis-high-cpu-usage-emonsd-13jun18-dev-specific/8263

glynhudson commented 6 years ago

Hi @greeebs,

Thanks for this, I've just tried to test. First I needed to install the python redis module

pip install redis

Now when I run the python service runner it seems to run then exit without printing anything. Is there anything I'm misssing?

$ pi@emonpi:~/emonpi $ ./service-runner
$
borpin commented 6 years ago

I would suggest taking out all the PID stuff and simply make a systemd control file to run this as a daemon.

Let the builtin OS tools do all that stuff for you.

greeebs commented 6 years ago

@glynhudson , I just dropped it straight into a Pi with a freshly installed emonSD-13Jun18 SD Card image with no other changes. The behaviour you see is exactly what you get when the /tmp/service-runner file is locked. Have you made sure you killed the one normally started by cron every minute first?

@borpin, my intention was simply to replace the existing bash script which was already executed by cron every minute... Functionally, this script is an exact replacement for the bash version. I agree completely that using systemd is a much better approach, I guess I can have a look at that too.

greeebs commented 6 years ago

On the subject of systemd, looking at the existing emonSD-13Jun18 image, any tips on where the service unit files should actually end up? There doesn't appear to be anything emon related under /etc/systemd/system - it looks like all of the existing ones are sitting under the SysV init folder structure?

glynhudson commented 6 years ago

Yes, your right it was actually running fine. I can see the log output in /var/log/service-runner.log when the script was started via cron. I made sure to stop the old script first.

Your right, it would be best ran by systemd. On the emonPi systemd service files are stored in /etc/systemd/system/ e.g. mqtt_input.service. Most of the emonPi services e.g. emonPiLCD are still running as init.d services in /etc/init.d .

If you could adapt the script to run as a systemd process, that would be really great.

greeebs commented 6 years ago

Looks like I've had a git fail trying to add additional changes... I'll try and clean that up!

glynhudson commented 6 years ago

Great, just tested the systemd update. It works well for me.

I see you renamed service-runner to service-runner.py, this is a good idea since if you restored service-runner this would mean that older systems could continue to run the older cron bash version. and the new Stretch image I'm working on will use the new systemd version. Users can obviously choose to self upgrade.

We should probably also add upgrade instructions, something along the lines of

git pull 
sudo pip install redis
sudo ln -s /home/pi/emonpi/service-runner.service /etc/systemd/system
sudo systemctl daemon-reload
sudo systemctl enable service-runner
sudo systemctl start service-runner
sudo systemctl status service-runner

Anyone else like to test before I merge @borpi @Paul-Reed

glynhudson commented 6 years ago

Looks like I've had a git fail trying to add additional changes... I'll try and clean that up!

Don't worry, it happens! At least all the changes look like their preset and correct

greeebs commented 6 years ago

We should probably also add upgrade instructions, something along the lines of

I agree. Where would those instructions go?

Don't worry, it happens! At least all the changes look like their preset and correct

I don't use git enough to be completely confident in what I'm doing... I've somehow merged a heap of commits from a different branch into here but yes the particular files I changed are all there and correct. Clean-up attempt is continuing :) hopefully thats sorted it

glynhudson commented 6 years ago

I've added install instructions and restored the old bash service runner to maintain compatibility with older systems.

I wasn't able to push commits to your PR, in the future it would be a good idea to tick the "allow edits from maintainers" box when opening a PR: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

For now I've pushed all our changes up as a new branch on this repo and opened another PR https://github.com/openenergymonitor/emonpi/pull/66

glynhudson commented 6 years ago

I will close this PR, please use the new one