Weathermap fix for local rrdcached users and cron fix

fbouynot commented 1 year ago

The last commit in librenms-plugins https://github.com/librenms-plugins/Weathermap/pull/82 did break some things I'm trying to fix.

The env path at the start of map-poller.php is missing, so cron cannot call the file anymore The fix for remote rrdcached breaks local rrdcached installs I deal with both cases separately in map-poller, ~~although I'm not sure how to do it in WeatherMapDataSource_rrd.php~~ ~~My PR might revert what Skylark did for rrdcached remote installs though.~~

jwiechmann commented 1 year ago

I can confirm that this PR fixed our librenms crontab issue since updating to php81 with librenms version 22.8.0 on Debian 10 and on Debian 11. Thanks!

santiag0z commented 1 year ago

Hi,

This PR, corrects the problem described here: https://community.librenms.org/t/html-pages-not-being-automated-created-in-output-folder-opt-librenms-html-plugins-weathermap-output/19480

@laf or @murrant, can you check?

kkrumm1 commented 1 year ago

this will fix the cron issue but I'm worried about the remote rrd cached fix.

kkrumm1 commented 1 year ago

can you break up the PR?

cjwbath commented 1 year ago

Also, in 0496219 you are possibly working around a bug that actually exists on line 281 of weathermap.php (that was not actually introduced by LoveSkylark's change), in that if --chdir='' is passed rather than being omitted altogether, it doesn't trigger the $rrd_default_path1 to be appended, because of some dodgy use of isset().

I'm not sure if there is some intentional hidden meaning behind an empty string argument to --chdir that I have missed, but I do know it broke all the paths in my setup until I fixed weathermap.php:281 to $rrdbase = (isset($chdir) && $chdir !== '') ? $chdir : $rrd_default_path1;. Sorry I'm too lazy to patch and PR this myself.

fbouynot commented 1 year ago

I did add the correction you suggested after I could confirm it doesn't break my setup. Thanks!

I'm not sure if there is some intentional hidden meaning behind an empty string argument to --chdir that I have missed

Are you using distributed polling? I think I remember it is needed on centralized polling but I don't know why.

cjwbath commented 1 year ago

Are you using distributed polling? I think I remember it is needed on centralized polling but I don't know why.

I have distributed polling, with Weathermap (and the rest of the Web UI) running on the rrdcached server, communicating via a Unix socket. I guess even with centralised polling, if you were using local rrdcached you'd need it to work the same way.

Thinking out loud some more...

If you weren't using rrdcached (i.e. doing direct RRD file access) I suppose you might want to chdir to /opt/librenms/rrd, but just using the absolute RRD pathnames the script builds would also work fine.

If rrdcached was running on a different machine to weathermap, you'd use a TCP socket and so would want the filenames being passed to rrdtool to be relative to the server's BASE_PATH, not absolute (which is not allowed). Ah yes, that's it - we don't want to automatically build absolute paths if using TCP so --chdir='' has the meaning of "don't automatically prepend the RRD base path".

So your original 0496219 is the right solution and my suggested change to weathermap.php:281 is wrong (just tested and it does break TCP). I still think the original isset($chdir) != '' is nasty and misleading though; isset returns a boolean so why are we comparing to a string? How about: $rrdbase = isset($chdir) ? $chdir : $rrd_default_path1; instead?

Sorry for any confusion caused.

fbouynot commented 1 year ago

I still think the original isset($chdir) != '' is nasty and misleading though; isset returns a boolean so why are we comparing to a string? How about: $rrdbase = isset($chdir) ? $chdir : $rrd_default_path1; instead?

Yes you are right, I fixed that. On my rrdcached + local polling everything works fine with it, can you confirm it doesn't break rrdcached + distributed neither no rrdcached+tcp?

cjwbath commented 1 year ago

Yes, looks good to me. We are no longer changing any logic in weathermap.php so that's as it always was, and your 0496219 works because:

$config['rrdcached'] = unset, then chdir to the rrd_dir to find files locally
$config['rrdcached'] = a Unix socket, then don't supply --chdir at all so absolute paths are built
$config['rrdcached'] = a TCP socket, then use --chdir='' so that relative paths based from the default RRD location are used

geyson2911 commented 1 year ago

O último commit no librenms-plugins #82 quebrou algumas coisas que estou tentando consertar.

O caminho env no início do map-poller.php está faltando, então o cron não pode mais chamar o arquivo A correção para rrdcached remoto quebra as instalações locais do rrdcached Eu lido com ambos os casos separadamente no map-poller,~embora eu não tenha certeza de como fazer isso em WeatherMapDataSource_rrd.php~ ~Meu PR pode reverter o que Skylark fez para instalações remotas rrdcached.~

I followed all the steps but I didn't get success, I still have the same problem

can you help me Screenshot_2

sorano commented 1 year ago

@fbouynot Thanks for your effort. The version from your repo worked perfectly fine for me with local rrdcached.

ZPrimed commented 1 year ago

Yes, looks good to me. We are no longer changing any logic in weathermap.php so that's as it always was, and your 0496219 works because:

$config['rrdcached'] = unset, then chdir to the rrd_dir to find files locally

$config['rrdcached'] = a Unix socket, then don't supply --chdir at all so absolute paths are built

$config['rrdcached'] = a TCP socket, then use --chdir='' so that relative paths based from the default RRD location are used

~~So, there's at least one stumbling block here... this is assuming you've hard-coded $config['rrdcached'] in the base librenms config.php file. I was setting it via the GUI...~~

~~I don't really care where I set it, I had just seen from the dev team various times that setting config via the GUI (which sets it in the SQL DB, I believe?) is "the best way."~~

Nevermind the above; this is correctly pulling config out of the database too (I don't have to have this config stanza in the base config.php).

Is there a timeline on merging this PR? I think it will fix my problem (local / unix socket RRDcached, but I'm getting relative paths to the files which don't seem to work).

ciroiriarte commented 1 year ago

Rooting for the fix too!, moving to 8.1 wreaked havoc over here :)

Weathermapper is pretty much dead in the water, trying to save Weathermap

fbouynot commented 1 year ago

Yes, looks good to me. We are no longer changing any logic in weathermap.php so that's as it always was, and your 0496219 works because:

$config['rrdcached'] = unset, then chdir to the rrd_dir to find files locally

$config['rrdcached'] = a Unix socket, then don't supply --chdir at all so absolute paths are built

$config['rrdcached'] = a TCP socket, then use --chdir='' so that relative paths based from the default RRD location are used

~So, there's at least one stumbling block here... this is assuming you've hard-coded $config['rrdcached'] in the base librenms config.php file. I was setting it via the GUI...~

~I don't really care where I set it, I had just seen from the dev team various times that setting config via the GUI (which sets it in the SQL DB, I believe?) is "the best way."~

Nevermind the above; this is correctly pulling config out of the database too (I don't have to have this config stanza in the base config.php).

Is there a timeline on merging this PR? I think it will fix my problem (local / unix socket RRDcached, but I'm getting relative paths to the files which don't seem to work).

You can use this PR using:

cd /opt/librenms/html/plugins/Weathermap/
curl https://patch-diff.githubusercontent.com/raw/librenms-plugins/Weathermap/pull/85.diff | git apply

For the merging, we are waiting for laf to merge the requests, he's alone on this repo, and it looks like he doesn't have a lot of time right now.

ciroiriarte commented 1 year ago

Yes, looks good to me. We are no longer changing any logic in weathermap.php so that's as it always was, and your 0496219 works because:

$config['rrdcached'] = unset, then chdir to the rrd_dir to find files locally

$config['rrdcached'] = a Unix socket, then don't supply --chdir at all so absolute paths are built

$config['rrdcached'] = a TCP socket, then use --chdir='' so that relative paths based from the default RRD location are used

~So, there's at least one stumbling block here... this is assuming you've hard-coded $config['rrdcached'] in the base librenms config.php file. I was setting it via the GUI...~ ~I don't really care where I set it, I had just seen from the dev team various times that setting config via the GUI (which sets it in the SQL DB, I believe?) is "the best way."~ Nevermind the above; this is correctly pulling config out of the database too (I don't have to have this config stanza in the base config.php). Is there a timeline on merging this PR? I think it will fix my problem (local / unix socket RRDcached, but I'm getting relative paths to the files which don't seem to work).

You can use this PR using:
cd /opt/librenms/html/plugins/Weathermap/
curl https://patch-diff.githubusercontent.com/raw/librenms-plugins/Weathermap/pull/85.diff | git apply
For the merging, we are waiting for laf to merge the requests, he's alone on this repo, and it looks like he doesn't have a lot of time right now.

how do I sync up once the PR is accepted if I patch this way now?

ciroiriarte commented 1 year ago

I can confirm it fixes map generation from cron, on a opensuse Leap 15.4 updated with PHP 8.1

fbouynot commented 1 year ago

how do I sync up once the PR is accepted if I patch this way now?

cd /opt/librenms/html/plugins/Weathermap/
curl https://patch-diff.githubusercontent.com/raw/librenms-plugins/Weathermap/pull/85.diff | git apply -R
git pull

geyson2911 commented 1 year ago

solved for me thank you

santiag0z commented 1 year ago

What is missing for this PR to be merged?

lovalim commented 1 year ago

how do I sync up once the PR is accepted if I patch this way now?

cd /opt/librenms/html/plugins/Weathermap/
curl https://patch-diff.githubusercontent.com/raw/librenms-plugins/Weathermap/pull/85.diff | git apply -R
git pull

master@monitor-srv:/opt/librenms/html/plugins/Weathermap$ sudo curl https://patch-diff.githubusercontent.com/raw/libr enms-plugins/Weathermap/pull/85.diff | git apply -R git pull error: can't open patch 'git': No such file or directory [sudo] password for master: Sorry, try again. [sudo] password for master: % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 4515 0 4515 0 0 4871 0 --:--:-- --:--:-- --:--:-- 4870 curl: (23) Failure writing output to destination master@monitor-srv:/opt/librenms/html/plugins/Weathermap$ su - librenms Password: librenms@monitor-srv:~$ librenms@monitor-srv:~$ librenms@monitor-srv:~$ cd /opt/librenms/html/plugins/Weathermap/ librenms@monitor-srv:~/html/plugins/Weathermap$ curl https://patch-diff.githubusercontent.com/raw/librenms-plugins/We athermap/pull/85.diff | git apply -R git pull % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0error: can't open patch 'git': No such file or directory 100 4514 0 4514 0 0 5420 0 --:--:-- --:--:-- --:--:-- 5418 curl: (23) Failure writing output to destination

fbouynot commented 1 year ago

You are missing a newline before git pull. Also, this is the revert actions.

So in order to apply this PR, you have to do:

su - librenms
cd /opt/librenms/html/plugins/Weathermap/
curl https://patch-diff.githubusercontent.com/raw/librenms-plugins/Weathermap/pull/85.diff | git apply

Once it is merged and you want to revert the patch and update the repo, at this moment only (not now), you will do:

su - librenms
cd /opt/librenms/html/plugins/Weathermap/
curl https://patch-diff.githubusercontent.com/raw/librenms-plugins/Weathermap/pull/85.diff | git apply -R
git pull