Graceful restart also for imapd.conf

[Greg Banks]

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.