Graceful restart also for imapd.conf

[Greg Banks]

On 09/08/11 18:50, Olivier ROLAND 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 ...

I'm sorry I misunderstood.  Can you explain exactly what signal you 
propose to send to which process when?

>   In
> fact I'm very surprised that this was not done before ;-)
>
>

The master process has many other problems :(

-- 
Greg.