Graceful restart also for imapd.conf

[Dave McMurtrie]

On Aug 9, 2011, at 4:50 AM, Olivier ROLAND <cyrus-dev at edla.org> wrote:

> 2011/8/9 Greg Banks <gnb at fastmail.fm>:
>> G'day Olivier,
>> 
>> On 08/08/11 19:39, Olivier ROLAND wrote:
>>> 
>>> Hi there,
>>> 
>>> Since version 2.0.9 Cyrus master can re-read /etc/cyrus.conf on SIGHUP.
>>> 
>>> The man say :
>>>        Master rereads its configuration file when it receives a hangup
>>> signal,
>>>        SIGHUP.  Services and events may be added, deleted or modified when
>>> the
>>>        configuration file is reread.  Any active  services  removed  from
>>>  the
>>>        configuration file will be allowed to run until completion.
>>> 
>>> Looking at the code I wonder why we don't do the same to re-reread
>>> /etc/imapd.conf.
>>> Actually, master only send a SIGHUP to children when the service have
>>> been removed.
>>> My guess is that  we could send a SIGHUP to all services. That way we
>>> don't have to wait for all preforked processes to finish before taking
>>> into account the new imapd.conf
>>> I have made some test and till now all seem working well.
>> 
>> I haven't seen your code so I don't know how you're dealing with re-reading
>> imapd.conf.  But I note that in the code I see there's a bunch of global
>> variables in the imapd process which are set as a side effect of reading
>> imapd.conf.  Not handling those properly can have all sorts of unpleasant
>> effects, including memory leaks and pointers to freed memory.  You might
>> want to look at the recent commit in the master branch that adds a function
>> config_reset() that handles all that; it was added for unit testing but
>> could be useful when re-reading imapd.conf.
>> 
>> http://git.cyrusimap.org/cyrus-imapd/commit/?id=03b4fcc4d956ccbfbbd5220b6d1ca9107707821f
>> 
>>> The only tricky thing I see is that we don't want to SIGHUP children
>>> in the SERVICE_STATE_DEAD state but the actual code already take care
>>> of that.
>>> 
>>> So is there any problem that justify to limit the SIGHUP propagation
>>> to children for removed service only ?
>>> Or could we go a bit further (with very few modification to the code)
>>> and allow graceful restart when we modify /etc/imapd.conf.
>>> 
>> 
>> There are some imapd.conf entries which it would probably be a really bad
>> idea to modify partway through a client session, like 'virtdomains'.  There
>> are others which are only consulted during imapd process initialisation and
>> would be ignored if the file was re-read later, like 'mupdate_server'.  All
>> the code that uses config data was written under the assumption that the
>> data would not change in the lifetime of the process, and in some cases that
>> assumption allowed simplifying short-cuts which would not be valid
>> otherwise.
>> 
>> So while I think it would be great to be able to re-read imapd.conf on
>> SIGHUP, there may be quite a lot of work to clean up the fallout.
>> 
>> --
>> Greg.
>> 
>> 
> 
> Thanks a lot Greg for your feedback but you missed the point ;-)
> I don't change anything during the lifetime of the process.
> Cyrus let the last active child finally terminate with the old
> imapd.conf and then force the process to exit cleanly immediatly
> without recycling. (no additional code is needed, all is already
> there, see the code used when a service is disabled)
> We are with a brand new process for the new imapd.conf, so no problem
> with global variables, no problem with initialisation, etc ...
> 
> The idea here is to use this code not only when a service is disabled
> but every time master receive a SIGHUP.
> I see no side effect during all my tests but prefer to ask the mailing
> list just in case I missed something.
> Seem that we could have re-read imapd.conf functionality for free. In
> fact I'm very surprised that this was not done before ;-)
> 

If you're interested, you can already accomplish this by updating the mtime (touch) of the on-disk executable for whichever process you want to re-read imapd.conf.  This check is actually in place to detect if a new binary has been installed so you could (in theory) upgrade while Cyrus is still running, but changing the mtime of the binary should have the effect you desire.

Hth,

Dave