Bug 10517 - Samba servers should integrate with systemd
Summary: Samba servers should integrate with systemd
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: 4.1.6
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-26 13:27 UTC by Alexander Bokovoy
Modified: 2014-05-27 19:14 UTC (History)
2 users (show)

See Also:


Attachments
patch for samba 4.1 (24.55 KB, patch)
2014-03-26 13:37 UTC, Alexander Bokovoy
no flags Details
patch for samba git master (24.58 KB, patch)
2014-03-26 13:41 UTC, Alexander Bokovoy
no flags Details
patch for samba git master (24.59 KB, patch)
2014-03-26 14:35 UTC, Alexander Bokovoy
metze: review-
Details
patch for samba 4.1 (24.56 KB, patch)
2014-03-26 14:36 UTC, Alexander Bokovoy
no flags Details
patch for samba git master (23.68 KB, patch)
2014-03-27 09:11 UTC, Alexander Bokovoy
no flags Details
patch for samba 4.1 (24.07 KB, patch)
2014-03-27 09:11 UTC, Alexander Bokovoy
no flags Details
patch for samba git master (24.12 KB, patch)
2014-03-27 10:25 UTC, Alexander Bokovoy
no flags Details
patch for samba 4.1 (24.51 KB, patch)
2014-03-27 10:25 UTC, Alexander Bokovoy
kseeger: review? (jra)
asn: review+
Details
patch for samba git master (24.06 KB, patch)
2014-04-22 19:01 UTC, Alexander Bokovoy
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Bokovoy 2014-03-26 13:27:45 UTC
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).
Comment 1 Alexander Bokovoy 2014-03-26 13:37:28 UTC
Created attachment 9803 [details]
patch for samba 4.1
Comment 2 Alexander Bokovoy 2014-03-26 13:41:07 UTC
Created attachment 9804 [details]
patch for samba git master

Patch for git master
Comment 3 Alexander Bokovoy 2014-03-26 13:42:39 UTC
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.
Comment 4 Alexander Bokovoy 2014-03-26 14:35:45 UTC
Created attachment 9805 [details]
patch for samba git master

Fix Andreas name in the comments.
Comment 5 Alexander Bokovoy 2014-03-26 14:36:16 UTC
Created attachment 9806 [details]
patch for samba 4.1

Fix Andreas name in the comments
Comment 6 Stefan Metzmacher 2014-03-26 19:24:37 UTC
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?
Comment 7 Alexander Bokovoy 2014-03-27 09:11:01 UTC
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.
Comment 8 Alexander Bokovoy 2014-03-27 09:11:40 UTC
Created attachment 9809 [details]
patch for samba 4.1

Updated version for v4-1-test
Comment 9 Alexander Bokovoy 2014-03-27 10:25:33 UTC
Created attachment 9810 [details]
patch for samba git master

One more update: forgot packaging/systemd/samba.service changes
Comment 10 Alexander Bokovoy 2014-03-27 10:25:51 UTC
Created attachment 9811 [details]
patch for samba 4.1
Comment 11 Andreas Schneider 2014-04-02 05:49:09 UTC
Alexander please update the patch for 4.1 so the cherry pick sha1's are correct!

$ git sh 54b5d9a7384ae27b2a26586ff909128427c05abe
fatal: bad object 54b5d9a7384ae27b2a26586ff909128427c05abe
Comment 12 Jeremy Allison 2014-04-21 23:04:00 UTC
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 :-) :-).
Comment 13 Simo Sorce 2014-04-21 23:36:56 UTC
(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.
Comment 14 Alexander Bokovoy 2014-04-22 06:08:13 UTC
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).
Comment 15 Jeremy Allison 2014-04-22 17:51:16 UTC
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.
Comment 16 Alexander Bokovoy 2014-04-22 19:01:21 UTC
Created attachment 9866 [details]
patch for samba git master
Comment 17 Alexander Bokovoy 2014-04-22 19:02:26 UTC
Fixed, it was just one line, extending list of library deps
Comment 18 Jeremy Allison 2014-05-06 18:06:16 UTC
(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.
Comment 19 Alexander Bokovoy 2014-05-07 02:19:23 UTC
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.
Comment 20 Karolin Seeger 2014-05-19 10:16:03 UTC
(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 21 Andreas Schneider 2014-05-20 09:46:18 UTC
Comment on attachment 9811 [details]
patch for samba 4.1

LGTM
Comment 22 Karolin Seeger 2014-05-20 09:48:07 UTC
Pushed to autobuild-v4-1-test.
Comment 23 Karolin Seeger 2014-05-27 19:13:28 UTC
(In reply to comment #22)
> Pushed to autobuild-v4-1-test.

Pushed to v4-1-test.
Closing out bug report.

Thanks!