[PATCH] LDAP/AD: do not resolve group members during tokenGroups request

[Sumit Bose]

Hi,

with this patch I hope to improve the initgroup-request performance
especially is the user is a member of many nested groups. Currently all
groups the user is a member of which are not in the cache are resolved
completely with all members and nested groups. With this patch only the
group object is looked up to determine the type of the group, POSIX GID
(if any), SID, ... Resolving all members is deferred until a request to
resolve this specific groups is received. This should speed up plain
getgrouplist() calls e.g. used by 'id -G'. Please note that this will
not improve the overall speed of a 'id' call without any options because
in this case after calling getgrouplist() 'id' will call getgrgid() for
every GID returned by getgrouplist(). So in this case all groups are
resolved completely as well. But the overall time is now better spread
over many request and chances for timeout should be much lower.

bye,
Sumid

[Jakub Hrozek]

On Tue, Mar 10, 2015 at 03:43:41PM +0100, Sumit Bose wrote:
>  Hi,
>  
>  with this patch I hope to improve the initgroup-request performance
>  especially is the user is a member of many nested groups. Currently all
>  groups the user is a member of which are not in the cache are resolved
>  completely with all members and nested groups. With this patch only the
>  group object is looked up to determine the type of the group, POSIX GID
>  (if any), SID, ... Resolving all members is deferred until a request to
>  resolve this specific groups is received. This should speed up plain
>  getgrouplist() calls e.g. used by 'id -G'. Please note that this will
>  not improve the overall speed of a 'id' call without any options because
>  in this case after calling getgrouplist() 'id' will call getgrgid() for
>  every GID returned by getgrouplist(). So in this case all groups are
>  resolved completely as well. But the overall time is now better spread
>  over many request and chances for timeout should be much lower.
>  
>  bye,
>  Sumid 
ACK

What we might also want to do in future is to first just gather the
groups and then save them to conserve sysdb transactions. But the ticket
is an improvement on its own.

[Jakub Hrozek]

On Mon, Mar 16, 2015 at 03:07:44PM +0100, Jakub Hrozek wrote:
>  On Tue, Mar 10, 2015 at 03:43:41PM +0100, Sumit Bose wrote:
>  > Hi,
>  > 
>  > with this patch I hope to improve the initgroup-request performance
>  > especially is the user is a member of many nested groups. Currently all
>  > groups the user is a member of which are not in the cache are resolved
>  > completely with all members and nested groups. With this patch only the
>  > group object is looked up to determine the type of the group, POSIX GID
>  > (if any), SID, ... Resolving all members is deferred until a request to
>  > resolve this specific groups is received. This should speed up plain
>  > getgrouplist() calls e.g. used by 'id -G'. Please note that this will
>  > not improve the overall speed of a 'id' call without any options because
>  > in this case after calling getgrouplist() 'id' will call getgrgid() for
>  > every GID returned by getgrouplist(). So in this case all groups are
>  > resolved completely as well. But the overall time is now better spread
>  > over many request and chances for timeout should be much lower.
>  > 
>  > bye,
>  > Sumid
>  
>  ACK
>  
>  What we might also want to do in future is to first just gather the
>  groups and then save them to conserve sysdb transactions. But the ticket
>  is an improvement on its own. 
btw here is the CI link:
http://sssd-ci.duckdns.org/logs/job/10/97/summary.html

[Lukas Slebodnik]

On (16/03/15 15:07), Jakub Hrozek wrote:
> On Tue, Mar 10, 2015 at 03:43:41PM +0100, Sumit Bose wrote:
> > Hi,
> > 
> > with this patch I hope to improve the initgroup-request performance
> > especially is the user is a member of many nested groups. Currently all
> > groups the user is a member of which are not in the cache are resolved
> > completely with all members and nested groups. With this patch only the
> > group object is looked up to determine the type of the group, POSIX GID
> > (if any), SID, ... Resolving all members is deferred until a request to
> > resolve this specific groups is received. This should speed up plain
> > getgrouplist() calls e.g. used by 'id -G'. Please note that this will
> > not improve the overall speed of a 'id' call without any options because
> > in this case after calling getgrouplist() 'id' will call getgrgid() for
> > every GID returned by getgrouplist(). So in this case all groups are
> > resolved completely as well. But the overall time is now better spread
> > over many request and chances for timeout should be much lower.
> > 
> > bye,
> > Sumid
>
> ACK
>
> What we might also want to do in future is to first just gather the
> groups and then save them to conserve sysdb transactions. But the ticket
> is an improvement on its own. 
Shall we mention in release notes that "id user" and "id -G user"
needn't
return the same results?

LS

[Sumit Bose]

On Mon, Mar 16, 2015 at 04:03:00PM +0100, Lukas Slebodnik wrote:
>  On (16/03/15 15:07), Jakub Hrozek wrote:
>  >On Tue, Mar 10, 2015 at 03:43:41PM +0100, Sumit Bose wrote:
>  >> Hi,
>  >> 
>  >> with this patch I hope to improve the initgroup-request performance
>  >> especially is the user is a member of many nested groups. Currently all
>  >> groups the user is a member of which are not in the cache are resolved
>  >> completely with all members and nested groups. With this patch only the
>  >> group object is looked up to determine the type of the group, POSIX GID
>  >> (if any), SID, ... Resolving all members is deferred until a request to
>  >> resolve this specific groups is received. This should speed up plain
>  >> getgrouplist() calls e.g. used by 'id -G'. Please note that this will
>  >> not improve the overall speed of a 'id' call without any options
> because
>  >> in this case after calling getgrouplist() 'id' will call getgrgid() for
>  >> every GID returned by getgrouplist(). So in this case all groups are
>  >> resolved completely as well. But the overall time is now better spread
>  >> over many request and chances for timeout should be much lower.
>  >> 
>  >> bye,
>  >> Sumid
>  >
>  >ACK
>  >
>  >What we might also want to do in future is to first just gather the
>  >groups and then save them to conserve sysdb transactions. But the ticket
>  >is an improvement on its own.
>  
>  Shall we mention in release notes that "id user" and "id -G user"
> needn't
>  return the same results? 
I'm not sure what you mean. Both should return the same list of groups,
plain id would just need more time with an empty cache because if will
completely resolve all groups as well.

bye,
Sumit

>  
>  LS
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Mar 16, 2015 at 04:03:00PM +0100, Lukas Slebodnik wrote:
>  On (16/03/15 15:07), Jakub Hrozek wrote:
>  >On Tue, Mar 10, 2015 at 03:43:41PM +0100, Sumit Bose wrote:
>  >> Hi,
>  >> 
>  >> with this patch I hope to improve the initgroup-request performance
>  >> especially is the user is a member of many nested groups. Currently all
>  >> groups the user is a member of which are not in the cache are resolved
>  >> completely with all members and nested groups. With this patch only the
>  >> group object is looked up to determine the type of the group, POSIX GID
>  >> (if any), SID, ... Resolving all members is deferred until a request to
>  >> resolve this specific groups is received. This should speed up plain
>  >> getgrouplist() calls e.g. used by 'id -G'. Please note that this will
>  >> not improve the overall speed of a 'id' call without any options
> because
>  >> in this case after calling getgrouplist() 'id' will call getgrgid() for
>  >> every GID returned by getgrouplist(). So in this case all groups are
>  >> resolved completely as well. But the overall time is now better spread
>  >> over many request and chances for timeout should be much lower.
>  >> 
>  >> bye,
>  >> Sumid
>  >
>  >ACK
>  >
>  >What we might also want to do in future is to first just gather the
>  >groups and then save them to conserve sysdb transactions. But the ticket
>  >is an improvement on its own.
>  
>  Shall we mention in release notes that "id user" and "id -G user"
> needn't
>  return the same results? 
Have you found a case that would differ actually? I hope it doesn't,
since TGs should already contain all groups.

[Jakub Hrozek]

On Mon, Mar 16, 2015 at 03:07:44PM +0100, Jakub Hrozek wrote:
>  On Tue, Mar 10, 2015 at 03:43:41PM +0100, Sumit Bose wrote:
>  > Hi,
>  > 
>  > with this patch I hope to improve the initgroup-request performance
>  > especially is the user is a member of many nested groups. Currently all
>  > groups the user is a member of which are not in the cache are resolved
>  > completely with all members and nested groups. With this patch only the
>  > group object is looked up to determine the type of the group, POSIX GID
>  > (if any), SID, ... Resolving all members is deferred until a request to
>  > resolve this specific groups is received. This should speed up plain
>  > getgrouplist() calls e.g. used by 'id -G'. Please note that this will
>  > not improve the overall speed of a 'id' call without any options because
>  > in this case after calling getgrouplist() 'id' will call getgrgid() for
>  > every GID returned by getgrouplist(). So in this case all groups are
>  > resolved completely as well. But the overall time is now better spread
>  > over many request and chances for timeout should be much lower.
>  > 
>  > bye,
>  > Sumid
>  
>  ACK
>  
>  What we might also want to do in future is to first just gather the
>  groups and then save them to conserve sysdb transactions. But the ticket
>  is an improvement on its own. 
* master: d81d8d3dc151ebc95cd0e3f3b14c1cdaa48980f1
* sssd-1-12: b8d9eca0d9469c1209161b31a0109d8e4ea2868c

[Pavel Reichl]

On 03/10/2015 03:43 PM, Sumit Bose wrote:
>  @@ -1982,6 +1986,36 @@ static void sdap_get_groups_process(struct
> tevent_req *subreq)
>            return;
>        }
>    
>  +    if (state->no_members) {
>  +        ret = sysdb_attrs_primary_name_list(state->sysdb, state,
>  +                                state->groups, state->count,
>  +                                state->opts->group_map[SDAP_AT_GROUP_NAME].name,
>  +                                &groupnamelist);
>  +        if (ret != EOK) {
>  +            DEBUG(SSSDBG_OP_FAILURE,
>  +                  "sysdb_attrs_primary_name_list failed.\n");
>  +            tevent_req_error(req, ret);
>  +            return;
>  +        }
>  +
>  +        ret = sdap_add_incomplete_groups(state->sysdb, state->dom,
> state->opts,
>  +                                         groupnamelist, state->groups,
>  +                                         state->count);
>  +        if (ret == EOK) {
>  +            DEBUG(SSSDBG_TRACE_LIBS,
>  +                  "Reading only group data without members successful.\n");
>  +            tevent_req_done(req);
>  +        } else {
>  +            DEBUG(SSSDBG_OP_FAILURE, "sdap_add_incomplete_groups failed.\n");
>  +            tevent_req_error(req, ret);
>  +        }
>  +        return; 
unused code starts here
>  +
>  +        ret = sdap_save_groups(state, state->sysdb, state->dom, state->opts,
>  +                               state->groups, state->count, false,
>  +                               NULL, true, NULL);
>  +    }
>  + 
Hello,

coverity found this unused code - is it OK to just remove 
sdap_save_groups() ?

Thanks!

[Sumit Bose]

On Thu, Mar 19, 2015 at 03:18:11PM +0100, Pavel Reichl wrote:
>  
>  
>  On 03/10/2015 03:43 PM, Sumit Bose wrote:
>  >@@ -1982,6 +1986,36 @@ static void sdap_get_groups_process(struct tevent_req
> *subreq)
>  >          return;
>  >      }
>  >+    if (state->no_members) {
>  >+        ret = sysdb_attrs_primary_name_list(state->sysdb, state,
>  >+                                state->groups, state->count,
>  >+                               
> state->opts->group_map[SDAP_AT_GROUP_NAME].name,
>  >+                                &groupnamelist);
>  >+        if (ret != EOK) {
>  >+            DEBUG(SSSDBG_OP_FAILURE,
>  >+                  "sysdb_attrs_primary_name_list failed.\n");
>  >+            tevent_req_error(req, ret);
>  >+            return;
>  >+        }
>  >+
>  >+        ret = sdap_add_incomplete_groups(state->sysdb, state->dom,
> state->opts,
>  >+                                         groupnamelist, state->groups,
>  >+                                         state->count);
>  >+        if (ret == EOK) {
>  >+            DEBUG(SSSDBG_TRACE_LIBS,
>  >+                  "Reading only group data without members
> successful.\n");
>  >+            tevent_req_done(req);
>  >+        } else {
>  >+            DEBUG(SSSDBG_OP_FAILURE, "sdap_add_incomplete_groups
> failed.\n");
>  >+            tevent_req_error(req, ret);
>  >+        }
>  >+        return;
>  unused code starts here
>  >+
>  >+        ret = sdap_save_groups(state, state->sysdb, state->dom,
> state->opts,
>  >+                               state->groups, state->count, false,
>  >+                               NULL, true, NULL);
>  >+    }
>  >+
>  Hello,
>  
>  coverity found this unused code - is it OK to just remove sdap_save_groups() 
yes, please remove it. While you are at it I wonder if you can fix the
debug message (s/Reading/Writing/) above as well?

bye,
Sumit

>  ?
>  
>  Thanks!
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel