Red Hat Bugzilla – Full Text Bug Listing
|Summary:||Provide mdadm native systemd unit file|
|Product:||[Fedora] Fedora||Reporter:||Stephen Gallagher <sgallagh>|
|Component:||mdadm||Assignee:||Milan Broz <mbroz>|
|Status:||CLOSED RAWHIDE||QA Contact:||Fedora Extras Quality Assurance <extras-qa>|
|Version:||rawhide||CC:||agk, dledford, harald, johannbg, mbroz, mschmidt, pvrabec|
|Fixed In Version:||mdadm-3.2.2-3.fc16||Doc Type:||Bug Fix|
|Doc Text:||Story Points:||---|
|Last Closed:||2011-07-07 07:49:23 EDT||Type:||---|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
|Bug Depends On:|
Description Stephen Gallagher 2011-06-15 15:34:02 EDT
Comment 1 Jóhann B. Guðmundsson 2011-06-15 19:36:09 EDT
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 19:37:27 EDT
Created attachment 504954 [details] rpcbind.target
Comment 3 Jóhann B. Guðmundsson 2011-06-15 19:38:31 EDT
Created attachment 504955 [details] rpcbind.service socket related
Comment 4 Jóhann B. Guðmundsson 2011-06-15 19:39:21 EDT
Created attachment 504956 [details] native systemd socket file for rpcbind
Comment 5 Jóhann B. Guðmundsson 2011-06-16 09:43:37 EDT
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 10:18:39 EDT
Created attachment 505054 [details] Native systemd service file for mdmonitor
Comment 7 Jóhann B. Guðmundsson 2011-06-16 10:24:44 EDT
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 10:02:01 EDT
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 15:53:51 EDT
Created attachment 510149 [details] Native systemd service file for mdmonitor
Comment 10 Jóhann B. Guðmundsson 2011-06-27 15:55:01 EDT
Created attachment 510150 [details] tmpfile for mdmonitor
Comment 11 Jóhann B. Guðmundsson 2011-06-27 15:56:00 EDT
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 10:13:40 EDT
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 10:24:52 EDT
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 10:49:31 EDT
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 11:40:10 EDT
(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 11:54:55 EDT
Created attachment 510311 [details] removed ExecStartPre= line
Comment 17 Jóhann B. Guðmundsson 2011-06-28 15:03:59 EDT
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 07:14:40 EDT
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 07:15:43 EDT
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 07:21:34 EDT
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 07:35:44 EDT
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 07:54:35 EDT
(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 08:05:18 EDT
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 08:19:53 EDT
(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 08:52:26 EDT
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 08:59:27 EDT
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 09:14:56 EDT
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 12:33:41 EDT
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 12:49:32 EDT
(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 12:58:48 EDT
(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 13:24:58 EDT
(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 13:27:44 EDT
(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 15:16:23 EDT
(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 15:20:01 EDT
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 15:21:42 EDT
No wait I'm mistaken...
Comment 36 Milan Broz 2011-07-07 07:15:38 EDT
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 07:49:23 EDT
The proof of the pudding's in the eating... Commited in mdadm-3.2.2-3.fc16