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