Ovirt devel - May 2008 - [PATCH] wrappers for taskomatic.rb and host-status.rb

Hi Ovirt!

taskomatic.rb and host-status.rb were becoming daemons by using
"Daemons.daemonize".

This had the unfortunate effect of detaching them from any controlling
terminal, and so using killproc in /etc/init.d/ovirt-wui was *not*
killing them. I noticed that after repeated 'service ovirt-wui
restart', I had many stray taskomatic & host-status processes, all
making connections to postgresql, and sending my appliance way into
swap.

So this patch:
0) renames taskomatic.rb and host-status.rb to taskomatic_.rb and
host-status_.rb, respectively.
1) replaces taskomatic.rb and host-status.rb with wrapper scripts for
taskomatic_.rb and host-status_.rb. The wrappers use the
"Daemons.run(script,options)" method instead of Daemons.daemonize.
2) edits /etc/init.d/ovirt-wui to invoke 'taskomatic.rb start' and
'taskomatic.rb stop' (same for host-status.rb) instead of the
initscript functions daemon and killproc.

Please note that the original scripts accepted some command line
arguments; that code is still present in taskomatic_.rb and
host-status_.rb, but I did not provide a mechanism for passing command
line options via the wrapper scripts.

Thanks for your consideration,
Steve Linabery
-------------- next part --------------
A non-text attachment was scrubbed...
Name: daemons.patch
Type: text/x-patch
Size: 15970 bytes
Desc: not available
URL:
<http://listman.redhat.com/archives/ovirt-devel/attachments/20080501/9a62f4fd/attachment.bin>

On Thu, May 01, 2008 at 05:37:04PM -0500, steve linabery
wrote:
> Hi Ovirt!
> 
> taskomatic.rb and host-status.rb were becoming daemons by using
> "Daemons.daemonize".
> 
> This had the unfortunate effect of detaching them from any controlling
> terminal, and so using killproc in /etc/init.d/ovirt-wui was *not*
> killing them. I noticed that after repeated 'service ovirt-wui
> restart', I had many stray taskomatic & host-status processes, all
> making connections to postgresql, and sending my appliance way into
> swap.
> 
> So this patch:
> 0) renames taskomatic.rb and host-status.rb to taskomatic_.rb and
> host-status_.rb, respectively.
> 1) replaces taskomatic.rb and host-status.rb with wrapper scripts for
> taskomatic_.rb and host-status_.rb. The wrappers use the
> "Daemons.run(script,options)" method instead of
Daemons.daemonize.
> 2) edits /etc/init.d/ovirt-wui to invoke 'taskomatic.rb start' and
> 'taskomatic.rb stop' (same for host-status.rb) instead of the
> initscript functions daemon and killproc.
> 
> Please note that the original scripts accepted some command line
> arguments; that code is still present in taskomatic_.rb and
> host-status_.rb, but I did not provide a mechanism for passing command
> line options via the wrapper scripts.
> 
> Thanks for your consideration,
> Steve Linabery
[snip]

Patch looks fine. Git experts: Is it cleaner to "move" taskomatic.rb
to taskomatic_.rb in the git repo, thereby preserving the changelog
for the file even while renaming?

With that caveat,
ACK

--Hugh

On Thu, May 01, 2008 at 05:37:04PM -0500, steve linabery
wrote:
> Hi Ovirt!
> 
> taskomatic.rb and host-status.rb were becoming daemons by using
> "Daemons.daemonize".
> 
> This had the unfortunate effect of detaching them from any controlling
> terminal, and so using killproc in /etc/init.d/ovirt-wui was *not*
> killing them. I noticed that after repeated 'service ovirt-wui
> restart', I had many stray taskomatic & host-status processes, all
> making connections to postgresql, and sending my appliance way into
> swap.
> 
> So this patch:
> 0) renames taskomatic.rb and host-status.rb to taskomatic_.rb and
> host-status_.rb, respectively.
> 1) replaces taskomatic.rb and host-status.rb with wrapper scripts for
> taskomatic_.rb and host-status_.rb. The wrappers use the
> "Daemons.run(script,options)" method instead of
Daemons.daemonize.
> 2) edits /etc/init.d/ovirt-wui to invoke 'taskomatic.rb start' and
> 'taskomatic.rb stop' (same for host-status.rb) instead of the
> initscript functions daemon and killproc.
This is not really a good idea.

The decision about how/if to daemonize belows to the initscripts. In 
SysV init this is daemon/killproc. In the new Upstart world, it doesn't
daemonize at all - it keeps ownership of the process so it can detect 
death & restart.  

Xen used to follow the approach of your patch too and we just spent
time ripping out all that code and making it used daemon/killproc
correctly in the initscripts in F10.

The simpler way around this problem would have just been to change the
existing initscript to pass '-n' when starting the programs so that
the Daemons.daemonize() call was skipped - it was redundant when running
the program via the 'daemon' shell command. And figuring out whatever 
is
needed to make killproc work correctly.

The existing initscript also really needs to be split up so that there is
one initscript per daemon that ovirt has.

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|