21

As per my Issue that I raised in mid FEB!!!

22

@glyn.hudson, Great you’ve agreed this is the best way forward. But this isn’t my idea,

This was originally submitted as a PR by @bwduncan on 29th Dec 2018 and merged by you that same day.

It is still, to this day, part of the “Move mysql database section” of the read-only.md install guide

image
image1227×435 16.9 KB

And @borpin has been tirelessly and frequently reminding us of this for months too, so the credit for this “fix” isn’t really mine, it was already fixed in principle, the image just pre-dates the fix and it hasn’t been rolled out in the emonpi updater.

To fix this via the updater (ie non-interactive), you will need to create a folder, and then write the file and reload/restart manually. Something like

sudo mkdir -p /etc/systemd/system/mariadb.service.d
printf "[Service]\nProtectHome=false\n" | sudo tee /etc/systemd/system/mariadb.service.d/override.conf
sudo systemctl daemon-reload
sudo systemctl restart mariadb

(untested)

1 Like
23

Ah yes, thanks a lot.

I’ve implemented this fix as part of an emonSD update

https://github.com/openenergymonitor/emonpi/commit/59faeea2494fd27f76b66af04d73983917c3488c

emonSD users just need to run Emoncms update to apply this fix.

24

Have you tested it?

I think the systemctl edit function creates the mariadb.service.d folder, so as per my suggestion above, your solution needs to also create that folder as you cannot cp a file to a folder that doesn’t exist

1 Like
25

Ah yes, correct folder needs to be created.

I’ve updated the update script and tested on a fresh image:

https://github.com/openenergymonitor/emonpi/commit/fd6ec7837a7d3e3246d1cec9af149010da48f64a

https://github.com/openenergymonitor/emonpi/commit/c15fa8493c60e4e5af03a5833b42600773418df2

Thanks to @bwduncan for the PR, I do remember it now. The most important thing is that this fix has now been included in @TrystanLea new image build scripts which means this change will automatically be included in any future image builds. Thanks for your help.

Going forward any improvements to the image build can be made as direct PR’s to these build scripts to ensure improvements are implemented.

1 Like
26

Could we move the scripts, as proposed, to their own repo? It will make commenting on them much easier.

27

I really hope this is not needed/included in the new install scripts. The mysql folder was only moved as part of the RO changes, the last and next images are not RO! There will be no need to move the mysql folder and if there was, there are much better ways to do it and places to put it. None of the other previous mysql tweaks and fiddles are currently in the new install scripts so can we please remove this again, mysql shouldn’t need access to any users homedir.

from the systemd.exec.5 man page

ProtectHome=
Takes a boolean argument or “read-only”. If true, the directories
/home, /root and /run/user are made inaccessible and empty for
processes invoked by this unit. If set to “read-only”, the three
directories are made read-only instead. It is recommended to
enable this setting for all long-running services (in particular
network-facing ones), to ensure they cannot get access to private
user data, unless the services actually require access to the
user’s private data.** This setting is implied if DynamicUser= is
set. For this setting the same restrictions regarding mount
propagation and privileges apply as for ReadOnlyPaths= and
related calls, see above.

Firstly I agree the use of PR’s over directly editing the source code would be a far better way of discussing changes before they are merged in to production repo’s!

Secondly, these changes are already in the build guide, why on earth would we suspect they need to go into the update script? It’s only due to diving into the commit history that I discovered these changes were made after the release of the Oct 2018 image. Why would I suspect the build guide had been updated since the build when the (RO - RW) changes contained in that release have not yet been added? Plus if the update script had also been updated when the PR for those changes to the build guide was reviewed the issue would not of occurred.

and lastly, it is way too difficult to follow what where how things are supposed to be to know were how to submit a PR (hence the changes in the build guide not reflected in the update script). I was just getting to grips with the old emonpi update routine and it has all changed again. I didn’t even notice that I was commenting on your changes to the “install” script when I said about creating the folder, or I would have said something about this setting not being needed then, that comment was firmly aimed at fixing the existing update script as in my mind, that is what “The most important thing is” to avoid anyone else falling foul of this issue.

2 Likes
28

Agreed. I’m running the install script and it is not needed.

It will be needed in the update scripts while the mysql data is in the home folder.

More haste, less speed.

2 Likes
30

I’ve just submitted a PR for changes to the “ProtectHome fix” that removes it from the install script and changes the update code to create the additional service unit edit file rather than cp one included in the repo. Removing that repo copy tidies things up as it may have caused confusion once it was redundant.

2 Likes
31

Answering @borpin’s comments on github here to try and keep discussion in one place (ref Revise mariadb ProtectHome fix by pb66 · Pull Request #104 · openenergymonitor/emonpi · GitHub)

Could you check for the location of the mysql database and only introduce the change if it is located in home?

I did consider that but did not want to over complicate things at this point,

Just thinking that the update script is the same however the emoncms instance has been installed. Having installed via the script, I think this update would introduce an override that was not required.

I’m thinking that if a new update script isn’t introduced with the new install script, then the old one will at the very least require serious review as there are going to be other anomalies too.

This is simply a PR to remove the unnecessary code in the install and since we already have so many redundant files in the various repos, I modified the required fix to existing images to not use another file as the need for the fix should be short lived, no more.

Running an old (current) update script on a newly installed (via new script) image would indeed add the “fix” without need, but that hopefully will not be a possibility, having installed via a script, the latest update method will (hopefully) have been installed and therefore be recognised as a new style install and run a new style update (script or function) rather than every line in the updater needing individual qualification. But who knows?

Or check and see if the newer /var/opt/emon/ location exists and not install the override if it does?

No not really, this either set’s that path in concrete, introducing a hardcoded path again, Or a more complex arrangement is needed to check the installer config and get the actual install path.

There are other issues caused by the mysql location, rather than burn time and effort trying to get a sticky plaster perfectly fitted, it would make much more sense to correct the issue. ie undo the edits made to the mysql config and add a symlink to where it should be located so that all the old links and paths work, this gets things back as close as possible to “stock” whilst still supporting RO installs.

If all the Oct 2018 image has different is fstab mounts the partitions RW, maybe those “oct 2018” changes should now be added to the build guide and the emonpi updater too. It seems a fairly trivial task to get the pre-Oct2018 images (and docs) up to Oct2018 spec, then finish the job properly by putting the mysql folder location and config paths back as stock.

This way moving forward, all mysql installs across all updated images would be the same and correct, no patches needed!

[edit] I have added another PR for a simple test to check if the /home/pi/data path is used in the mysql config as this is more accurate than just seeing if the mysql folder exists on that path. (Glyn had already merged the previous PR before I added this).

32

The mariadb help What to Do if MariaDB Doesn't Start - MariaDB Knowledge Base has a couple of ways to extract what datadir is set to. I have noticed before that there are several datadirentries when on the SDImage so perhaps a check to see if any datadir entry has ‘home’ in it would be even more robust?

33

Just how robust do you need this sticking plaster to be? This is just a quick fix, the “proper fix” is to put the config back as it was!

Come to think of it, are all images even all definitely mariadb, aren’t some still mysql?

Rather than keep reviewing the fixes and applying further fixes to fixes and fixing the issues caused by the fixes, it would make far more sense to fix it properly than getting the partial fixes absolutely correct and robust. It doesn’t matter how well coded a fix is if it doesn’t entirely fix the issue it’s never gonna be good enough.

1 Like