** Description changed: [Impact] The fix for bug #1882272, currently applied to Impish and Jammy via a debian sync, has a collateral effect that it may leave the fancontrol service started after a resume from suspend even if it was disabled and/or stopped. The fix wraps the restart call in a is-active check, and only issues the restart if the fancontrol service was running before (at suspend time). [Test Plan] The goal is to compare the state of the fancontrol service before and after suspend. It should be the same. There are two hardware requirements to fully test this SRU: - a machine capable of suspend/resume, like a laptop - a machine with hardware fan control compatible with fancontrol/lm-sensors A valid /etc/fancontrol configuration file must be generated, either manually or via pwmconfig(8). Without this file, the fancontrol service won't start. I didn't have any of those when preparing the fix, so I used a similarly configured systemd service to emulate what would happen (rsync in this case, which also depends on a config file to start). With the above setup, this is the test plan. For the following combinations, check that the desired result is achieved when resuming from suspend: At suspend time -> When resuming: Service is enabled and active --> is restarted Service is disabled and active --> is restarted Service is enabled and inactive --> is not restarted Service is disabled and inactive --> is not restarted [Where problems could occur] One could argue that this is changing behavior, but I think in the right direction. I might have missed some scenario where a restart of fancontrol is *always* desired, regardless of its previous state. If the script in /lib/systemd/system-sleep fails, there doesn't seem to be any impact on the resume event. An initial iteration of my fix was calling systemctl is-active incorrectly, and all that happened was a log entry about that. If the fancontrol daemon was started outside of systemd, then "systemctl is-active" won't see it, and it won't be restarted after resume. That is already an edge case without the is-active check, as "systemctl restart" might also fail to start fancontrol again if one copy is already running. [Other Info] + While testing the fix, I used rsync as the service because the hardware I have avaliable does not support fancontrol. - * Anything else you think is useful to include - * Anticipate questions from users, SRU, +1 maintenance, security teams and the Technical Board - * and address these questions in advance + I created /etc/rsyncd.conf and then went through the scenarios (started, + stopped, enabled, disabled) with a script in /lib/systemd/system- + sleep/rsync identical to the fancontrol one, except for the service + name. + + In by itself, this fix here might not warrant an update (or 0-day SRU) + for jammy, but focal and bionic don't have the original fix (to restart + fancontrol), and I wanted to SRU it there with both fixes: restart, and + only when it was active before. Hence I need the is-active fix in jammy + and impish, to be able to SRU focal and bionic. [Original Description] The fix for bug #1882272 issues a "systemctl restart fancontrol" when resuming from suspend. This ignores the fact that fancontrol could have been disabled, or could not even have been runing at suspend time. I suggest to wrap the restart with a "systemctl is-active fancontrol.service" call, and only issue the restart if fancontrol was already running when the machine was suspended.
** Description changed: [Impact] The fix for bug #1882272, currently applied to Impish and Jammy via a debian sync, has a collateral effect that it may leave the fancontrol service started after a resume from suspend even if it was disabled and/or stopped. The fix wraps the restart call in a is-active check, and only issues the restart if the fancontrol service was running before (at suspend time). [Test Plan] The goal is to compare the state of the fancontrol service before and after suspend. It should be the same. There are two hardware requirements to fully test this SRU: - a machine capable of suspend/resume, like a laptop - a machine with hardware fan control compatible with fancontrol/lm-sensors A valid /etc/fancontrol configuration file must be generated, either manually or via pwmconfig(8). Without this file, the fancontrol service won't start. I didn't have any of those when preparing the fix, so I used a similarly configured systemd service to emulate what would happen (rsync in this case, which also depends on a config file to start). With the above setup, this is the test plan. For the following combinations, check that the desired result is achieved when resuming from suspend: At suspend time -> When resuming: Service is enabled and active --> is restarted Service is disabled and active --> is restarted Service is enabled and inactive --> is not restarted Service is disabled and inactive --> is not restarted [Where problems could occur] One could argue that this is changing behavior, but I think in the right direction. I might have missed some scenario where a restart of fancontrol is *always* desired, regardless of its previous state. If the script in /lib/systemd/system-sleep fails, there doesn't seem to be any impact on the resume event. An initial iteration of my fix was calling systemctl is-active incorrectly, and all that happened was a log entry about that. If the fancontrol daemon was started outside of systemd, then "systemctl is-active" won't see it, and it won't be restarted after resume. That is already an edge case without the is-active check, as "systemctl restart" might also fail to start fancontrol again if one copy is already running. [Other Info] While testing the fix, I used rsync as the service because the hardware I have avaliable does not support fancontrol. I created /etc/rsyncd.conf and then went through the scenarios (started, stopped, enabled, disabled) with a script in /lib/systemd/system- sleep/rsync identical to the fancontrol one, except for the service name. - In by itself, this fix here might not warrant an update (or 0-day SRU) - for jammy, but focal and bionic don't have the original fix (to restart + In by itself, this fix here might not warrant an update for jammy or + impish, but focal and bionic don't yet have the original fix (to restart fancontrol), and I wanted to SRU it there with both fixes: restart, and only when it was active before. Hence I need the is-active fix in jammy and impish, to be able to SRU focal and bionic. [Original Description] The fix for bug #1882272 issues a "systemctl restart fancontrol" when resuming from suspend. This ignores the fact that fancontrol could have been disabled, or could not even have been runing at suspend time. I suggest to wrap the restart with a "systemctl is-active fancontrol.service" call, and only issue the restart if fancontrol was already running when the machine was suspended. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1967432 Title: After resume from suspend, fancontrol can be running even if it was disabled before To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/lm-sensors/+bug/1967432/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
