On 09/14/2012 07:55 PM, Simo Sorce wrote:
On Fri, 2012-09-14 at 17:16 +0200, Michal Židek wrote:
> On 09/13/2012 07:59 PM, Simo Sorce wrote:
>> Main questions:
>> the invalidation functions seem to be called unconditionally, what
>> prevent cache invalidation durint a normal log rotation ?
>> Shouldn't cache invalidation happen exclusively when sss_cache is asking
>> to drop caches ?
>>
>
> It would be nice to have these two operations separate, but I do not
> know how to do it if we are to use SIGHUP for this. The application
> does not know what is the origin of SIGHUP, so it has to do both
> operations. For now, I wrote FIXME comment to the code in the new patch.
Before sending the SIGHUP touch a file in the same directory where the
cache files are, and in sssd_nss stat and check if that file is present.
If it is, remove it and clean the caches.
> Question: What do we plan to use to signal SSSD to reload configuration
> during runtime? Isn't it SIGHUP too? I am not sure now, but if it is
> SIGHUP, then we need to reinitialize memory caches every time SIGHUP is
> received by monitor (this is what the patch does right know). So maybe
> it is not that bad after all.
You really *do not* want purge the caches just because the configuration
was reloaded. That would be quite a waste.
>>
>> Nack:
>>
>> I do not like the following function as it is largely a copy/paste of
>> sss_mmap_cache_init.
>> Why donp;t you simply pass a pointer to struct sss_mc_ctx *mc_ctx
>> release it, and then simply call sss_mmap_cache_init() from scratch ?
>>
>> Also n_elem is currently hardcoded, so saving it is not really useful,
>> however I would also *really* not save it as on cache invalidation you
>> will want to go and see if the configuration also changed and get the
>> new number of elements.
>> This is a perfect time to do that because you are killing the old file
>> and creating a new one so changing size during this operation is
>> actually a good idea (if needed).
>>
>> Same for memcache_timeout parameters (which are already parametrized and
>> can be fetched from CONFDB_NSS_CONF_ENTRY / CONFDB_MEMCACHE_TIMEOUT
>>
>
> I removed the old invalidate function and created new called
> sss_mmap_cache_reinit. This function is used for re-initialize
> already initialized memory cache. I think the name is more suitable,
> because it now allows to change entry timeout and cache size.
>
> New patches are attached.
Looks ok, but I do not like that you keep the n_elem variable.
Please create a macro until we get a tunable option.
Something like:
#define SSS_MC_CACHE_ELEMENTS 50000
And just use that.
Simo.
Ok. I changed it all a bit. The mechanism is now as follows:
1. sss_cache creates a file, that serves as flag with the meaning
"It was me, who sended SIGHUP".
2. monitor receives sighup and checks if the flag (file) exist
- exists: signal (dbus signal) nss to clear memory cache
- not exists: rotate logs and signal other services to rotate
logs too
To achieve this the te_server_hup is no longer used by monitor, all
what monitor uses to handle SIGHUP is monitor_hup. Other services
still remain using te_server_hup.
With the file flag, there is no need for nss_hup function. NSS will
remain using the old te_server_hup function and new nss_clear_memcache
function is introduced to handle the caches.
The flag (file) is removed in the sss_mmap_cache_init function. The
reason for not placing it to the monitor_hup handler was to avoid this
scenario:
1. Someone by mistake calls sss_cache tool when sssd is not running
2. The flag (file) is created
3. after that, sssd is turned on
4. someone sends SIGHUP to monitor to rotate logs
5. monitor sees the flag (file)
6. sssd clears memory cache and does not rotate anything
7. someone thinks how good he cleared the caches (?!NOoo!?)
When unlink is moved to init function, this will never happen (reinit
calls init, so it should be OK).
New patches are in attachment.
Thanks
Michal