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

Jakub Hrozek jhrozek at redhat.com
Tue May 21 10:06:25 UTC 2013


On Wed, May 15, 2013 at 11:45:47AM +0200, Pavel Březina wrote:

I have a couple of questions.

[PATCH 1/4] back end: periodic task API
What is the purpose of BE_PTASK_OFFLINE_SKIP? Why not simply let the
back end go online and re-enable the task instead?

I think you should either always store last_execution time or introduce
last_attempt. The reason is that if back end was offline for a long
time and then be_ptask_schedule was called with
BE_PTASK_SCHEDULE_FROM_LAST then the task might be scheduled in the
past. I'm not sure how tevent behaves in this respect.

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.

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

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

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

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".

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

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

[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.

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

> --- 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?



More information about the sssd-devel mailing list