Skip to content

Update upsmon ALARM support and related themes from issue #415#2709

Merged
jimklimov merged 34 commits intonetworkupstools:masterfrom
jimklimov:issue-415
Dec 6, 2024
Merged

Update upsmon ALARM support and related themes from issue #415#2709
jimklimov merged 34 commits intonetworkupstools:masterfrom
jimklimov:issue-415

Conversation

@jimklimov
Copy link
Member

@jimklimov jimklimov commented Dec 3, 2024

Closes: #415
Follow-up to: #2658
Touches on: #2708

arnaudquette-eaton and others added 6 commits April 4, 2017 15:13
@arnaudquette-eaton
upsmon: add support for alarms
d4a6263 
Add general support for alarms to upsmon, which relates to the ups.status
variable of a device holding the flag "ALARM". upsmon now has a "notify type"
alarm, which allows to react on such events, as with other events

Signed-off-by: Arnaud Quette <[email protected]>
@arnaudquette-eaton
upsstats (CGI): add support for alarms
f7de20a 
Whenever a device publishes the ALARM flag in ups.status, the CGI will publish
it part of the Status

Signed-off-by: Arnaud Quette <[email protected]>
@jimklimov
Merge remote-tracking branch 'upstream/upsmon_alarm' into issue-415
fc80a94 
Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.{c,h}, docs, conf: apply suggestions from issue networ…
7f314af 
…kupstools#415 discussion adding to PR networkupstools#2658 changes

Previously merged-in the couple of commits from issue
networkupstools#415 proposal at
https://github.com/networkupstools/nut/tree/upsmon_alarm
for posterity, now cherry-picking some changes and further
discussion ideas from mailing list thread at
http://lists.alioth.debian.org/pipermail/nut-upsuser/2017-April/010591.html

Closes: networkupstools#415

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
drivers/usbhid-ups.c, drivers/netxml-ups.c, NEWS.adoc, UPGRADING.adoc…
c85e741 
…: handle STATUS_BIT(NOBATTERY) to also status_set("RB") [networkupstools#415]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.c, NEWS.adoc: optimize parse_status() by not checking …
97c6838 
…further strings if we had a match; report unexpected tokens [networkupstools#415, networkupstools#2708]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov jimklimov added this to the 2.8.3 milestone Dec 3, 2024
@jimklimov
Copy link
Member Author

jimklimov commented Dec 3, 2024

NOTE: This PR currently stops short of actually handling the notifications, as we would need some sort of Map<String, Boolean> to track that a non-standard token was raised once and remains raised continuously (otherwise it would notify about it every cycle).

Probably this can be done with dstate/sstate and a catch-all method similar to ups_is_cal et al (accepting the token as another parameter, and not caring about it beyond notification).

desertwitch
desertwitch approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@desertwitch desertwitch left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (so far) 👍

@jimklimov
clients/upsmon.{c,h}, NEWS.adoc: revise do_notify() to allow passing …
c7c0a83 
…an extra arg into formatting strings with two "%s" placeholders [networkupstools#415]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
upsmon code, docs, conf: introduce "OTHER"/"NOTOTHER" notifications f…
6bf775c 
…or unknown tokens in `ups.status` [networkupstools#415]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
docs/new-drivers.txt: in "Status data", stress the life-cycle of `ups…
32fcb3f 
….status` flag tokens [networkupstools#2708]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.c: allow to get_var(ups, "alarm"...) and use it in ups…
04087cc 
…_is_alarm() notification [networkupstools#415]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.c: ups_is_alarm(): track if ups.alarm value changed, r…
380b3e4 
…e-notify [networkupstools#415]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.h, docs: default "ALARM" message format to two "%s" pl…
1ac8b9a 
…aceholders [networkupstools#415]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
Copy link
Member Author

jimklimov commented Dec 3, 2024 •
edited
Loading

Updated with more comprehensive upsmon support for "ALARM" (to report ups.alarm contents if the formatting string has two %s placeholders), and newly introduced older discussed "OTHER" to report any changes about ups.status tokens not known to upsmon.

Note that unlike most other notifications about change of UPS status, the "OTHER"/"NOTOTHER" events do not map to a specific ups.status flag (so there is no code elsewhere to support a token named OTHER as one of UPS status flag values). This is an upsmon internal detail.

The napkin logic makes sense to me and compiles, but was not tested - that would be most welcome :D

@jimklimov jimklimov added the need testing Code looks reasonable, but the feature would better be tested against hardware or OSes label Dec 3, 2024
@jimklimov
Copy link
Member Author

jimklimov commented Dec 5, 2024 •
edited
Loading

Verified with make check-NIT-sandbox-devel and modified generated dummy.seq to include alarm bits:

ups.status: OB
TIMER 5
ups.status: OL
TIMER 5
ups.status: ALARM OL BOGUS
ups.alarm: Test bug!
TIMER 2

Console messages:

UPS dummy@localhost:12345 on battery
UPS dummy@localhost:12345 on line power
UPS dummy@localhost:12345: one or more active alarms: [Test bug!]
UPS dummy@localhost:12345: has at least one unclassified status token: [BOGUS]
UPS dummy@localhost:12345 is no longer in an alarm state (no active alarms)
UPS dummy@localhost:12345 on battery
UPS dummy@localhost:12345 has no unclassified status tokens anymore

Using notifyme-debug as the handler got:

...
Thu Dec  5 20:10:29 UTC 2024    [uid=1000(jim) gid=1000(jim) groups=1000(jim)]        ONBATT  [dummy@localhost:12345]:      UPS dummy@localhost:12345 on battery    (1 tokens)
Thu Dec  5 20:10:39 UTC 2024    [uid=1000(jim) gid=1000(jim) groups=1000(jim)]        ONLINE  [dummy@localhost:12345]:      UPS dummy@localhost:12345 on line power (1 tokens)
Thu Dec  5 20:10:44 UTC 2024    [uid=1000(jim) gid=1000(jim) groups=1000(jim)]        ALARM   [dummy@localhost:12345]:      UPS dummy@localhost:12345: one or more active alarms: [Test bug!]       (1 tokens)
Thu Dec  5 20:10:44 UTC 2024    [uid=1000(jim) gid=1000(jim) groups=1000(jim)]        OTHER   [dummy@localhost:12345]:      UPS dummy@localhost:12345: has at least one unclassified status token: [BOGUS]  (1 tokens)
Thu Dec  5 20:10:49 UTC 2024    [uid=1000(jim) gid=1000(jim) groups=1000(jim)]        ONBATT  [dummy@localhost:12345]:      UPS dummy@localhost:12345 on battery    (1 tokens)
Thu Dec  5 20:10:49 UTC 2024    [uid=1000(jim) gid=1000(jim) groups=1000(jim)]        NOTALARM        [dummy@localhost:12345]:      UPS dummy@localhost:12345 is no longer in an alarm state (no active alarms)     (1 tokens)
Thu Dec  5 20:10:49 UTC 2024    [uid=1000(jim) gid=1000(jim) groups=1000(jim)]        NOTOTHER        [dummy@localhost:12345]:      UPS dummy@localhost:12345 has no unclassified status tokens anymore     (1 tokens)
...

It seems to lock up after handling "NOTOTHER" though "ALARM/NOTALARM" handling behaves well, investigating.

UPDATE: Small but important bug found, fixed :)

@jimklimov
conf/upsmon.conf.sample.in: suggest NOTIFYFLAG for OTHER/NOTOTHER too […
d4c361d 
…networkupstools#415]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
scripts/misc/notifyme-debug: honour TEMPDIR or TMPDIR set by caller; …
25dcfab 
…fall back to /dev/shm or /tmp otherwise [networkupstools#2709]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
tests/NIT/Makefile.am: allow to pass custom NUT_PORT and NIT_CASE [ne…
5af374d 
…tworkupstools#2709]

...default the latter to `testgroup_sandbox_upsmon_master` now
(includes the earlier `testcase_sandbox_start_drivers_after_upsd`)

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
tests/NIT/nit.sh: testcase_sandbox_upsc_query_timer(): add a retry wi…
0f81a75 
…th longer delay [networkupstools#2709]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
tests/NIT/nit.sh: generatecfg_upsmon_trivial(): refer to NOTIFYCMD=..…
3d63a05 
….notifyme-debug where possible [networkupstools#2709]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
common/common.c: refactor upsnotify_suggest_NUT_QUIET_INIT_UPSNOTIFY_…
3d6ccd8 
…once() [networkupstools#1590, networkupstools#2136]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
common/common.c: debug-log isPreparingForSleep() outcomes (when extre…
66a4c31 
…mely high verbosity level is on) [networkupstools#1070]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
common/common.c: only close_sdbus_once() if we are the same PID that …
a6496a6 
…originally opened it [networkupstools#1070]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.c: break the main sub-loop where we check sleep_inhibi…
8fa18c0 
…tor_status also if exit_flag gets raised (e.g. Ctrl+C during the wait) [networkupstools#1070]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.c: report start/end/duration of the "sleepval" delay b…
a1125ab 
…etween main loop cycles [networkupstools#1070]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 5, 2024
@jimklimov
clients/upsmon.c: notify(): clarify (when debug-logging) upsmon-centr…
a0b5ccf 
…ic messages if upsname==NULL [networkupstools#415, networkupstools#2709]

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 5, 2024
@jimklimov
clients/upsmon.c: parse_status(): fix releasing of ups->status_tokens […
1a778df 
…networkupstools#415, networkupstools#2709]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.c: redefine_ups(): comment a concern about "pv" changes
50965c0 
Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.c: addups(): start with definitive states of all fields
a36bc24 
Kudos to `make check-NI-sandbox-devel` and
LD_LIBRARY_PATH=`pwd`/clients/.libs valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes ./clients/.libs/upsmon -F

Signed-off-by: Jim Klimov <[email protected]>
jimklimov added a commit to jimklimov/nut that referenced this pull request Dec 6, 2024
@jimklimov
clients/upsmon.c: ups_free(): release ups->status_tokens if needed [n…
63a4bcc 
…etworkupstools#415, networkupstools#2709]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
Revert "common/common.c: only close_sdbus_once() if we are the same P…
bd97de8 
…ID that originally opened it [networkupstools#1070]"

This reverts commit a6496a6:
close from a child process does not seem to impact parent,
but a non-close is a memory leak when such child exits.

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
tests/NIT/nit.sh: generatecfg_upsmon_trivial() allow to toggle NOTIFY…
a14a55f 
…FLAG targets

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
tests/NIT/nit.sh: refer to scripts/valgrind for debugging in a sandbox
ef3c86c 
Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.c: ups_free(): release ups->status_tokens if needed [n…
9046e23 
…etworkupstools#415, networkupstools#2709]

Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
clients/upsmon.c: upsmon_cleanup(): free(configfile) in the end
245e18e 
Signed-off-by: Jim Klimov <[email protected]>
@jimklimov
Copy link
Member Author

jimklimov commented Dec 6, 2024

Had some fun with valgrind to make sure these alert loops do not leak memory...

@jimklimov jimklimov merged commit f657e0e into networkupstools:master Dec 6, 2024
@jimklimov jimklimov deleted the issue-415 branch December 6, 2024 15:01
@jimklimov jimklimov restored the issue-415 branch April 15, 2025 09:45
@jimklimov jimklimov deleted the issue-415 branch April 15, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@desertwitch desertwitch desertwitch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

documentation need testing Code looks reasonable, but the feature would better be tested against hardware or OSes upsmon

Projects

None yet

Milestone

2.8.3

Development

Successfully merging this pull request may close these issues.

upsmon: add ALARM support

3 participants

@jimklimov @desertwitch @arnaudquette-eaton