Repository navigation
Log NUT query failures and save the UPS Poller ID - #58
Merged
Merged
Conversation
A failed NUT query only wrote to the UPS error_message column and -d output, so an unreachable upsd left nothing in the Cacti log; log a WARNING with the UPS, host, port and reason (never credentials). The UPS form displayed a Poller ID field that form_save() never stored; validate and save it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Linked-device reassignment remains unresolved, and distributed-poller behavior needs integration verification before approval.
1 open finding
What changed in this PR
Improves NUT failure diagnostics and saves UPS poller selections without claiming to resolve the reported “Unknown” status.
Changes:
- Logs failed NUT queries to the Cacti log.
- Validates and saves submitted Poller IDs.
- Adds source-assertion tests and changelog entries.
| File | Description |
|---|---|
| upses.php | Validates and stores Poller ID selections. |
| tests/Unit/NutWiringTest.php | Adds logging and Poller ID source assertions. |
| poller_apcupsd.php | Logs NUT query failures. |
| CHANGELOG.md | Documents both fixes under 1.4.0. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Changing a UPS's Poller ID updated apcupsd_ups but left the linked device on the old poller, so collection and the device assignment could disagree. Moving a device between pollers needs Cacti's own device handling, so reject the change with an explanation and have the user change the poller on the device. Compare against the UPS's stored value so a device already moved elsewhere does not block unrelated edits. Restructure the new tests as a compact dataset to avoid duplicating the neighbouring source-assertion tests (SonarCloud duplication gate), and add the new message to cacti.pot. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
bmfmancini
approved these changes
Oct 16, 2026
3 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two fixes found while investigating #57, where newly added NUT UPSes stay "Unknown" with nothing in the Cacti log:
collect_nut_ups_data()stored the reason a NUT query failed inapcupsd_ups.error_messageand printed it only with-d. The poller runs from thepoller_bottomhook with its output discarded, so an unreachable or misconfiguredupsdleft nothing in the Cacti log. It now writesWARNING: NUT query for UPS <name> (<host>:<port>) failed: <reason>(log typeAPCUPSD) before recording the failure. The NUT username and password are never included.form_save()never read it, so the choice was discarded and every UPS used the column default (poller 1). It is now validated (digits only) and stored when the field is posted. Changing the Poller ID of a UPS that is already linked to a Cacti Device is refused with an explanatory message: moving a device between pollers needs Cacti's own device handling (poller cache and remote replication), so the poller has to be changed on the device itself.Both changes are in
poller_apcupsd.phpandupses.php, with source-assertion tests intests/Unit/NutWiringTest.php, CHANGELOG entries under 1.4.0, and the new message added tolocales/po/cacti.pot.Related Issue
Fixes #57
Motivation and Context
"Unknown" is status 0, which means neither the success path (status 3) nor the failure path (status 1) of the NUT poll has run for that row. Every completed poller run also writes an
APCUPSD STATS:line, so its absence points at the poller script not completing a run. With this change, failures that do occur are visible in the Cacti log, which makes the next report diagnosable. I checkeddevelop's NUT client against a scripted fakeupsdand it connected, parsed the variables and reported ONLINE, so the NUT protocol code itself looks sound.If #57 persists after this change, run
php plugins/apcupsd/poller_apcupsd.php -das the Cacti poller user and look for theAPCUPSD STATS:line in the Cacti log.How Has This Been Tested?
php -lis clean on all changed PHP files.NutClientTest, which exercisesincludes/nut.phpand is unchanged by this PR.sql_save()), and the linked-device guard.upsd; the changes are small and covered by source-level assertions only.Types of changes
Checklist: