Bug 719419 - Provide native systemd unit file
Provide native systemd unit file
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: bind (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Adam Tkac
Fedora Extras Quality Assurance
:
: 714436 (view as bug list)
Depends On:
Blocks: SysVtoSystemd 751869
  Show dependency treegraph
 
Reported: 2011-07-06 14:43 EDT by Jóhann B. Guðmundsson
Modified: 2015-01-06 14:04 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-01-30 11:02:26 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Native systemd service file named (614 bytes, text/plain)
2011-07-06 14:49 EDT, Jóhann B. Guðmundsson
no flags Details
New native systemd service file for named (576 bytes, text/plain)
2012-01-23 10:17 EST, Jóhann B. Guðmundsson
no flags Details
New native systemd service file for named (567 bytes, text/plain)
2012-01-23 11:13 EST, Jóhann B. Guðmundsson
no flags Details
Native systemd service file for named (532 bytes, text/plain)
2012-01-23 11:37 EST, Jóhann B. Guðmundsson
no flags Details
Native systemd service file for named (591 bytes, text/plain)
2012-01-24 11:35 EST, Jóhann B. Guðmundsson
no flags Details

  None (edit)
Description Jóhann B. Guðmundsson 2011-07-06 14:43:20 EDT
Description of problem:

https://fedoraproject.org/wiki/Features/SysVtoSystemd


Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:
Comment 1 Jóhann B. Guðmundsson 2011-07-06 14:49:47 EDT
Created attachment 511556 [details]
Native systemd service file named

Let's start with getting a correct non chrooted service once that is achieved we can take a look at http://0pointer.de/blog/projects/changing-roots.html and create another ( named-chroot.service )
Comment 2 Jóhann B. Guðmundsson 2011-11-16 11:57:08 EST
What's the current status on this?
Comment 3 Markus Mayer 2012-01-12 08:45:35 EST
*** Bug 714436 has been marked as a duplicate of this bug. ***
Comment 4 Michal Schmidt 2012-01-23 06:48:24 EST
The named initscript has an LSB header with:
# Provides: $named

This translates into systemd as:
Wants=nss-lookup.target
Before=nss-lookup.target

nss-lookup.target is described in 'man systemd.special'.

Please make sure this is preserved in the transition to the native unit.
Comment 5 Jóhann B. Guðmundsson 2012-01-23 07:43:24 EST
As you know we have not properly mapped out how we are going to use/take advantages of targets in general and units something we need to discuss and get cleared out.

Thus arguably the wants section should be dropped and all name servers should
be using nss-lookup.target in their [Install] section as opposed to be using
the multi-user.target.
Comment 6 Jóhann B. Guðmundsson 2012-01-23 07:45:40 EST
+ I need to rewrite the submitted unit to reflect the current systemd in rawhide
Comment 7 Jóhann B. Guðmundsson 2012-01-23 10:17:16 EST
Created attachment 556985 [details]
New native systemd service file for named

Added to the [Unit] section
Wants=nss-lookup.target
Before=nss-lookup.target

Removed from the [Unit] section 
syslog.target from the After= line ( no longer necessary )

Added to the [Service] section
PrivateTmp=yes ( I do believe that bind was on Dan's feature proposal just remove this line if it's not necessary )

Updated the unit to match /run and /usr proposals 

Removed environments references ( /etc/sysconfig/foo files are Fedora/Red Hat spesific ) since it wont fly with upstream and it's no longer necessary since users/administrators should copy the unit /etc/systemd/system directory and add any addtional option to the startup of the daemon there or create a new unit in that directory bearing the same name with .include reference and any addition options they want to add.
Comment 8 Jóhann B. Guðmundsson 2012-01-23 10:19:05 EST
I recommend using a separated chroot service named named-chroot.service that gets shipped with the bind-chroot package
Comment 9 Jóhann B. Guðmundsson 2012-01-23 11:13:31 EST
Created attachment 557005 [details]
New native systemd service file for named

Dropping user and group from named and starting the daemon instead with -u named due to permission problems during startup as pointed out by Michal
Comment 10 Jóhann B. Guðmundsson 2012-01-23 11:37:21 EST
Created attachment 557011 [details]
Native systemd service file for named

Dropping ExecReload=/bin/kill -HUP $MAINPID line from the unit as discussed on irc probably not necessary anymore.
Comment 11 Adam Tkac 2012-01-24 06:28:50 EST
(In reply to comment #10)
> Created attachment 557011 [details]
> Native systemd service file for named

Is the "Wants=nss-lookup.target" directive necessary?

If I understand correctly description of the nss-lookup.target in systemd.special(7) manpage, the nss-lookup.target stands for "DNS is ready" so only the "Before=nss-lookup.target" is needed, in my opinion, isn't it?

> Dropping ExecReload=/bin/kill -HUP $MAINPID line from the unit as discussed on
> irc probably not necessary anymore.

Usage of rndc in the ExecReload and ExecStop directives is tricky because some servers are configured not to accept rndc commands. The best would be to use rndc and if it returns non-zero exit code then use `kill -HUP` / `kill -TERM`. Do you know if such conditional execution of Exec* directives is supported in current systemd?
Comment 12 Michal Schmidt 2012-01-24 06:41:26 EST
(In reply to comment #11)
> Is the "Wants=nss-lookup.target" directive necessary?

Yes, it is.

> If I understand correctly description of the nss-lookup.target in
> systemd.special(7) manpage, the nss-lookup.target stands for "DNS is ready" so
> only the "Before=nss-lookup.target" is needed, in my opinion, isn't it?

That would not be sufficient because then nothing would pull nss-lookup.target into the boot transaction. All the {Before,After}=nss-lookup.target dependencies would then have no effect whatsoever.

> > Dropping ExecReload=/bin/kill -HUP $MAINPID line from the unit as discussed on
> > irc probably not necessary anymore.
> 
> Usage of rndc in the ExecReload and ExecStop directives is tricky because some
> servers are configured not to accept rndc commands.

Is it a fully supported configuration?
Can't we just declare that for the reload action to work the configuration needs to allow rndc?

> The best would be to use
> rndc and if it returns non-zero exit code then use `kill -HUP` / `kill -TERM`.
> Do you know if such conditional execution of Exec* directives is supported in
> current systemd?

You cannot do that just with systemd directives. You'd have to call an external shell script in ExecReload or if it's a one-liner, this might me sufficient:
ExecReload=/bin/sh -c '...'
Comment 13 Jóhann B. Guðmundsson 2012-01-24 06:47:46 EST
(In reply to comment #12)
> (In reply to comment #11) 
> You cannot do that just with systemd directives. You'd have to call an external
> shell script in ExecReload or if it's a one-liner, this might me sufficient:
> ExecReload=/bin/sh -c '...'

Let's try to avoid calling shell script hacks shall we. 

If an administrator does not want to use the set defaults and disables rndc he should also copy the unit file to /etc/systemd/system directory and remove those lines there.
Comment 14 Jóhann B. Guðmundsson 2012-01-24 06:51:43 EST
Arguably named should be patched to run "rndc" as a separate daemon then rndc parts of the unit could be moved into it's own unit which administrators could simply choose to enable/disable at will.
Comment 15 Michal Schmidt 2012-01-24 07:13:26 EST
(In reply to comment #13)
> Let's try to avoid calling shell script hacks shall we.

If it's the only way to achieve correctness (I'm not necessarily saying it is), I don't mind using the shell. In ExecReload, which should be a rare operation, there are no performance concerns either.

(In reply to comment #14)
> Arguably named should be patched to run "rndc" as a separate daemon

I believe there is no daemon needed for rndc to work. bind just needs to be configured to listen on a special TCP socket on loopback.
Comment 16 Jóhann B. Guðmundsson 2012-01-24 07:42:11 EST
Well you dont have to have run it as a daemon. 

Can we strip out the rndc part from the unit file and add an unit file for rndc 

If doable would look something like this...

### rncd.service ###

[Unit]
Description=Remote Name Daemon Control (RNDC)
After=network.target named.service

[Service]
ExecStart=/usr/sbin/rndc -c /etc/rndc.conf start
ExecReload=/usr/sbin/rndc reload
ExecStop=/usr/sbin/rndc stop

[Install]
WantedBy=multi-user.target


And administrators would just run if they choose to use rndc 

#rndc-confgen > /etc/rndc.conf 
#systemctl enable rndc.service
#systemctl start rndc.service 

And the named.service would add the new reload option to the unit which I cant recall what is called at the moment.

Adam is that something that can be considered and or would fly by upstream?
Comment 17 Michal Schmidt 2012-01-24 07:55:37 EST
I'd find that more confusing than helpful. rndc is just a client that talks to the bind daemon. I see no gain in wrapping it with a unit of its own.
Comment 18 Adam Tkac 2012-01-24 07:56:37 EST
(In reply to comment #12)
> (In reply to comment #11)
> > Is the "Wants=nss-lookup.target" directive necessary?
> 
> Yes, it is.
> 
> > If I understand correctly description of the nss-lookup.target in
> > systemd.special(7) manpage, the nss-lookup.target stands for "DNS is ready" so
> > only the "Before=nss-lookup.target" is needed, in my opinion, isn't it?
> 
> That would not be sufficient because then nothing would pull nss-lookup.target
> into the boot transaction. All the {Before,After}=nss-lookup.target
> dependencies would then have no effect whatsoever.

Understood, thanks for explanation.

> > > Dropping ExecReload=/bin/kill -HUP $MAINPID line from the unit as discussed on
> > > irc probably not necessary anymore.
> > 
> > Usage of rndc in the ExecReload and ExecStop directives is tricky because some
> > servers are configured not to accept rndc commands.
> 
> Is it a fully supported configuration?
> Can't we just declare that for the reload action to work the configuration
> needs to allow rndc?

Non-working rndc is supported configuration (but not recommended) and I know some users prefer not to use rndc for various reasons. I prefer to use simple script for reload/stop named service. Since named is rarely stopped/reloaded by initscript, this shouldn't be a problem.

> > The best would be to use
> > rndc and if it returns non-zero exit code then use `kill -HUP` / `kill -TERM`.
> > Do you know if such conditional execution of Exec* directives is supported in
> > current systemd?
> 
> You cannot do that just with systemd directives. You'd have to call an external
> shell script in ExecReload or if it's a one-liner, this might me sufficient:
> ExecReload=/bin/sh -c '...'
Comment 19 Jóhann B. Guðmundsson 2012-01-24 08:06:39 EST
(In reply to comment #17)
> I'd find that more confusing than helpful. rndc is just a client that talks to
> the bind daemon. I see no gain in wrapping it with a unit of its own.

I dont disagree with you here. 

What strikes me as odd is why rndc reload and stop needs to be called in the first place. 

From my pov the daemon should handle that if it's not some case of lazy admin workaround
Comment 20 Michal Schmidt 2012-01-24 08:20:36 EST
(In reply to comment #19)
> What strikes me as odd is why rndc reload and stop needs to be called in the
> first place. 

I can imagine "rndc reload" being preferred to a SIGHUP. For example it may be implemented as a synchronous operation (I have no idea if that's the case, but in theory it can be), which is generally a nice property for the reload action.

I don't know what the advantage of "rndc stop" to the default killing by SIGTERM could be.
Adam, could you explain?
Comment 21 Adam Tkac 2012-01-24 10:16:25 EST
(In reply to comment #20)
> (In reply to comment #19)
> > What strikes me as odd is why rndc reload and stop needs to be called in the
> > first place. 
> 
> I can imagine "rndc reload" being preferred to a SIGHUP. For example it may be
> implemented as a synchronous operation (I have no idea if that's the case, but
> in theory it can be), which is generally a nice property for the reload action.

The main advantage of `rndc reload` over SIGHUP is that `rndc reload` returns non-zero exit code when reload fails (for example due to incorrectly modified configuration). In this case admin can immediately see error:

[root@rawhide ~]# systemctl reload named.service
Job failed. See system logs and 'systemctl status' for details.

When I used `kill -HUP`, there is only error message in /var/log/messages which can be easily overlooked.

> I don't know what the advantage of "rndc stop" to the default killing by
> SIGTERM could be.
> Adam, could you explain?

`rndc stop` behaves differently from `kill -TERM`. When you send SIGTERM to named, it immediately exits. When it receives `rndc stop` command, it saves pending updates (both DDNS updates and IXFR zone transfers) to zone files.

I think we should use the wrapper script for rndc. Otherwise admins (not sure how many) familiar with behavior of current initscript will be confused.
Comment 22 Michal Schmidt 2012-01-24 10:57:16 EST
(In reply to comment #21)
> The main advantage of `rndc reload` over SIGHUP is that `rndc reload` returns
> non-zero exit code when reload fails (for example due to incorrectly modified
> configuration). In this case admin can immediately see error:
> 
> [root@rawhide ~]# systemctl reload named.service
> Job failed. See system logs and 'systemctl status' for details.

Yes, this is a good thing.

> > I don't know what the advantage of "rndc stop" to the default killing by
> > SIGTERM could be.
> > Adam, could you explain?
> 
> `rndc stop` behaves differently from `kill -TERM`. When you send SIGTERM to
> named, it immediately exits. When it receives `rndc stop` command, it saves
> pending updates (both DDNS updates and IXFR zone transfers) to zone files.

OK, that explains it.

> I think we should use the wrapper script for rndc. Otherwise admins (not sure
> how many) familiar with behavior of current initscript will be confused.

You mean using something like this?:

ExecReload=/bin/sh -c '/usr/sbin/rndc reload >/dev/null 2>&1 || /usr/bin/kill -HUP $MAINPID'
Comment 23 Adam Tkac 2012-01-24 11:07:05 EST
(In reply to comment #22)
> > I think we should use the wrapper script for rndc. Otherwise admins (not sure
> > how many) familiar with behavior of current initscript will be confused.
> 
> You mean using something like this?:
> 
> ExecReload=/bin/sh -c '/usr/sbin/rndc reload >/dev/null 2>&1 || /usr/bin/kill
> -HUP $MAINPID'

Yes, this seems like the best for me.
Comment 24 Jóhann B. Guðmundsson 2012-01-24 11:35:01 EST
Created attachment 557265 [details]
Native systemd service file for named

With discussed changes
Comment 25 Jóhann B. Guðmundsson 2012-01-24 11:54:04 EST
So what's remaining here chroot?
Comment 26 Adam Tkac 2012-01-25 06:19:31 EST
(In reply to comment #24)
> Created attachment 557265 [details]
> Native systemd service file for named
> 
> With discussed changes

I improved the service file a little and this is the final version:

[Unit]
Description=Berkeley Internet Name Domain (DNS)
Wants=nss-lookup.target
Before=nss-lookup.target
After=network.target

[Service]
Type=forking
EnvironmentFile=-/etc/sysconfig/named
Environment=KRB5_KTNAME=/etc/named.keytab
PIDFile=/var/run/named/named.pid
ExecStartPre=/usr/sbin/named-checkconf -z /etc/named.conf
ExecStart=/usr/sbin/named -u named $OPTIONS
ExecReload=/bin/sh -c '/usr/sbin/rndc reload > /dev/null 2>&1 || /bin/kill -HUP $MAINPID'
ExecStop=/bin/sh -c '/usr/sbin/rndc stop > /dev/null 2>&1 || /bin/kill -TERM $MAINPID'
PrivateTmp=true
TimeoutSec=25

[Install]
WantedBy=multi-user.target

Are you OK with it?
Comment 27 Adam Tkac 2012-01-25 06:23:04 EST
Btw after we finish named.service then we must also create named-chroot.service, named-sdb.service and named-sdb-chroot.service. However this will be only slightly modified named.service.
Comment 28 Jóhann B. Guðmundsson 2012-01-25 06:32:21 EST
(In reply to comment #26)
> (In reply to comment #24)
> 
> Are you OK with it?

I personally would like this to be dropped EnvironmentFile=-/etc/sysconfig/named along with the $OPTION since it's no longer needed. 

User/Administrators really should get used to copy the unit to /etc/systemd/system directory and add the additional daemon startup option there which is the proper thing to do when daemons dont support reading configuration files on startup.

This also means that the unit is not cross distributable and thus wont fly with upstream. 

If necessary this should be in a patch against the unit. ( granted that this unit will be accepted by upstream ).

PID path should be /run not /var/run these days...

Other then the above it looks good.
Comment 29 Michal Schmidt 2012-01-25 06:51:32 EST
(In reply to comment #26)
> ExecStop=/bin/sh -c '/usr/sbin/rndc stop > /dev/null 2>&1 || /bin/kill -TERM
> $MAINPID'

It should not be necessary to explicitly send a SIGTERM when rndc fails, because systemd will send it by itself (and after some timeout, SIGKILL) if there are any processes in the cgroup still alive after the ExecStop command finishes.
Comment 30 Jóhann B. Guðmundsson 2012-01-25 07:01:59 EST
(In reply to comment #27)
> Btw after we finish named.service then we must also create
> named-chroot.service, named-sdb.service and named-sdb-chroot.service. However
> this will be only slightly modified named.service.

The *_chroot_conf () probably needs to be wrapped in a script 

Then I guess something along the lines of...


[Unit]
Description=Berkeley Internet Name Domain (DNS)
Wants=nss-lookup.target
Before=nss-lookup.target
After=network.target

[Service]
Type=forking
RootDirectory=/var/named/chroot
Environment=KRB5_KTNAME=/etc/named.keytab
PIDFile=/run/named/named.pid
ExecStartPre=/usr/bin/setup-named-chroot.sh
ExecStartPre=/usr/sbin/named-checkconf -z /etc/named.conf
ExecStart=/usr/sbin/named -u named 
ExecReload=/bin/sh -c '/usr/sbin/rndc reload > /dev/null 2>&1 || /bin/kill -HUP
$MAINPID'
ExecStop=/usr/sbin/rndc stop
RootDirectoryStartOnly=yes
PrivateTmp=true
TimeoutSec=25

[Install]
WantedBy=multi-user.target

It's a question if we should create a named-chroot-setup.service type oneshot
service with conditions that setups the chroot environment prior to starting
the service ( would be added to Wants= in the named-chroots.service ).
Comment 31 Adam Tkac 2012-01-26 06:56:05 EST
(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #24)
> > 
> > Are you OK with it?
> 
> I personally would like this to be dropped
> EnvironmentFile=-/etc/sysconfig/named along with the $OPTION since it's no
> longer needed. 

Many users have OPTIONS='-4' set in their /etc/sysconfig/named files so ignoring it can be seen as a regression.

> User/Administrators really should get used to copy the unit to
> /etc/systemd/system directory and add the additional daemon startup option
> there which is the proper thing to do when daemons dont support reading
> configuration files on startup.
> 
> This also means that the unit is not cross distributable and thus wont fly with
> upstream. 
>
> If necessary this should be in a patch against the unit. ( granted that this
> unit will be accepted by upstream ).

This sounds fine for me. When upstream accepts unit file without EnvironmentFile opt, I will use the patch. For now unit file will be kept in git as installed.

> PID path should be /run not /var/run these days...

Right you are, I will modify this.
Comment 32 Adam Tkac 2012-01-26 08:14:47 EST
(In reply to comment #29)
> (In reply to comment #26)
> > ExecStop=/bin/sh -c '/usr/sbin/rndc stop > /dev/null 2>&1 || /bin/kill -TERM
> > $MAINPID'
> 
> It should not be necessary to explicitly send a SIGTERM when rndc fails,
> because systemd will send it by itself (and after some timeout, SIGKILL) if
> there are any processes in the cgroup still alive after the ExecStop command
> finishes.

Although you are right, without the `kill -TERM` system log contains:

...
Jan 26 14:07:22 rawhide named[1552]: running
Jan 26 14:07:26 rawhide rndc[1558]: rndc: neither /etc/rndc.conf nor /etc/rndc.key was found
Jan 26 14:07:26 rawhide systemd[1]: named.service: control process exited, code=exited status=1
Jan 26 14:07:26 rawhide systemd[1]: Unit named.service entered failed state.
Jan 26 14:07:26 rawhide named[1552]: shutting down
...

However with the `kill -TERM` log looks nicer:

...
Jan 26 14:11:36 rawhide named[1832]: running
Jan 26 14:11:43 rawhide named[1832]: shutting down
...

So I will keep the `kill -TERM` in the unit file.
Comment 33 Michal Schmidt 2012-01-26 08:48:54 EST
(In reply to comment #32)
> Jan 26 14:07:26 rawhide systemd[1]: named.service: control process exited,
> code=exited status=1
> Jan 26 14:07:26 rawhide systemd[1]: Unit named.service entered failed state.

FYI, if you want systemd to ignore the non-zero exit status of an Exec* command, you can prefix it with a dash:
ExecStop=-/usr/sbin/rndc stop
Comment 34 Adam Tkac 2012-01-30 11:02:26 EST
bind-9.9.0-0.6.rc1.fc17 is "systemd-ready", closing.

Note You need to log in before you can comment on or make changes to this bug.