[SSSD] [PATCH] implement cleanup task

Simo Sorce ssorce at redhat.com
Mon Nov 9 22:32:39 UTC 2009


On Mon, 2009-11-09 at 15:57 -0500, Stephen Gallagher wrote:
> On 11/09/2009 02:20 PM, Simo Sorce wrote:
>  
> > So here are 2 patches:
> > 0001 add some functions I needed for the task to sysdb and reworks a few
> > others
> 
> Nack.
> sysdb_delete_user_send() should check whether both name and UID are
> specified and fail if they are. Right now, if they are both set, we are
> silently only using the name. Similarly, if neither is set (name==NULL
> and uid==0) we should fail.
> 
> Same comments about sysdb_delete_group_send().
> 
> Otherwise it looks fine.

Ok I added code to verify that the uid/gid match the name being
searched.

> > 
> > 0002 the actual cleanup task files.
> 
> Nack.
> 
> Please update manpages for ldap_purge_cache timeout, as well as the
> SSSDConfig API configuration files.

Like for other timeouts I'd like to keep this undocumented for now.
I added the option for the API configuration file.

> I don't like using SDAP_CACHE_PURGE_TIMEOUT for both the interval
> between purge attempts AND the timeout for the purge running. Please add
> one second to the interval time. Otherwise we're going to start queuing
> up transaction waits on subsequent timeout tries if for some reason the
> remote server is being very slow or unresponsive to our requests. Over a
> long enough period of time, this could be a genuine resource problem.
> I'd much prefer to guarantee that only one such request can be running
> at any given time.

No this can't happen, new runs are not schedule until the previous run
is over.

> The general approach looks fine to me, though I see a lot of duplication
> between the cleanup_users_send() and cleanup_groups_send() functions and
> I can't help wondering if there's a way that these could be made into a
> single set of functions.

I know but I prefer to do things right first and optimize later,
although there is some duplication here I preferred to keep things
simpler than trying to add switch/case or if/else along the code to
address all the differences.

> Also, can you add a set of tests to the sysdb-tests to manually set some
> cache expiration times to "1" and verify that the cleanup task clears
> them appropriately. (For regression-detection)

I'll open a bug to add sysdb-tests for these cases.

Also while checking the enumeration/cleanup code about the tiemouts I
find out there where 2 places where the wrong timeout being set. That
would have caused busy loops, because we were telling the code to
schedule the next task for "now" in case we were offline, oops :)

I also changed the cleanup send function to take a memory context so
that when called from the enumeration task it will actually be a child
of that task (therefore it will be killed if the enum task timeout
kicks-in).

Simo.


-- 
Simo Sorce * Red Hat, Inc * New York
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Refatctor-delete-functions-and-add-a-few.patch
Type: text/x-patch
Size: 39145 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20091109/94287cce/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-cleanup-task.patch
Type: text/x-patch
Size: 52362 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20091109/94287cce/attachment-0001.bin>


More information about the sssd-devel mailing list