Graceful restart also for imapd.conf

[Olivier ROLAND]

2011/8/9 Greg Banks <gnb at fastmail.fm>:
> 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?

The idea is to propagate SIGHUP to all service/children when master
receive a SIGHUP
(if we don't want to check exactly what change in the imapd.conf ...)
This mean : more CPU and I/O when we change only master.conf because
process were not recycled this time but more convenient for sysadmin
and graceful restart on imapd.conf.

>
>>  In
>> fact I'm very surprised that this was not done before ;-)
>>
>>
>
> The master process has many other problems :(

Is there any tickets about that ? Maybe we could help.

> --
> Greg.
>
>

Olivier ROLAND
AtoS