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

Jakub Hrozek jakub.hrozek at gmail.com
Sun Jun 2 22:17:23 UTC 2013


On Thu, 2013-05-23 at 11:46 +0200, Pavel Březina wrote:
> 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.

Thank you, I like the new code. I re-read the patches again and actually
tested them quite a lot this time. 

Here are some more comments:

[PATCH 1/4] back end: periodic task API
Here I only have minor comments. I think it would be defensive to add a
NULL check for "task" to be_ptask_online_cb() and maybe even
be_ptask_destructor(). The reason is that the task is now (correctly)
owned by the called but especially the online callback might be called
based on external event.

In be_ptask_create() I think assigning to "_task" should be moved to
when "EOK" is assigned to "ret" to only modify external variable when
the function succeeds.

[PATCH 2/4] back end: periodical refresh of expired records API
In be_refresh_add_cb() I agree with checking the parameters as it's
quite a generic API, but you could also check "enum be_refresh_type
type" for being lesser than BE_REFRESH_TYPE_SENTINEL and return EINVAL
(or a new error code) otherwise.

I wonder if it would be cleaner to move the "talloc_steal(subreq,
values);" after creating the subreq and before setting the callback to
make sure the values are always owned by subreq? It probably doesn't
matter that much from functional point of view as values is otherwise
owned by state, but it would make the code more consistent to me.

I think that EAGAIN is mishandled in be_refresh_done(). The done label
errors out in any case, but EAGAIN would be returned in case there was
more entities to return:

diff --git a/src/providers/dp_refresh.c b/src/providers/dp_refresh.c
index b4e4cb6..52b564a 100644
--- a/src/providers/dp_refresh.c
+++ b/src/providers/dp_refresh.c
@@ -288,14 +288,16 @@ static void be_refresh_done(struct tevent_req
*subreq)
     }

     ret = be_refresh_step(req);
+    if (ret == EOK) {
+        tevent_req_done(req);
+        return;
+    }

 done:
-    if (ret != EOK) {
+    if (ret != EOK && ret != EAGAIN) {
         tevent_req_error(req, ret);
         return;
     }
-
-    tevent_req_done(req);
 }

[PATCH 3/4] back end: add refresh expired records periodic task
The patch looks good to me, but I'm wondering about the defaults of
be_ptask_create. I would suggest making them shorter, maybe even half of
the current values. 

[PATCH 4/4] providers: refresh expired netgroups
EAGAIN is mishandled in this patch, too. I did the following change to
make the refresh work correctly:

diff --git a/src/providers/ldap/sdap_refresh.c
b/src/providers/ldap/sdap_refresh.c
index b564eca..e9fd948 100644
--- a/src/providers/ldap/sdap_refresh.c
+++ b/src/providers/ldap/sdap_refresh.c
@@ -138,14 +138,17 @@ static void sdap_refresh_netgroups_done(struct
tevent_req *subreq)
     }
 
     ret = sdap_refresh_netgroups_step(req);
+    if (ret == EOK) {
+        tevent_req_done(req);
+        return;
+    }
+
 
 done:
-    if (ret != EOK) {
+    if (ret != EOK && ret != EAGAIN) {
         tevent_req_error(req, ret);
         return;
     }
-
-    tevent_req_done(req);
 }

I also need to take another look at the responder caching, because in my
tests with about 1000 netgroups, I was still getting the data provider
contacted even if I set relatively long netgroup timeout and short
background refresh. But with the above changes, the background refresh
seems to have been working fine.




More information about the sssd-devel mailing list