[SSSD] [PATCH] back end: periodic task API + refresh of expired records

Pavel Březina pbrezina at redhat.com
Thu May 23 09:46:10 UTC 2013


On 05/21/2013 12:06 PM, Jakub Hrozek wrote:
> In general it's not clear to me from the code who is responsible for
> freeing the task -- is it the caller, but only on success? I think this
> should always behave the same.

After discussion with Jakub we decided that the caller is responsible
for freeing the task.

When an internal error occur (unable to create tevent_timer), the task
is now disabled.

ERR_STOP_PERIODIC_TASK was removed. The request can disable the task
instead.

> be_ptask_schedule(): The DEBUG message uses %d but passes in a string.

Fixed.

> [PATCH 2/4] back end: periodical refresh of expired records API
> The "enum be_refresh" name is too generic. Maybe "enum be_refresh_type"
> ?

Renamed.

>
> In be_refresh_get_names(), can you use sysdb_attrs_to_list() to gather
> the names?

Done.

> Is it wise to assume that all object have names and name the getter
> get_names()? Some object might not have names at all, but for instance
> UUIDs.. Also in be_refresh_step() you call get_names but get back a
> "dn".

Fixed.

> Please file tickets to add unit tests for these two modules and make the
> tickets block upstream #1923.

https://fedorahosted.org/sssd/ticket/1939 be_ptask
https://fedorahosted.org/sssd/ticket/1940 be_refresh

> Nitpick:
>> +    filter = talloc_asprintf(tmp_ctx, "(&(%s<=%lld))",
>> +                             SYSDB_CACHE_EXPIRE, (long long)now);
>                                                              ^^^
>                                                          missing space

Fixed.

> [PATCH 3/4] back end: add refresh expired records periodic task
>> --- a/src/config/SSSDConfig/__init__.py.in
>> +++ b/src/config/SSSDConfig/__init__.py.in
>> @@ -125,6 +125,7 @@ option_strings = {
>>       'entry_cache_service_timeout' : _('Entry cache timeout length (seconds)'),
>>       'entry_cache_autofs_timeout' : _('Entry cache timeout length (seconds)'),
>>       'entry_cache_sudo_timeout' : _('Entry cache timeout length (seconds)'),
>> +    'refresh_expired_interval' : _('How often should expired rules be refreshed in background'),
>
> We should use "objects" or "entries" instead of rules.

Fixed.

> Can you explain the "circular dependency" here? I think you already told
> me in person, but I forgot..

dp_backend.h defines struct be_ctx.

dp_ptask.h uses struct be_ctx.
dp_refresh.h uses struct be_ctx.


     dp_backend.h <---- dp_ptask.h
        | ^                ^
        | |                |
        v |                |
    dp_refresh.h -----------

data_provider_be.c includes all three of them (it needs to create ptask 
for be_refresh and remember it in be_ctx).

>> --- a/src/providers/dp_ptask.h
>> +++ b/src/providers/dp_ptask.h
>> @@ -27,6 +27,9 @@
>>
>>   #include "providers/dp_backend.h"
>>
>> +/* solve circular dependency */
>> +struct be_ctx;
>> +
>>   struct be_ptask;
>>
>>   /**
>> diff --git a/src/providers/dp_refresh.h b/src/providers/dp_refresh.h
>> index 54c6aac99866415d4f33f61ac05d487f71b8c49c..d93a932637804dfbf743c395af3f8e640f79c12f 100644
>> --- a/src/providers/dp_refresh.h
>> +++ b/src/providers/dp_refresh.h
>> @@ -27,6 +27,9 @@
>>   #include "providers/dp_backend.h"
>>   #include "providers/dp_ptask.h"
>>
>> +/* solve circular dependency */
>> +struct be_ctx;
>> +
>
>>> [PATCH 4/4] providers: refresh expired netgroups
>>> I don't see one important part implemented -- what if all netgroups
>>> expire at once, then they are all refreshed at once, right? Can we add
>>> some throttling and refresh them in batches with some delay to avoid
>>> starving the back end?
>>
>> I thought we agreed in person that we will first push a basic stupid
>> solution and than fine tune it if necessary - hopefully based on real
>> environment.
>
> OK, I thought there was going to be some proof of concept that we would
> just amend based on testing. Please file a ticket about this, we'll get
> the throttling implemented post-beta.

https://fedorahosted.org/sssd/ticket/1941 throttling

https://fedorahosted.org/sssd/ticket/1942 convert enumeration
https://fedorahosted.org/sssd/ticket/1943 convert sudo
https://fedorahosted.org/sssd/ticket/1944 convert dyndns

Also fix make check problems.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-back-end-periodic-task-API.patch
Type: text/x-patch
Size: 15250 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130523/54dee429/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-back-end-periodical-refresh-of-expired-records-API.patch
Type: text/x-patch
Size: 12240 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130523/54dee429/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-back-end-add-refresh-expired-records-periodic-task.patch
Type: text/x-patch
Size: 9906 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130523/54dee429/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-providers-refresh-expired-netgroups.patch
Type: text/x-patch
Size: 9400 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130523/54dee429/attachment-0003.bin>


More information about the sssd-devel mailing list