systemd provides means to allow daemons to report their startup status properly up until they are fully able to serve clients. Currently smbd/winbindd/nmbd all returning control back to systemd before they even create pid file. Returning back before client could be served makes situation unpredictable for clustered environments. systemd API for daemons is very simple and Samba should use it. This bug serves as upstream anchor for the patch set to introduce use of sd_notify(3).
Created attachment 9803 [details] patch for samba 4.1
Created attachment 9804 [details] patch for samba git master Patch for git master
At http://copr.fedoraproject.org/coprs/abbra/samba-systemd-test/ one can find Fedora 20 packages built with the patch for 4.1 version. It properly reports smbd/nmbd/winbindd status to systemd upon startup, including issues that happen there.
Created attachment 9805 [details] patch for samba git master Fix Andreas name in the comments.
Created attachment 9806 [details] patch for samba 4.1 Fix Andreas name in the comments
Comment on attachment 9805 [details] patch for samba git master Can't we hide this behavior in lib/util/ ? I'd add a daemon_ready() function. There's no reason to include #include <systemd/sd-daemon.h> in a public header file. I also don't see the need for a separate wscript_configure_systemd file. Do we really need these new configure options?
Created attachment 9808 [details] patch for samba git master Updated patch following Metze's comments: - moved all sd_notify() logic to become_daemon.c - added simple wrapper daemon_ready() which takes daemon name as a parameter - moved waf checks into a single file We still need both --with-systemd and --without-systemd options as there are distributions which would like explicitly support only one case even if libraries are installed.
Created attachment 9809 [details] patch for samba 4.1 Updated version for v4-1-test
Created attachment 9810 [details] patch for samba git master One more update: forgot packaging/systemd/samba.service changes
Created attachment 9811 [details] patch for samba 4.1
Alexander please update the patch for 4.1 so the cherry pick sha1's are correct! $ git sh 54b5d9a7384ae27b2a26586ff909128427c05abe fatal: bad object 54b5d9a7384ae27b2a26586ff909128427c05abe
They're really nice (good job ! - even though I wanted to hate them because of systemd :-). Just one question. They change the error returns from the daemons from always (1) to a range of errno values. This seems to be to be the correct thing to do, but I still fear external dependencies on the error return... :-(. Alexander, persuade me it's ok, and I'll push the changes :-) :-).
(In reply to comment #12) > They're really nice (good job ! - even though I wanted > to hate them because of systemd :-). Just one question. > > They change the error returns from the daemons from > always (1) to a range of errno values. This seems to > be to be the correct thing to do, but I still fear > external dependencies on the error return... :-(. > > Alexander, persuade me it's ok, and I'll push > the changes :-) :-). Why would it be wrong ? You are returning valid error codes. Any script the improperly handles 0 being OK, and anything else being an error is bogus anyway. This way wrapper scripts or init systems get a chance of logging a meaningful error value.
I think Simo is right. Although we never defined or documented our exit behavior explicitly, a convention was that anything non-zero is error. I've checked some older documentation and init scripts and everywhere we have $RETVAL -eq 0 condition (or inverse of that).
Boo. Patch doesn't currently apply cleanly to master : Applying: add systemd integration error: patch failed: lib/util/wscript_build:10 error: lib/util/wscript_build: patch does not apply Patch failed at 0001 add systemd integration Sorry. Alexander, can you fix it up and I'll apply asap. Sorry for the delay that caused the patch to bit-rot :-(. Jeremy.
Created attachment 9866 [details] patch for samba git master
Fixed, it was just one line, extending list of library deps
(In reply to comment #16) > Created attachment 9866 [details] > patch for samba git master This patch already went in: Autobuild-Date(master): Wed Apr 23 04:44:46 CEST 2014 on sn-devel-104 Jeremy.
Thanks, Jeremy! Now it was me not noticing that you pushed it already :) Karolin, the patch for 4.1 is attached and can be merged to the 4.1 tree at your convenience.
(In reply to comment #19) > Thanks, Jeremy! Now it was me not noticing that you pushed it already :) > > Karolin, the patch for 4.1 is attached and can be merged to the 4.1 tree at > your convenience. Asking for second review flag.
Comment on attachment 9811 [details] patch for samba 4.1 LGTM
Pushed to autobuild-v4-1-test.
(In reply to comment #22) > Pushed to autobuild-v4-1-test. Pushed to v4-1-test. Closing out bug report. Thanks!