Bug 754421 - Provide native systemd service
Provide native systemd service
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: autofs (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Ian Kent
Fedora Extras Quality Assurance
:
: 761330 (view as bug list)
Depends On:
Blocks: 751869
  Show dependency treegraph
 
Reported: 2011-11-16 07:53 EST by Jóhann B. Guðmundsson
Modified: 2012-04-18 15:34 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-04-18 15:34:22 EDT
Type: Bug
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 unit for autofs (281 bytes, text/plain)
2011-11-16 07:55 EST, Jóhann B. Guðmundsson
no flags Details
Patch - add piddir to configure (4.47 KB, patch)
2011-11-18 11:33 EST, Ian Kent
no flags Details | Diff
Patch - add systemd unit support (11.30 KB, patch)
2011-11-18 11:35 EST, Ian Kent
no flags Details | Diff
Patch - remove empty command line arguments (passed by systemd). (2.98 KB, patch)
2011-12-09 02:13 EST, Ian Kent
no flags Details | Diff
Patch - systemd support fixes (3.45 KB, patch)
2012-02-23 21:40 EST, Ian Kent
no flags Details | Diff

  None (edit)
Description Jóhann B. Guðmundsson 2011-11-16 07:53:01 EST
Description of problem:

Let's get the ball rolling on this one...

http://fedoraproject.org/wiki/Features/SysVtoSystemd
https://fedoraproject.org/wiki/Packaging:Guidelines:Systemd
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

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-11-16 07:55:25 EST
Created attachment 534010 [details]
Native systemd unit for autofs
Comment 2 Ian Kent 2011-11-16 08:40:06 EST
Judging by our recent discussion this unit file should be
sufficient, at least to start with, agreed?

Where should this file be installed so that it will get
picked up by systemd?
Comment 3 Jóhann B. Guðmundsson 2011-11-16 09:05:19 EST
See the links provided in comment 1 

Or take a short cut and look at this =)

http://pkgs.fedoraproject.org/gitweb/?p=freeradius.git;a=commitdiff;h=a755be0e23bcb72b356b8f9431d16404481e9075
Comment 4 Jóhann B. Guðmundsson 2011-11-16 09:07:59 EST
Uhum and this ( seems to a fix to the previous radius spec ) 

http://pkgs.fedoraproject.org/gitweb/?p=freeradius.git;a=commitdiff;h=ae46b032fd8c9958ff95b495ce114f57636e42ca
Comment 5 Ian Kent 2011-11-16 23:27:00 EST
There are a couple of problems with autofs that need to be
addressed.

First there are two custom init script actions that need to
be provided. Will systemd will delegate these to calling the
init script or do I need to do something extra?

There is the restart issue. It's not usually a problem for
Rawhide and Fedora users or even most RHEL users but it is
a problem. When a signal is sent to the daemon to tell it
to expire all mounts and exit it can still be shutting down
when the init script has waited its allotted time. The
problem is that if there are many mounts and servers are
not responding the shutdown can take a long time but this
shouldn't prevent the system shutdown so we can't wait
forever. Clearly if this situation arises during a restart
the restart won't work properly.

It isn't worth holding up this change but I need to fix it.
Comment 6 Jóhann B. Guðmundsson 2011-11-17 04:35:19 EST
(In reply to comment #5)
> There are a couple of problems with autofs that need to be
> addressed.
> 
> First there are two custom init script actions that need to
> be provided. Will systemd will delegate these to calling the
> init script or do I need to do something extra?

Depends on what actually needs to be done as in what those custom init script actions are and what they are doing or trying to solve.


> There is the restart issue. It's not usually a problem for
> Rawhide and Fedora users or even most RHEL users but it is
> a problem. When a signal is sent to the daemon to tell it
> to expire all mounts and exit it can still be shutting down
> when the init script has waited its allotted time. The
> problem is that if there are many mounts and servers are
> not responding the shutdown can take a long time but this
> shouldn't prevent the system shutdown so we can't wait
> forever. Clearly if this situation arises during a restart
> the restart won't work properly.
> 
> It isn't worth holding up this change but I need to fix it.

Hum...

We can increase the time out for the unit a in increase TimeoutSec= if necessary ( defaults to 90s see man systemd.service for details ) 

I'm not sure if you have looked into it ( I know I have not ) but I have been wondering a bit if autofs could take advantage of the mount and automount capabilities systemd has to offer ( see man systemd.mount and man systemd.automount for those. You might also want to take a look at man systemd.path ) or be extended to do so ( or systemd be extended to cover any short comings it might have in that area ).
Comment 7 Ian Kent 2011-11-18 11:33:32 EST
Created attachment 534434 [details]
Patch - add piddir to configure
Comment 8 Ian Kent 2011-11-18 11:35:56 EST
Created attachment 534436 [details]
Patch - add systemd unit support

First attempt to add systemd support to autofs.
The changes to the package spec file will be the same as the
changes I will make to the Rawhide spec file.

Comments please.
Comment 9 Jóhann B. Guðmundsson 2011-11-21 11:59:32 EST
(In reply to comment #8)
> Created attachment 534436 [details]
> Patch - add systemd unit support
> 
> First attempt to add systemd support to autofs.
> The changes to the package spec file will be the same as the
> changes I will make to the Rawhide spec file.
> 
> Comments please.

Given that I'm not a proven package ( nor a packager in general ) there is very little comment I can add so just go ahead build it if I find the time I can test it. 

We are doing this that early in the development cycle we should be able to catch any issues relatively fast.
Comment 10 Ian Kent 2011-12-08 09:41:42 EST
I've discovered a problem with the argument passing in the
unit file.

autofs in Rawhide is currently broken.
I will be posting an update tomorrow or perhaps over the
weekend.
Comment 11 Ian Kent 2011-12-08 09:44:03 EST
*** Bug 761330 has been marked as a duplicate of this bug. ***
Comment 12 Jóhann B. Guðmundsson 2011-12-08 12:08:28 EST
Hum...

${OPTIONS} needing to be $OPTIONS ?

Well we really should drop the /etc/sysconfig/autofs support in the unit file and have it look something like this...

[Unit]
Description=Automounts filesystems on demand
After=network.target ypbind.service

[Service]
Type=forking
PIDFile=/run/autofs.pid
ExecStart=/usr/sbin/automount -p /run/autofs.pid

[Install]
WantedBy=multi-user.target

Since admins/power users alike should get used to the idea to copy their unit files to /etc/systemd/system directory and add any options to it there... 

Btw what where those custom init script action you mention in comment 5?
Comment 13 Ian Kent 2011-12-08 20:14:36 EST
(In reply to comment #12)
> Hum...
> 
> ${OPTIONS} needing to be $OPTIONS ?

Don't think so, they are basically equivalent, the curly
brackets should only make a difference when parsing the
macro name can't be done and needs an explicit terminator,
such as when embedded within some string, like a file name.

I always include a problem description when I post patches
so lets wait until then. The patch will have sufficient
information for systemd maintainers to understand the
problem and they can choose to change or not. In any case
autofs isn't going to be constrained by it.

> 
> Well we really should drop the /etc/sysconfig/autofs support in the unit file
> and have it look something like this...

Perhaps but that might not be the best thing to do in this
case.

I think it is better to have a place outside of the unit file
for this because it is better for the admin to not have to
change these files and so not risk messing them up. Especially
so since there are a couple of commonly (and frequently) used
options that are site dependent. Bottom line is that it is
much simpler to "sed" edit a single file than to copy and
then "sed" edit a file when you need to do this on a large
number of machines. But also, when you have a large number
of machines, you always want to keep the amount of change
to the base install to an absolute minimum from a maintenance
POV.

Clearly, on a site with a dozen or so machines it isn't
really an issue.

> 
> [Unit]
> Description=Automounts filesystems on demand
> After=network.target ypbind.service
> 
> [Service]
> Type=forking
> PIDFile=/run/autofs.pid
> ExecStart=/usr/sbin/automount -p /run/autofs.pid
> 
> [Install]
> WantedBy=multi-user.target
> 
> Since admins/power users alike should get used to the idea to copy their unit
> files to /etc/systemd/system directory and add any options to it there... 
> 
> Btw what where those custom init script action you mention in comment 5?

There are two, forcestart and forcerestart.

These two options allow the user to force startup behaviour
that previously existed. The real use of them now is to force
all mounts to be lazy umounted at start if automount has become
confused.

However, lazy umounting has it's own set of problems so it
isn't something that is recommended. I think it best to just
not provide these and give guidance to manually use them in
bugs if needed. So lets leave it for now.

Ian
Comment 14 Ian Kent 2011-12-09 02:13:43 EST
Created attachment 544361 [details]
Patch - remove empty command line arguments (passed by systemd).
Comment 15 Jóhann B. Guðmundsson 2011-12-09 04:00:18 EST
(In reply to comment #13)
> (In reply to comment #12)
> 
> I think it is better to have a place outside of the unit file
> for this because it is better for the admin to not have to
> change these files and so not risk messing them up. Especially
> so since there are a couple of commonly (and frequently) used
> options that are site dependent. Bottom line is that it is
> much simpler to "sed" edit a single file than to copy and
> then "sed" edit a file when you need to do this on a large
> number of machines. But also, when you have a large number
> of machines, you always want to keep the amount of change
> to the base install to an absolute minimum from a maintenance
> POV.
> 
> Clearly, on a site with a dozen or so machines it isn't
> really an issue.

Well that's why you would create and keep a unit file in the /etc/systemd/system directory which will either be a full blown unit file with your site specific changes and you keep synced across servers or an unit file that contains only .include ( inhered everything from the unit in /lib/systemd/system useful if you want to inherit any updates the distribution package would bring to an unit file without having to make what ever changes the update brings across all servers ) and only your site specific changes. 

For daemons that cannot parse configuration files at startup the need for a separate file that contains nothing but OPTIONS="" ( and many legacy sysv init script where parsing nothing more then if OPTIONS == YES then do $foo which is nothing more than an scripted admins workaround for a limitation in the daemon ) is obsolete and a thing of the past ( legacy sysv init system ).

Using unit's in the /etc/systemd/system directory brings unison across distribution using systemd since afaik /etc/sysconfig is Red Hat specific and other distributions have their own directory called something else so in the long run across the GNU/Linux ecosystem it's best that all distribution using systemd stop doing that because it really does not server any purpose and the need for it truly is obsoleted...
Comment 16 Ian Kent 2011-12-09 19:09:27 EST
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > 
> > I think it is better to have a place outside of the unit file
> > for this because it is better for the admin to not have to
> > change these files and so not risk messing them up. Especially
> > so since there are a couple of commonly (and frequently) used
> > options that are site dependent. Bottom line is that it is
> > much simpler to "sed" edit a single file than to copy and
> > then "sed" edit a file when you need to do this on a large
> > number of machines. But also, when you have a large number
> > of machines, you always want to keep the amount of change
> > to the base install to an absolute minimum from a maintenance
> > POV.
> > 
> > Clearly, on a site with a dozen or so machines it isn't
> > really an issue.
> 
> Well that's why you would create and keep a unit file in the
> /etc/systemd/system directory which will either be a full blown unit file with
> your site specific changes and you keep synced across servers or an unit file
> that contains only .include ( inhered everything from the unit in
> /lib/systemd/system useful if you want to inherit any updates the distribution
> package would bring to an unit file without having to make what ever changes
> the update brings across all servers ) and only your site specific changes. 

Yes, that's a fair call.

I'm still not entirely convinced the unit file is the right
place to do this but, in time, when the configuration entries
are placed into a separate autofs specific file we'll see how
the user base reacts to requiring the command line options be
placed into the unit file itself.

> 
> For daemons that cannot parse configuration files at startup the need for a
> separate file that contains nothing but OPTIONS="" ( and many legacy sysv init
> script where parsing nothing more then if OPTIONS == YES then do $foo which is
> nothing more than an scripted admins workaround for a limitation in the daemon
> ) is obsolete and a thing of the past ( legacy sysv init system ).

In the automount case I did take the parsing of this configuration
file into the daemon and all the other entries don't actually need
to be set in the environment. Given the advent of systemd it now
seems I got that right. But parsing options to be passed to the
program can't be done by the program, ;)

> 
> Using unit's in the /etc/systemd/system directory brings unison across
> distribution using systemd since afaik /etc/sysconfig is Red Hat specific and
> other distributions have their own directory called something else so in the
> long run across the GNU/Linux ecosystem it's best that all distribution using
> systemd stop doing that because it really does not server any purpose and the
> need for it truly is obsoleted...

Good point, that does mean my upstream implementation needs
more work, I'll get onto that.
 
I can't argue against your logic but, at the end of the day,
this isn't a huge issue, to have, or not to have, such a
configuration file. Using it in this way is supported by systemd
so I won't break backward compatibility at this early stage
of adding systemd support. Clearly it will likely happen later.

Ian
Comment 17 Bill Peck 2012-01-20 10:40:13 EST
Hello,

Hope its ok to chime in here. 

I am using autofs on Fedora16 with ldap maps.  Even though my system is connected via wired ethernet it fails to get the maps because autofs comes up before NetworkManager gets the address.

I installed the rawhide version of autofs-5.0.6-6.fc17 hoping it would solve the problem, but it doesn't.

I ended up creating the following file in /etc/NetworkManager/dispatcher.d/12-autofs

#!/bin/sh

case "$2" in 
    up|down|vpn-up|vpn-down)
        /bin/systemctl restart autofs.service || :
        ;;
esac

Is this acceptable?  Should this be part of the autofs package by default?

Thanks!
Comment 18 Jóhann B. Guðmundsson 2012-01-20 10:47:41 EST
Does it not work after you enable the NetworkManager-wait-online.service ?
Comment 19 Bill Peck 2012-01-20 11:40:06 EST
(In reply to comment #18)
> Does it not work after you enable the NetworkManager-wait-online.service ?

Didn't seem to work.  Just going to show the commands I did here in case I missed something..

[root@localhost ~]# systemctl enable NetworkManager-wait-online.service

[root@localhost ~]# reboot

[root@localhost ~]# systemctl status NetworkManager-wait-online.service
NetworkManager-wait-online.service - Network Manager Wait Online
	  Loaded: loaded (/lib/systemd/system/NetworkManager-wait-online.service; enabled)
	  Active: inactive (dead) since Fri, 20 Jan 2012 11:34:30 -0500; 2min 30s ago
	 Process: 1004 ExecStart=/usr/bin/nm-online -q --timeout=30 (code=exited, status=0/SUCCESS)
	  CGroup: name=systemd:/system/NetworkManager-wait-online.service

[root@localhost ~]# ls /home
[root@localhost ~]# 

[root@localhost ~]# systemctl restart autofs.service
[root@localhost ~]# ls /home
bpeck  gpeck  jpeck  mpeck  vpeck
Comment 20 Jóhann B. Guðmundsson 2012-01-20 11:53:04 EST
Are you using the native systemd unit ( if so could you try adding to it NetworkManager-wait-online.service in the After= line ) or the legacy sysv init script?
Comment 21 Bill Peck 2012-01-20 16:05:07 EST
I'm using native systemd unit file

[root@localhost ~]# cat /lib/systemd/system/autofs.service 
[Unit]
Description=Automounts filesystems on demand
After=network.target ypbind.service NetworkManager-wait-online.service

[Service]
Type=forking
PIDFile=/run/autofs.pid
EnvironmentFile=-/etc/sysconfig/autofs
ExecStart=/usr/sbin/automount ${OPTIONS} --pid-file /run/autofs.pid

[Install]
WantedBy=multi-user.target


I added the NetworkManager-wait-online.service but it didn't make a difference.
Comment 22 Orion Poplawski 2012-02-20 13:10:36 EST
Another issue - the previous SysV script was enabled by default.  The new systemd init script is not.  Not sure if this is intentional or not.  See bug  768655.
Comment 23 Jóhann B. Guðmundsson 2012-02-20 13:30:34 EST
It should be enable in fresh installs not on upgrade from legacy sysv to systemd units. 

Users will need to enable the service again themselves after the upgrade/update that introduces the native unit files [1]. 

"When updating from a package containing a SysV initscript to a package containing a systemd unit file, we "start-over fresh" with default start and stop policy from the new package, and do not migrate what the user had previously configured. "

This also should have been mentioned in the F15/F16 and now also F17 release notes so the previous mentioned bug there in comment 22 should be closed NOTABUG


1.http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd
Comment 24 Orion Poplawski 2012-02-20 14:20:58 EST
Well, at least in autofs-5.0.6-11.fc17.x86_64 there is no:

%post
if [ $1 -eq 1 ] ; then 
    # Initial installation
    /bin/systemctl enable apache-httpd.service >/dev/null 2>&1 || :
fi

that would enable it by default on install.
Comment 25 Jóhann B. Guðmundsson 2012-02-20 15:42:05 EST
That certainly is a bug since autofs has been granted exception from FESCO to be enabled by default...

Basically the systemd relevant section should look something like this...

Note that I'm not provenpackager but then again spec files aren't build upon some rocket science...

# Systemd

# Adding the submitted unit

Source1: %{name}.service

# Adding the build require

BuildRequires: systemd-units

# Adding the requires

Requires(post): systemd-units
Requires(preun): systemd-units
Requires(postun): systemd-units

#%install...

# Create the unit directory

mkdir -p %{buildroot}%{_unitdir}

# Install the unit file which is SOURCE1

%{__install} -p -D -m 0644 %{SOURCE1} %{buildroot}%{_unitdir}/%{name}.service

# Drop the legacy sysv inits script if applicable

rm -rf %{buildroot}%{_initrddir}

# Initial systemd installation

%post
if [ $1 -eq 1 ] ; then
# Initial installation
%{_bindir}/systemctl daemon-reload >/dev/null 2>&1 || :
# Only applicable if service has been granted exception from fesco
# to be enabled by default
%{_bindir}/systemctl enable %{name}.service >/dev/null 2>&1 || :
fi

# systemd package removal not upgrade

%preun
if [ $1 -eq 0 ]; then
# Package removal, not upgrade
%{_bindir}/systemctl --no-reload disable %{name}.service > /dev/null 2>&1 || :
%{_bindir}/systemctl stop %{name}.service > /dev/null 2>&1 || :
fi

# systemd package upgrade not removal

%postun
%{_bindir}/systemctl daemon-reload >/dev/null 2>&1 || :
if [ $1 -ge 1 ] ; then
# Package upgrade, not removal
%{_bindir}/systemctl try-restart %{name}.service >/dev/null 2>&1 || :
fi

%triggerun -- %{name} < $bla release
# Save the current service runlevel info
# User must manually run systemd-sysv-convert --apply %{name}
# to migrate them to systemd targets
%{_bindir}/systemd-sysv-convert --save %{name} >/dev/null 2>&1 ||:

# Run these because the SysV package being removed won't do them
%{_sbindir}/chkconfig --del %{name} >/dev/null 2>&1 || :
%{_bindir}/systemctl try-restart %{name}.service >/dev/null 2>&1 || :

#%files ...
%{_unitdir}/%{name}.service

That's it...

Packagers/maintainers should be able to use this as an drop in replacement for the package with the exception of the "/bin/systemctl enable %{name}.service >/dev/null 2>&1 || :" in the %post section which should only be done if the relevant package is on this list [1],

Ian you can compare the above with what you have in your package(s) and fix any missing entries...

1.http://fedoraproject.org/wiki/Starting_services_by_default
Comment 26 Ian Kent 2012-02-20 19:34:19 EST
(In reply to comment #25)
> 
> Ian you can compare the above with what you have in your package(s) and fix any
> missing entries...

Sure, I can see a couple of differences.
I'll do it as soon as I can.
Comment 27 Ian Kent 2012-02-21 04:06:43 EST
(In reply to comment #26)
> (In reply to comment #25)
> > 
> > Ian you can compare the above with what you have in your package(s) and fix any
> > missing entries...
> 
> Sure, I can see a couple of differences.
> I'll do it as soon as I can.

While we're at any idea why can't I do a:
systemctl reload autofs.service

It returns:
Failed to issue method call: Job type reload is not applicable for unit autofs.service.
Comment 28 Jóhann B. Guðmundsson 2012-02-21 09:07:29 EST
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > 
> > > Ian you can compare the above with what you have in your package(s) and fix any
> > > missing entries...
> > 
> > Sure, I can see a couple of differences.
> > I'll do it as soon as I can.
> 
> While we're at any idea why can't I do a:
> systemctl reload autofs.service
> 
> It returns:
> Failed to issue method call: Job type reload is not applicable for unit
> autofs.service.

You are missing a ExecReload= statement in your unit

ExecReload=/usr/bin/kill -HUP $MAINPID <-- is probably what you are looking for
Comment 29 Ian Kent 2012-02-23 21:40:59 EST
Created attachment 565465 [details]
Patch - systemd support fixes

The spec file fixes seen applied to the tar spec file have
also been applied to the package spec file as well as enabling
the %triggerun scriplet.
Comment 30 Orion Poplawski 2012-02-27 12:44:35 EST
autofs-5.0.6-13.fc17.x86_64 looks good to me. Enabled by default.
Comment 31 Jóhann B. Guðmundsson 2012-03-02 05:37:25 EST
Ian is there something remaining preventing your from closing this bug as FIXED?
Comment 32 Gwyn Ciesla 2012-04-18 15:34:22 EDT
Looks fixed.

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