Bug 14947 - samba-bgqd still notifying systemd, triggering log warnings without NotifyAccess=all
Summary: samba-bgqd still notifying systemd, triggering log warnings without NotifyAcc...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Printing (show other bugs)
Version: 4.15.3
Hardware: All Linux
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-01-17 01:39 UTC by Frank Dana
Modified: 2022-03-15 13:26 UTC (History)
2 users (show)

See Also:


Attachments
patch for 4.16 (1.20 KB, patch)
2022-01-28 10:13 UTC, Andreas Schneider
ab: review+
Details
patch for 4.15 (1.20 KB, patch)
2022-01-28 10:14 UTC, Andreas Schneider
ab: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Dana 2022-01-17 01:39:08 UTC
On Fedora 35 running Samba 4.15.3, it appears that /usr/libexec/samba/samba-bgqd still emits notifications to systemd, which since commit d1740fb3 are expected to come only from the main process.

This results in messages like this in the systemd journal:

systemd[1]: smb.service: Got notification message from PID 2162, but reception only permitted for main PID 1779

Where PID 1779 is the initial smbd process, and PID 2162 is a spawned instance of /usr/libexec/samba/samba-bgqd

Similar issues were already addressed for other services, such as samba-ad-dc, in this MR:

https://gitlab.com/samba-team/samba/-/merge_requests/1813

However, it doesn't appear that samba-bgqd was covered by those fixes.

Reinserting "NotifyAccess=all" into the smb.service systemd configuration is effective as a workaround, but obviously the intent with commit d1740fb3 was for that to no longer be required.
Comment 1 Alexander Bokovoy 2022-01-17 13:09:38 UTC
Thank you, Frank.

Could you please submit an MR?
Comment 2 Frank Dana 2022-01-22 21:33:45 UTC
(In reply to Alexander Bokovoy from comment #1)

I could submit an MR adding "NotifyAccess=all" back into the systemd unit file, but my impression is that's not how the Samba devs want to approach this. Like I said, its removal was a deliberate act. When other issues like this have cropped up, their response has been to correct the daemons involved so that they no longer notify systemd.

So, that's what I would characterize as the "correct" fix, as opposed to "NotifyAccess=All" which would be a workaround and a step backwards in terms of the Samba systemd configuration.

But beyond the high-level description of the fix I just gave, I'm afraid I have no idea how to actually fix this issue correctly... or even where exactly it would need to be fixed.
Comment 3 Alexander Bokovoy 2022-01-24 12:32:41 UTC
(In reply to Frank Dana from comment #2)
> (In reply to Alexander Bokovoy from comment #1)
> 
> I could submit an MR adding "NotifyAccess=all" back into the systemd unit
> file, but my impression is that's not how the Samba devs want to approach
> this. Like I said, its removal was a deliberate act. When other issues like
> this have cropped up, their response has been to correct the daemons
> involved so that they no longer notify systemd.

Sorry for being too concise in my request. ;) I asked to submit a change to samba-bgqd in the same way as https://gitlab.com/samba-team/samba/-/merge_requests/1813 did for others. E.g. in case we are running in the configuration that expects launching samba-bgqd off existing samba processes, add daemon_sd_notifications(false); to the samba-bgqd startup.
Comment 4 Frank Dana 2022-01-25 02:47:40 UTC
(In reply to Alexander Bokovoy from comment #3)

Ah, I see! That I can do, looks like the only thing that matters is that daemon_sd_notifications(false) get called before become_daemon(). Straightforward enough.

(The documentation comment for daemon_sd_notifications() is a bit odd:)

/**
 * @brief Enable or disable daemon status systemd notifications
 * 
 * When samba runs as AD DC only the main 'samba' process has to
 * notify systemd. Child processes started by the main 'samba', like
 * smbd and winbindd should call this function to disable sd_notify()
 * calls.
 * 
 * @param[in] enable True to enable notifications, false to disable
**/


Doesn't seem like Active Directory has anything to do with it, really.
Comment 5 Frank Dana 2022-01-25 09:36:21 UTC
Opened: https://gitlab.com/samba-team/samba/-/merge_requests/2346
Comment 6 Samba QA Contact 2022-01-27 10:54:04 UTC
This bug was referenced in samba master:

36c861e25b1d9c5ce44bfcb46247e7e4747930c5
Comment 7 Andreas Schneider 2022-01-28 10:13:03 UTC
Created attachment 17131 [details]
patch for 4.16
Comment 8 Andreas Schneider 2022-01-28 10:14:12 UTC
Created attachment 17132 [details]
patch for 4.15
Comment 9 Alexander Bokovoy 2022-01-28 10:29:14 UTC
Comment on attachment 17131 [details]
patch for 4.16

LGTM
Comment 10 Alexander Bokovoy 2022-01-28 10:29:27 UTC
Comment on attachment 17132 [details]
patch for 4.15

LGTM
Comment 11 Andreas Schneider 2022-01-28 13:10:09 UTC
Jule, could you please apply the patches to the corresponding branches? Thanks!
Comment 12 Jule Anger 2022-01-30 09:20:23 UTC
Pushed to autobuild-v4-{16,15}-test.
Comment 13 Samba QA Contact 2022-01-30 11:19:03 UTC
This bug was referenced in samba v4-15-test:

82799c1f86d966be47bc7de29e8c7f0cd574b7c9
Comment 14 Samba QA Contact 2022-01-30 11:55:12 UTC
This bug was referenced in samba v4-16-test:

c4132ef482bc2a3a20742cfdc5b524bedc8c445b
Comment 15 Jule Anger 2022-01-30 12:55:36 UTC
Closing out bug report.

Thanks!
Comment 16 Samba QA Contact 2022-01-31 17:25:31 UTC
This bug was referenced in samba v4-16-stable (Release samba-4.16.0rc2):

c4132ef482bc2a3a20742cfdc5b524bedc8c445b
Comment 17 Samba QA Contact 2022-03-15 13:26:20 UTC
This bug was referenced in samba v4-15-stable (Release samba-4.15.6):

82799c1f86d966be47bc7de29e8c7f0cd574b7c9