Yeah the PR to put the change on hold was purely that! Just to stop more users falling into the issue that we didn’t have an immediate agreed fix in place for, especially before the emoncms v10 was released, but that ship has sailed and the “quick fix” didn’t get any attention in almost a week! ( I know you were away, but you aren’t the only one with access).
1, 3 and 4, great, I had pretty much done the same thing in my forks but realised there was little point in doing any PR’s by that time.
Regards the logfile creation in rc.local, IMO it doesn’t matter how temporary, temporary is, it’s not temporary enough. I explored getting the service unit to create the logdir as the init.d script has done so reliably for so long. I edited my service unit to include
ExecStart=/usr/share/emonhub/emonhub.py --config-file=/etc/emonhub/emonhub.conf --logfile=/var/log/emonhub/emonhub.log
PermissionsStartOnly=true
ExecStartPre=-/bin/mkdir -p /var/log/emonhub/
ExecStartPre=-/bin/chgrp -R emonhub /var/log/emonhub/
ExecStartPre=-/bin/chmod -R 777 /var/log/emonhub/
which works fine, it also includes some minor permission changes to allow the user to manipulate logfiles without using sudo (eg truncating or removing sensitive info before posting) and to set just the group ownership so the folder ownership is more in keeping with other package maintained log folders (ie root owned).
PS I also do not think the PIDFILE is used (unless type=forking) and the word “description” doesn’t need to be in the service unit description, when you look at the service log entries, “restarted the emonhub service description” doesn’t make much sense. So I now have
[Unit]
Description=emonHub service
DefaultDependencies=no
Before=shutdown.target
Conflicts=shutdown.target
Requires=local-fs.target
After=sysinit.target syslog.target local-fs.target
[Service]
User=emonhub
ExecStart=/usr/share/emonhub/emonhub.py --config-file=/etc/emonhub/emonhub.conf --logfile=/var/log/emonhub/emonhub.log
PermissionsStartOnly=true
ExecStartPre=-/bin/mkdir -p /var/log/emonhub/
ExecStartPre=-/bin/chgrp -R emonhub /var/log/emonhub/
ExecStartPre=-/bin/chmod -R 777 /var/log/emonhub/
Type=simple
Restart=always
[Install]
WantedBy=multi-user.target
and an entry in rc.local is not needed.
This don’t need to be a temporary “fix” either. IMO making the service unit(s) create their own log folder (and files?) makes for a much more robust set up, even if L2R is used, a service that can recreate it’s own log folder (at the cost of a couple of lines of code) is belt and braces, it would mean that even if users accidentally deleted emonhub.log, a simple restart would fix it.
Whilst it is not a good idea to start meddling with all the various packages (mysql redis etc) service units, I think there is value in ensuring that OEM services, intended for use on a sdcard (not exclusively) are able to create their own logfolders. There are now at least 2 emoncms services that will be logging to emoncms.log, so even the core emoncms install can have it’s log folder created before any web pages or requests are served. This will also shorten install steps and ensure any accidental changes to permissions or ownership are corrected automatically.
I’ve looked through the logrotate install/update and it seems to be currently complete (as intended on previous emonSD images but absent on the Oct2018 image) but I wonder about how “multi-run safe” that script would be, surely the backed up files will get overwritten on every run as symlinks will pass the -f test. Also there is some confusion around the hourly vs daily cron (linking then moving?) so I was going to propose these changes Comparing master...logrotate · emoncms/emoncms · GitHub which will backup either daily or hourly cron files (but not symlinked copies) and forces a new cron symlink, just in case an existing incorrect symlink blocks a correct one being written.
I had also added the logrotate log folder check and create to the cron so that the folder is always found (ie not required in rc.local) and so that permissions are corrected even if it is there since the rc.local tries to change the ownership to pi:pi, which I didn’t understand as pi doesn’t have the authority to run logrotate properly unless using sudo and then root ownership is fine!
There is also a problem with the recent addition of a /var/log/* wildcard being added that will cause issues with rotating rotated files as if they were independent logfiles (not as previous rotations). Plus the “rotate 2” was changed to “rotate 1” at the same time, IMO this should be put back (check to see if logs require rotation hourly · emoncms/emoncms@ddb0854 · GitHub).
Plus this “fix” seems to be on/off/on Update logrotate.conf · emoncms/emoncms@7cc85b7 · GitHub which I do not think is needed if logrotate is run as root (by cron), this maybe linked to the pi:pi ownership in rc.local?
I still think there are improvements to be had to both the way logrotate is implemented and the logrotate rules thenselves as part of the L2R roll-out, but a return to emonhub.log and the old intended logrotate is a good temporary position, although not perfect, it has been ok over the last couple of years, certainly good enough to see us through to roll out of a better solution.