Graceful restart also for imapd.conf

[Olivier ROLAND]

2011/8/9 Dave McMurtrie <dave64 at andrew.cmu.edu>:
>
>
> 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

Thank's Dave. Yes indeed your trick works (calling exactly the same
piece of code).
Good to know but can we reasonably tell a sysadmin to proceed like
that ? I don't think so.

Olivier ROLAND
AtoS