data/systemd: allow notifications for all snapd subprocesses #13797
Conversation
Allow all service's control group processes to send notifications via sd_notify. This is necesssary to prevent log flooding with systemd warnings like: Apr 05 14:36:55 qemuname systemd[1]: snapd.service: Got notification message from PID 1002, but reception only permitted for main PID 917 This warnings are happening due to a change in systemd 254 that makes all systemd programs (systemctl, udevadm, systemd-detect-virt, etc.) send EXIT_STATUS notifications when exiting. Fixes LP#2060310.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #13797 +/- ##
==========================================
- Coverage 78.86% 78.86% -0.01%
==========================================
Files 1043 1043
Lines 134595 134595
==========================================
- Hits 106144 106143 -1
Misses 21837 21837
- Partials 6614 6615 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -20,6 +20,7 @@ EnvironmentFile=-@SNAPD_ENVIRONMENT_FILE@ | |||
| Restart=always | |||
| WatchdogSec=5m | |||
| Type=notify | |||
| NotifyAccess=all | |||
There was a problem hiding this comment.
I think this is ok in principle. There's a separate question what this means whenever systemd receives event with EXIT_STATUS=xx from a PID that isn't the main one of the service, supposedly it's only informational.
Anyhow, if it's informational than maybe we should also report EXIT_STATUS in our tools we invoke from snapd, eg. snap-fde-keymgr, maybe snap-update-ns
There's even a funnier question, what if the process exits with non-0, but sends EXIT_STATUS=0 ? if we extend s-u-n then this could happen when s-u-n exits after finishing mount setup.
There was a problem hiding this comment.
Tbh I'm not sure of the use case of things like systemctl reportring EXIT_STATUS. https://github.com/systemd/systemd/blob/v254/NEWS#L649-L655 talks about containers, to collect info when the system shuts down, but still it is not clear to me how these notifications help. Maybe the last returned value is used in some cases as exit value of systemd, or that is the intention in the future.
Another option here is to unset the NOTIFY_SOCKET env var before calling the subprocesses to avoid these notifications.
There was a problem hiding this comment.
I think this looks fine, according to systemd docs around NotifyAccess
This option should be set to open access to the notification socket when using Type=notify/Type=notify-reload or WatchdogSec= (see above). If those options are used but NotifyAccess= is not configured, it will be implicitly set to main.
So it sounds like changing this to all when Type=notify sounds sensible.
There was a problem hiding this comment.
I like the idea of unsetting NOTIFY_SOCKET when calling commands, like systemctl or udevadm. I feel it should really not be defined.
Allow all service's control group processes to send notifications via sd_notify. This is necesssary to prevent log flooding with systemd warnings like:
Apr 05 14:36:55 qemuname systemd[1]: snapd.service: Got notification message from PID 1002, but reception only permitted for main PID 917
These warnings are happening due to a change in systemd 254 that makes all systemd programs (systemctl, udevadm, systemd-detect-virt, etc.) send EXIT_STATUS notifications when exiting.
Fixes LP#2060310.