On 09/08/11 18:50, Olivier ROLAND wrote:
2011/8/9 Greg Banks<g...@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.
