-
Notifications
You must be signed in to change notification settings - Fork 3.8k
nut: fix driver, server, and monitor reload/stop #28382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
nut: fix driver, server, and monitor reload/stop #28382
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
a298ac3 to
865f444
Compare
1df0cf7 to
a6ad063
Compare
|
I did get this error (backported patches to 24.10, with a fixup for basing it on my patch - my branch called nut-backport-2 ): But removing and re-inserting xhci_mtk_hcd appears to have made it work. It did appear to find the ups automatically once I did that, though there was an error/warning? I also got this a few minutes later which looked unfamiliar: |
To clarify, did it not work after a reboot, or did you do one of the following?
|
These are both harmless. The second message is slated to be removed or made suppressable as it only systemd has a notification tech that is applicable. The first is just letting you now that process is running under a supervisor rather than as a standalone daemon. |
This I am not sure about. Maybe a delayed reaction to unplugging and plugging back in? Does they also appear in the bootlogs if you reboot with the UPS attached? |
0802764 to
cf3ebaa
Compare
|
@BKPepe Any chance of another Copilot run? I tried a GitLab Duo review, but it keeps giving an error and saying "please request a new review", so I am not able to do it there. |
cf3ebaa to
997e24e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
997e24e to
a7eef34
Compare
|
@BKPepe FYI I was able to get GitLab Duo to do a review by setting its prompt injection protection settings to log only instead of interrupt. Apparently something in this PR, or in the OpenWrt codebase triggers its (and maybe Copilot's as well, which is why I mention it) prompt injection protection when doing code review. |
a7eef34 to
99e6a9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
99e6a9a to
1ed68b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1ed68b9 to
b8cb282
Compare
b8cb282 to
e4555ff
Compare
|
More work is needed; I realized that when a ups driver is deconfigured, it is not being stopped. |
|
Ok, resolved that. All going well, once I test on real hardware, it should be ready. |
9ac3a81 to
eec808c
Compare
Some driver options were noted as '(Diplay)' even though they are added to the `ups.conf` configuration file (this is because the drivers use them in the messages they emit). Since not all drivers support these options and it is quite a large job to manage the which options apply to which of the many drivers for nut, for now we add informational text to inform the user of the situation. Discussed in openwrt#8200 mfr, model fields not valid on usbhid-ups driver and openwrt/packages#28382 . As noted in the latter's comments, more than one driver uses the problematic options, so the solution is not as simple as hiding the options for any driver except that one. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
eec808c to
bdec640
Compare
|
My test device is slated to arrive early next week, so I look forward to final testing and merging soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
My hardware has been slow to arrive. I opened a ticket with the vendor a day or two ago, so hopefully it'll not be too much longer. |
|
As I am still waiting for hardware, taking off draft, though would prefer it be tested on real hardware before being merged. |
shellcheck is a useful linter if a bit pedantic and overzealous so add overrides to silence false positives Also, fix issues found by the linting. * misspelling meant initscript could skip updating configuration in certain circumstances * minor: assignment of the result of execution as the time of creating local. This has been separated. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
This is a noop, but since we are here. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
Updated configuration was not being applied after config change. This was due to the means used to do the daemon reloads. Closes openwrt#28298 "Drivers not restarted on config change" Enable creating PID files for the server, driver, and monitor daemon processes. This allows to use NUT's built-in facilities for signalling the daemon's. For server, when reloading: 1. Check if upsd is running 1. If not, start it. 2. If it is send reload signal to upsd 2. For each driver: 1. Check if the driver is running 1. If it is, send reload-or-exit signal to driver 2. If driver is not running, start it 3. Attempt to start server (upsd and drivers) if service was stopped. For server, when stopping: 1. Check if upsd is running 1. If it is send stop signal to upsd 2. Ensure it really is stopped 2. For each driver: 1. Check if the driver is running 1. If it is, send stop signal to driver 2. If driver is still running, stop it. 3. If the server process is active (even with not upsd or drivers), stop it. For monitor, send the reload signal on config change, with fallback to stopping and starting the daemon. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
Change the names of variables and functions to make it more clear what is being acted on, configured, or otherwise touched. NUT is complicated and making it easier to follow the plot is important. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
Avoid attempting to remove a procd server instance that does not exist,
as doing so results in confusing/scary messages in syslog, such as:
Command failed: ubus call service delete { "name": "nut-server", "instance": "upsd" } (Not found)
Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
In NUT some models of UPS use shutdown_delay rather than offdelay, and yet others use usd for the same purpose. shutdown_delay and usd were previously not available in the list of available driver options, so add them. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
Ensure that when a ups is removed from the configuration that its driver instance is stopped. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
This is cosmetic, but user-facing (for users building via SDK or buildroot). Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
bdec640 to
9ae7b42
Compare
Attempt to de-mystify the nut-server initscript by adding comments and factoring out some common code that adds to complexity of the functions of which it is part. Signed-off-by: Daniel F. Dickinson <dfdpublic@wildtechgarden.ca>
|
Expect to have hardware to work with tomorrow evening, so should be able test with this weekend at the latest. |
|
Looks like buildbots are hitting missing files due to rc5 builds? |
|
Yeah, there's a sporadic failure to create the package index files in 25.12/snapshot right now: openwrt/openwrt#21986 |
📦 Package Details
Maintainer: @hnyman @s-hamann @drujd @tofurky @neheb @BKPepe @systemcrash @rpavlik
No maintainer, so spamming last 2.x years worth of folks who may be interested.
Description:
Updated configuration was not being applied after config change. This
was due to the means used to do the daemon reloads.
Closes #28298 "Drivers not restarted on config change"
Enable creating PID files for the server, driver, and monitor daemon
processes. This allows to use NUT's built-in facilities for signalling
the daemon's.
For server, when reloading:
For server, when stopping:
stop it.
For monitor, send the reload signal on config change, with fallback to
stopping and starting the daemon.
In the process added linting (ignoring false positives) and fixed whitespace inconsistency.
🧪 Run Testing Details
Only tested with dummy-ups so far. I have a spare UPS on order which should arrive this coming week.
Needs more testing and review.
✅ Formalities