Bug 713573 - Provide mdadm native systemd unit file
Summary: Provide mdadm native systemd unit file
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: mdadm
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Milan Broz
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: SysVtoSystemd
TreeView+ depends on / blocked
 
Reported: 2011-06-15 19:34 UTC by Stephen Gallagher
Modified: 2013-03-01 04:10 UTC (History)
7 users (show)

Fixed In Version: mdadm-3.2.2-3.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-07-07 11:49:23 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
rpcbind.target (470 bytes, text/plain)
2011-06-15 23:37 UTC, Jóhann B. Guðmundsson
no flags Details
rpcbind.service socket related (243 bytes, text/plain)
2011-06-15 23:38 UTC, Jóhann B. Guðmundsson
no flags Details
native systemd socket file for rpcbind (132 bytes, text/plain)
2011-06-15 23:39 UTC, Jóhann B. Guðmundsson
no flags Details
Native systemd service file for mdmonitor (397 bytes, text/plain)
2011-06-16 14:18 UTC, Jóhann B. Guðmundsson
no flags Details
Native systemd service file for mdmonitor (397 bytes, text/plain)
2011-06-27 19:53 UTC, Jóhann B. Guðmundsson
no flags Details
tmpfile for mdmonitor (34 bytes, text/plain)
2011-06-27 19:55 UTC, Jóhann B. Guðmundsson
no flags Details
Spec file which introduce systemd unit file tmpfile for mdmonitor and drops SysV support (25.25 KB, text/plain)
2011-06-27 19:56 UTC, Jóhann B. Guðmundsson
no flags Details
removed ExecStartPre= line (330 bytes, text/plain)
2011-06-28 15:54 UTC, Jóhann B. Guðmundsson
no flags Details
renamed the mdmonitor.conf tmpfile to mdadm.conf for spec patch (34 bytes, text/plain)
2011-06-30 11:14 UTC, Jóhann B. Guðmundsson
no flags Details
Patch that Introduce systemd unit file, drop SysV support to the spec file.. (2.25 KB, patch)
2011-06-30 11:15 UTC, Jóhann B. Guðmundsson
no flags Details | Diff
service file that takes care of the missing part from mdmonitor (189 bytes, application/octet-stream)
2011-06-30 12:52 UTC, Jóhann B. Guðmundsson
no flags Details
Patch that Introduce systemd unit file mdmonitor and mdraid-takeover, drop SysV support to the spec file.. (2.38 KB, patch)
2011-06-30 12:59 UTC, Jóhann B. Guðmundsson
no flags Details | Diff
All in one patch (5.45 KB, patch)
2011-07-04 16:33 UTC, Milan Broz
no flags Details | Diff
All in one patch (5.50 KB, patch)
2011-07-07 11:15 UTC, Milan Broz
no flags Details | Diff

Description Stephen Gallagher 2011-06-15 19:34:02 UTC
https://fedoraproject.org/wiki/Features/SysVtoSystemd

Comment 1 Jóhann B. Guðmundsson 2011-06-15 23:36:09 UTC
Just throwing this in for history purpose.. 

http://www.spinics.net/lists/linux-nfs/msg14371.html

Comment 2 Jóhann B. Guðmundsson 2011-06-15 23:37:27 UTC
Created attachment 504954 [details]
rpcbind.target

Comment 3 Jóhann B. Guðmundsson 2011-06-15 23:38:31 UTC
Created attachment 504955 [details]
rpcbind.service socket related

Comment 4 Jóhann B. Guðmundsson 2011-06-15 23:39:21 UTC
Created attachment 504956 [details]
native systemd socket file for rpcbind

Comment 5 Jóhann B. Guðmundsson 2011-06-16 13:43:37 UTC
whoops had bug against the wrong component open when I filed all of these sorry ;)

Comment 6 Jóhann B. Guðmundsson 2011-06-16 14:18:39 UTC
Created attachment 505054 [details]
Native systemd service file for mdmonitor

Comment 7 Jóhann B. Guðmundsson 2011-06-16 14:24:44 UTC
Ok so I've converted most of the old sysv script to a native systemd one for you however this relevant section is missing.

# (Re)start mdmon to take over monitoring of mdmon started from the initrd
    for i in /dev/md/*.pid; do
        if [ -r $i ]; then
            origprog="$prog"; prog="mdmon"
            action $"Starting $prog: " /sbin/mdmon --takeover --all
            prog="$origprog"
            break
        fi
    done

Along with the creation of the directory which should be done in a tmpfile see 

http://fedoraproject.org/wiki/Packaging:Tmpfiles.d for details.. 

Again sorry for the rpbind stuff I attached here..

Comment 8 Jóhann B. Guðmundsson 2011-06-27 14:02:01 UTC
What's the current status on this?

We need this in rawhide as soon as possible unless ofcourse you guys want to
block alpha.. 

https://fedoraproject.org/wiki/Packaging:Guidelines:Systemd
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd
http://fedoraproject.org/wiki/Packaging:Tmpfiles.d

Comment 9 Jóhann B. Guðmundsson 2011-06-27 19:53:51 UTC
Created attachment 510149 [details]
Native systemd service file for mdmonitor

Comment 10 Jóhann B. Guðmundsson 2011-06-27 19:55:01 UTC
Created attachment 510150 [details]
tmpfile for mdmonitor

Comment 11 Jóhann B. Guðmundsson 2011-06-27 19:56:00 UTC
Created attachment 510151 [details]
Spec file which introduce systemd unit file tmpfile for mdmonitor and drops SysV support

Comment 12 Milan Broz 2011-06-28 14:13:40 UTC
Is this line needed?
ExecStartPre=/bin/grep '^\(MAILADDR\|PROGRAM\) .' /etc/mdadm.conf

I think that mdadm will fail to start in this case:
# mdadm --monitor --scan -f --pid-file=/var/run/mdadm/mdadm.pid ; echo $?
mdadm: No mail address or alert command - not monitoring.
1

Comment 13 Jóhann B. Guðmundsson 2011-06-28 14:24:52 UTC
It's taken from this part of the old sysv init script. 

# Make sure configuration file exists and has information we can use
# MAILADDR or PROGRAM or both must be set in order to run mdadm --monitor
    [ -f /etc/mdadm.conf ] || return 6
    grep '^\(MAILADDR\|PROGRAM\) .' /etc/mdadm.conf >/dev/null 2>&1 || return 6


it can be added if that line is needed "-" which will continue with the service startup

As in 

ExecStartPre=-/bin/grep '^\(MAILADDR\|PROGRAM\) .' /etc/mdadm.conf

Comment 14 Milan Broz 2011-06-28 14:49:31 UTC
I know that it is in sysv script but is this hack really needed?

I think check for mdadm.conf is enough. If mdadm.conf contains no valid entry - it is ok that it will end in failed state - it is not properly configured IMHO.

Did you test it with dracut and root fs over md?

Comment 15 Jóhann B. Guðmundsson 2011-06-28 15:40:10 UTC
(In reply to comment #14)
> I know that it is in sysv script but is this hack really needed?

You have to ask the mdmonitor sysv maintainer about if that is fine along with this part missing from the submitted service file

# (Re)start mdmon to take over monitoring of mdmon started from the initrd
    for i in /dev/md/*.pid; do
        if [ -r $i ]; then
            origprog="$prog"; prog="mdmon"
            action $"Starting $prog: " /sbin/mdmon --takeover --all
            prog="$origprog"
            break
        fi
    done


> I think check for mdadm.conf is enough. If mdadm.conf contains no valid entry -
> it is ok that it will end in failed state - it is not properly configured IMHO.

Ok let's simply delete that line are you aware of any potential After= or Before we might need to add to the service file.

> Did you test it with dracut and root fs over md?

Unfortunetly I dont have the necessary hw hence my test is limited to see if the service file starts and stop cleanly once I achieved that I filed the service file and made note if any of the old sysv init script function is missing.

Comment 16 Jóhann B. Guðmundsson 2011-06-28 15:54:55 UTC
Created attachment 510311 [details]
removed ExecStartPre= line

Comment 17 Jóhann B. Guðmundsson 2011-06-28 19:03:59 UTC
Just out of curiosity is this service applicant for hardware activation
unfortunately our guidelines are a bit short on how to do that 

"Hardware activation

Hardware activation occurs when a service is installed but only turns on if a
certain type of hardware is installed. Enabling of the service is normally done
with a udev rule. At this time we do not have further guidance on how to write
those udev rules. The service itself installs its .service files in the normal
places and are installed by the normal systemd scriptlets. These services
should never be enabled by the package as they will be enabled by udev. "

But you can read man systemd.device and look at the bluetooth service which is
doing that I believe..

Comment 18 Jóhann B. Guðmundsson 2011-06-30 11:14:40 UTC
Created attachment 510637 [details]
renamed the mdmonitor.conf tmpfile to mdadm.conf for spec patch

Comment 19 Jóhann B. Guðmundsson 2011-06-30 11:15:43 UTC
Created attachment 510638 [details]
Patch that Introduce systemd unit file, drop SysV support to the spec file..

Comment 20 Jóhann B. Guðmundsson 2011-06-30 11:21:34 UTC
Is there anything preventing the package being built with that patch,service and patch file so we can get this off the alpha blocker list?

Any changes can be introduced later in the release cycle as an update.

Comment 21 Milan Broz 2011-06-30 11:35:44 UTC
I just updated mdadm in rawhide, please wait few days if there are no problems with that and then this perhaps can be added.

There is some interaction with dracut - is it tested even in this situation? (e.g. if you have root device on MD raid1 activated in dracut).

(If Doug does not add this in the meantime, I'll try to check patches later.)

Comment 22 Jóhann B. Guðmundsson 2011-06-30 11:54:35 UTC
(In reply to comment #21)
> I just updated mdadm in rawhide, please wait few days if there are no problems
> with that and then this perhaps can be added.
> 
> There is some interaction with dracut - is it tested even in this situation?
> (e.g. if you have root device on MD raid1 activated in dracut).
> 
> (If Doug does not add this in the meantime, I'll try to check patches later.)

Unfortunately I dont have any raid capable hardware to spare to test this which is kinda why I'm emphasizing so much pushing this into rawhide as in to get it out there to get some experience on this. 

I have test building the package with that patch and tested starting and stopping the service however we need to catch any problems and fix them before we start composing alpha or else this will become an alpha release blocker the other alternative is to remove this from @base and and make sure this does not get installed on the official livecd. ( then we are working around what's considered an alpha blocker ). 

I'll ping Harald to see if this potentially causes some trouble with dracut and or to see if there are some changes that need to be made on his behalf.

Comment 23 Milan Broz 2011-06-30 12:05:18 UTC
It is easy to setup it in virtual machine.

If there is no problem with dracut interaction, I think we can switch to mdadm unit file. But I do not want to break it completely now :)
I can do only limited testing here.

Comment 24 Jóhann B. Guðmundsson 2011-06-30 12:19:53 UTC
(In reply to comment #23)
> It is easy to setup it in virtual machine.
> 
> If there is no problem with dracut interaction, I think we can switch to mdadm
> unit file. But I do not want to break it completely now :)
> I can do only limited testing here.

Harald mentioned in irc "in theory "mdmon" could be modified to recognize those files itsselves and then carry out whatever "takeover" strategy it has no need for extra bloat in scripts that would be my preferred solution"

Now we just need to wait for some input from Doug I guess to see what he has to say

Comment 25 Jóhann B. Guðmundsson 2011-06-30 12:52:26 UTC
Created attachment 510655 [details]
service file that takes care of the missing part from mdmonitor

Comment 26 Jóhann B. Guðmundsson 2011-06-30 12:59:27 UTC
Created attachment 510661 [details]
Patch that Introduce systemd unit file mdmonitor and mdraid-takeover, drop SysV support to the spec file..

Comment 27 Jóhann B. Guðmundsson 2011-06-30 13:14:56 UTC
So we Harald cooked up an extra service script which get's pulled in at sysinit.target get's started and runs /sbin/mdmon --takeover --all if /dev/md/*.pid exist but fails otherwise.. 

This should take care of "# (Re)start mdmon to take over monitoring of mdmon started from the initrd" from the old initscript hence it would be good Milan if you would build the package and we ship it in rawhide.

Thanks

Comment 28 Milan Broz 2011-07-04 16:33:41 UTC
Created attachment 511209 [details]
All in one patch

All-in-one patch, slightly tested.
(I'll commit it to git soon.)

Comment 29 Michal Schmidt 2011-07-04 16:49:32 UTC
(In reply to comment #28)
> ConditionPathExists=/dev/md/*.pid

I do not see wildcards in conditions implemented in the source. Are you this unit works as expected?

Comment 30 Michal Schmidt 2011-07-04 16:58:48 UTC
(In reply to comment #28)
> %post
> if [ $1 -eq 1 ] ; then
>     /bin/systemctl daemon-reload >/dev/null 2>&1 || :
> fi

This is the fresh installation case. You'll likely want to enable the services here.

Comment 31 Jóhann B. Guðmundsson 2011-07-04 17:24:58 UTC
(In reply to comment #29)
> (In reply to comment #28)
> > ConditionPathExists=/dev/md/*.pid
> 
> I do not see wildcards in conditions implemented in the source. Are you this
> unit works as expected?

Nope untested since I dont have a raid hardware but it could be mapped to the presence of the /dev/md directory or a known pid

Comment 32 Jóhann B. Guðmundsson 2011-07-04 17:27:44 UTC
(In reply to comment #30)
> (In reply to comment #28)
> > %post
> > if [ $1 -eq 1 ] ; then
> >     /bin/systemctl daemon-reload >/dev/null 2>&1 || :
> > fi
> 
> This is the fresh installation case. You'll likely want to enable the services
> here.

best case scenario would be if it could be hardware activated ondemand..

Comment 33 Milan Broz 2011-07-04 19:16:23 UTC
(In reply to comment #29)
> > ConditionPathExists=/dev/md/*.pid
> 
> I do not see wildcards in conditions implemented in the source. Are you this
> unit works as expected?

:) this is the only part I did not try to run (because I have no MD array with external metadata).

Not sure how to do fix that... And we will need to move /dev/md dir somewhere else but it can be done later.


>> if [ $1 -eq 1 ] ; then
>>     /bin/systemctl daemon-reload >/dev/null 2>&1 || :
>This is the fresh installation case. You'll likely want to enable the services
>here.

hm, yes. I tested  upgrade path... so this:

/bin/systemctl enable mdmonitor.service mdmonitor-takeover.service >/dev/null 2>&1 || :

The /etc/mdadm.conf is not there by default, so service is disabled. It is sw raid, so user can activate it anytime, I think this will work as expected.

Comment 34 Jóhann B. Guðmundsson 2011-07-04 19:20:01 UTC
Note that you dont have to enable the mdmonitor-takeover.service it gets pulled in via the sysinit.target

Comment 35 Jóhann B. Guðmundsson 2011-07-04 19:21:42 UTC
No wait I'm mistaken...

Comment 36 Milan Broz 2011-07-07 11:15:38 UTC
Created attachment 511672 [details]
All in one patch

Hopefully final version
- remove *pid condition, make it type-forking (takeover service is run always,
only if there are external arrays it restarts mdmon)
- added RemainAfterExit=yes so takeover is visible in services
- enable by default in %post

Comment 37 Milan Broz 2011-07-07 11:49:23 UTC
The proof of the pudding's in the eating...

Commited in mdadm-3.2.2-3.fc16


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