Hi,
Is it really the intended behaviour of the sssd LDAP backend (I am running the current code from the master branch) to only return the group members that are already cached in sysdb and to silently ignore everything else? E.g. when I start sssd with empty caches and do a "getent group <random-ldap-group>" I will only get back the group without any members. Somehow I think this can't be intended :)
I have started working on a patch to let sssd look up the non-cached users via LDAP (and save them into the cache). Find it attached. Note: That patch is not really complete (e.g. it doesn't handle rfc2307 groups correctly). But before putting more effort into this I like to make sure that I am not trying to fix a "feature" here.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/09/2010 09:14 AM, Ralf Haferkamp wrote:
Hi,
Is it really the intended behaviour of the sssd LDAP backend (I am running the current code from the master branch) to only return the group members that are already cached in sysdb and to silently ignore everything else? E.g. when I start sssd with empty caches and do a "getent group <random-ldap-group>" I will only get back the group without any members. Somehow I think this can't be intended :)
I have started working on a patch to let sssd look up the non-cached users via LDAP (and save them into the cache). Find it attached. Note: That patch is not really complete (e.g. it doesn't handle rfc2307 groups correctly). But before putting more effort into this I like to make sure that I am not trying to fix a "feature" here.
No, it is not intentional that groups should be missing users. This is definitely a bug. Please file a ticket upstream.
I'll take a look at this patch soon, but I can't promise that I'll get to it this week (I'm a bit swamped).
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Thu, 09 Sep 2010 09:18:12 -0400 Stephen Gallagher sgallagh@redhat.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/09/2010 09:14 AM, Ralf Haferkamp wrote:
Hi,
Is it really the intended behaviour of the sssd LDAP backend (I am running the current code from the master branch) to only return the group members that are already cached in sysdb and to silently ignore everything else? E.g. when I start sssd with empty caches and do a "getent group <random-ldap-group>" I will only get back the group without any members. Somehow I think this can't be intended :)
I have started working on a patch to let sssd look up the non-cached users via LDAP (and save them into the cache). Find it attached. Note: That patch is not really complete (e.g. it doesn't handle rfc2307 groups correctly). But before putting more effort into this I like to make sure that I am not trying to fix a "feature" here.
No, it is not intentional that groups should be missing users. This is definitely a bug. Please file a ticket upstream.
It is intended if enumerations are off. Thee reason is that you may end up effectively doing full user enumerations otherwise if you have a big group that contain all users.
Not only that but it would be an inefficient enumeration as it would be repeated multiple times for each group.
And if you have a *lot* of users this would defeat the point of disabling enumerations, making performances actually worse.
So please do not change this without proper discussion.
Simo.
On Thursday 09 September 2010 15:59:46 Simo Sorce wrote:
On Thu, 09 Sep 2010 09:18:12 -0400
Stephen Gallagher sgallagh@redhat.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/09/2010 09:14 AM, Ralf Haferkamp wrote:
Hi,
Is it really the intended behaviour of the sssd LDAP backend (I am running the current code from the master branch) to only return the group members that are already cached in sysdb and to silently ignore everything else? E.g. when I start sssd with empty caches and do a "getent group <random-ldap-group>" I will only get back the group without any members. Somehow I think this can't be intended :)
I have started working on a patch to let sssd look up the non-cached users via LDAP (and save them into the cache). Find it attached. Note: That patch is not really complete (e.g. it doesn't handle rfc2307 groups correctly). But before putting more effort into this I like to make sure that I am not trying to fix a "feature" here.
No, it is not intentional that groups should be missing users. This is definitely a bug. Please file a ticket upstream.
It is intended if enumerations are off. Thee reason is that you may end up effectively doing full user enumerations otherwise if you have a big group that contain all users.
Then it should probably be possible to disable that feature separately from enumeration. While, turning enumerations off by default makes sense to me, I think returning incomplete results when resolving groups by default goes a bit too far.
BTW, I just found out that the behaviour of getgrouplist()/initgroups() is similar currently. It will only return the groups that are already present in the cache. That means in many cases it will return nothing. (Or just the gid you supplied via the group argument).
Not only that but it would be an inefficient enumeration as it would be repeated multiple times for each group.
Why is that? Users should of course be saved in the sysdb after being read from LDAP. If you read another group which has that user as a member the result is taken from the cache of course. (That's what my patch tries to do at least, though I admit there is still room for optimization)
And if you have a *lot* of users this would defeat the point of disabling enumerations, making performances actually worse.
So please do not change this without proper discussion.
regards, Ralf
On Thu, 9 Sep 2010 16:46:35 +0200 Ralf Haferkamp rhafer@suse.de wrote:
On Thursday 09 September 2010 15:59:46 Simo Sorce wrote:
On Thu, 09 Sep 2010 09:18:12 -0400
Stephen Gallagher sgallagh@redhat.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/09/2010 09:14 AM, Ralf Haferkamp wrote:
Hi,
Is it really the intended behaviour of the sssd LDAP backend (I am running the current code from the master branch) to only return the group members that are already cached in sysdb and to silently ignore everything else? E.g. when I start sssd with empty caches and do a "getent group <random-ldap-group>" I will only get back the group without any members. Somehow I think this can't be intended :)
I have started working on a patch to let sssd look up the non-cached users via LDAP (and save them into the cache). Find it attached. Note: That patch is not really complete (e.g. it doesn't handle rfc2307 groups correctly). But before putting more effort into this I like to make sure that I am not trying to fix a "feature" here.
No, it is not intentional that groups should be missing users. This is definitely a bug. Please file a ticket upstream.
It is intended if enumerations are off. Thee reason is that you may end up effectively doing full user enumerations otherwise if you have a big group that contain all users.
Then it should probably be possible to disable that feature separately from enumeration. While, turning enumerations off by default makes sense to me, I think returning incomplete results when resolving groups by default goes a bit too far.
It depends on the point of view. For the specific logged in user the groups he is interested in are fully update wrt his own membership.
It is true you may not see other members unless they log in, but why would that be a problem ? If they do not log in they are not affected by group memberships anyway.
BTW, I just found out that the behaviour of getgrouplist()/initgroups() is similar currently. It will only return the groups that are already present in the cache. That means in many cases it will return nothing. (Or just the gid you supplied via the group argument).
Initgroups will always call the LDAP server, and will make sure all groups the user is member of are available.
Not only that but it would be an inefficient enumeration as it would be repeated multiple times for each group.
Why is that? Users should of course be saved in the sysdb after being read from LDAP. If you read another group which has that user as a member the result is taken from the cache of course. (That's what my patch tries to do at least, though I admit there is still room for optimization)
In order to save a user you have to look it up. so if you have a group with 50k users in it you will have to do 50k lookups. Multiply this for N groups.
If you need enumeration completeness then you should just have enumerations turned on IMO.
Now thinking a bit more there is probably a way to avoid the lookups. You can avoid the 50k lookups by actually *not* fetching each member from LDAP but create fake entries that are marked as expired so they are immediately refreshed with the right data if they are ever requested from the user space.
But this technique has some pretty sever limits when using rfc2307bis and nested groups. (should work just fine for classic rfc2307)
One limit is that you don't know whether a DN is a user or a Group. It is true that we can adopt some heuristics like examining the DN components to get a clue, but they would be pretty much schema dependent and would fail consistently in some scenarios. So you may fail to properly unroll nested groups.
The other is that the actual user name may not be contained within the DN. if the DN includes the uid attribute as part of it does, but if the CN is used (like against AD where the samAccoutnName attribute is used as the OS user name) then you do not have the user name you need to present in the group as member. So, again, it would work only in some setups.
Another concern is about how to properly do caching. When we did the initgroups call I used to fully resolve groups, we removed that code because it would make initgroups unbearably slow as it would end up doing way too many lookups. So we need to make sure initgroups can create/manipulate groups and know when a group can be considered up to date, and yet at the same time mark it as "incomplete" or something like that for getent group <group> calls (and vice versa ?)
I guess we could have a third option to manage this, and allow people full group (all member users/groups(recursive)) lookups if they really want/need it. But it has a lot of subtle interactions we need to care about to avoid perf. side effects.
May be something like: fully_resolve_groups defaulting to false ?
Simo.
On Thu, Sep 9, 2010 at 8:48 AM, Simo Sorce ssorce@redhat.com wrote:
On Thu, 9 Sep 2010 16:46:35 +0200 Ralf Haferkamp rhafer@suse.de wrote:
On Thursday 09 September 2010 15:59:46 Simo Sorce wrote:
On Thu, 09 Sep 2010 09:18:12 -0400
Stephen Gallagher sgallagh@redhat.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/09/2010 09:14 AM, Ralf Haferkamp wrote:
Hi,
Is it really the intended behaviour of the sssd LDAP backend (I am running the current code from the master branch) to only return the group members that are already cached in sysdb and to silently ignore everything else? E.g. when I start sssd with empty caches and do a "getent group <random-ldap-group>" I will only get back the group without any members. Somehow I think this can't be intended :)
I have started working on a patch to let sssd look up the non-cached users via LDAP (and save them into the cache). Find it attached. Note: That patch is not really complete (e.g. it doesn't handle rfc2307 groups correctly). But before putting more effort into this I like to make sure that I am not trying to fix a "feature" here.
No, it is not intentional that groups should be missing users. This is definitely a bug. Please file a ticket upstream.
It is intended if enumerations are off. Thee reason is that you may end up effectively doing full user enumerations otherwise if you have a big group that contain all users.
Then it should probably be possible to disable that feature separately from enumeration. While, turning enumerations off by default makes sense to me, I think returning incomplete results when resolving groups by default goes a bit too far.
It depends on the point of view. For the specific logged in user the groups he is interested in are fully update wrt his own membership.
It is true you may not see other members unless they log in, but why would that be a problem ? If they do not log in they are not affected by group memberships anyway.
BTW, I just found out that the behaviour of getgrouplist()/initgroups() is similar currently. It will only return the groups that are already present in the cache. That means in many cases it will return nothing. (Or just the gid you supplied via the group argument).
Initgroups will always call the LDAP server, and will make sure all groups the user is member of are available.
Not only that but it would be an inefficient enumeration as it would be repeated multiple times for each group.
Why is that? Users should of course be saved in the sysdb after being read from LDAP. If you read another group which has that user as a member the result is taken from the cache of course. (That's what my patch tries to do at least, though I admit there is still room for optimization)
In order to save a user you have to look it up. so if you have a group with 50k users in it you will have to do 50k lookups. Multiply this for N groups.
If you need enumeration completeness then you should just have enumerations turned on IMO.
Now thinking a bit more there is probably a way to avoid the lookups. You can avoid the 50k lookups by actually *not* fetching each member from LDAP but create fake entries that are marked as expired so they are immediately refreshed with the right data if they are ever requested from the user space.
But this technique has some pretty sever limits when using rfc2307bis and nested groups. (should work just fine for classic rfc2307)
One limit is that you don't know whether a DN is a user or a Group. It is true that we can adopt some heuristics like examining the DN components to get a clue, but they would be pretty much schema dependent and would fail consistently in some scenarios. So you may fail to properly unroll nested groups.
The other is that the actual user name may not be contained within the DN. if the DN includes the uid attribute as part of it does, but if the CN is used (like against AD where the samAccoutnName attribute is used as the OS user name) then you do not have the user name you need to present in the group as member. So, again, it would work only in some setups.
Another concern is about how to properly do caching. When we did the initgroups call I used to fully resolve groups, we removed that code because it would make initgroups unbearably slow as it would end up doing way too many lookups. So we need to make sure initgroups can create/manipulate groups and know when a group can be considered up to date, and yet at the same time mark it as "incomplete" or something like that for getent group <group> calls (and vice versa ?)
I guess we could have a third option to manage this, and allow people full group (all member users/groups(recursive)) lookups if they really want/need it. But it has a lot of subtle interactions we need to care about to avoid perf. side effects.
May be something like: fully_resolve_groups defaulting to false ?
Having enumerate = False but still enumerate the members of each group (when the group is looked up) would be nice. Some users got really confused with the output of getent group showed them in the right groups, but not all of the members.
Remember that while optimizing for hte 50k member group is important, it is likely not the most common usage scenario.
On Thu, 9 Sep 2010 08:57:37 -0700 Jeff Schroeder jeffschroeder@computer.org wrote:
On Thu, Sep 9, 2010 at 8:48 AM, Simo Sorce ssorce@redhat.com wrote:
On Thu, 9 Sep 2010 16:46:35 +0200 Ralf Haferkamp rhafer@suse.de wrote:
On Thursday 09 September 2010 15:59:46 Simo Sorce wrote:
On Thu, 09 Sep 2010 09:18:12 -0400
Stephen Gallagher sgallagh@redhat.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/09/2010 09:14 AM, Ralf Haferkamp wrote:
Hi,
Is it really the intended behaviour of the sssd LDAP backend (I am running the current code from the master branch) to only return the group members that are already cached in sysdb and to silently ignore everything else? E.g. when I start sssd with empty caches and do a "getent group <random-ldap-group>" I will only get back the group without any members. Somehow I think this can't be intended :)
I have started working on a patch to let sssd look up the non-cached users via LDAP (and save them into the cache). Find it attached. Note: That patch is not really complete (e.g. it doesn't handle rfc2307 groups correctly). But before putting more effort into this I like to make sure that I am not trying to fix a "feature" here.
No, it is not intentional that groups should be missing users. This is definitely a bug. Please file a ticket upstream.
It is intended if enumerations are off. Thee reason is that you may end up effectively doing full user enumerations otherwise if you have a big group that contain all users.
Then it should probably be possible to disable that feature separately from enumeration. While, turning enumerations off by default makes sense to me, I think returning incomplete results when resolving groups by default goes a bit too far.
It depends on the point of view. For the specific logged in user the groups he is interested in are fully update wrt his own membership.
It is true you may not see other members unless they log in, but why would that be a problem ? If they do not log in they are not affected by group memberships anyway.
BTW, I just found out that the behaviour of getgrouplist()/initgroups() is similar currently. It will only return the groups that are already present in the cache. That means in many cases it will return nothing. (Or just the gid you supplied via the group argument).
Initgroups will always call the LDAP server, and will make sure all groups the user is member of are available.
Not only that but it would be an inefficient enumeration as it would be repeated multiple times for each group.
Why is that? Users should of course be saved in the sysdb after being read from LDAP. If you read another group which has that user as a member the result is taken from the cache of course. (That's what my patch tries to do at least, though I admit there is still room for optimization)
In order to save a user you have to look it up. so if you have a group with 50k users in it you will have to do 50k lookups. Multiply this for N groups.
If you need enumeration completeness then you should just have enumerations turned on IMO.
Now thinking a bit more there is probably a way to avoid the lookups. You can avoid the 50k lookups by actually *not* fetching each member from LDAP but create fake entries that are marked as expired so they are immediately refreshed with the right data if they are ever requested from the user space.
But this technique has some pretty sever limits when using rfc2307bis and nested groups. (should work just fine for classic rfc2307)
One limit is that you don't know whether a DN is a user or a Group. It is true that we can adopt some heuristics like examining the DN components to get a clue, but they would be pretty much schema dependent and would fail consistently in some scenarios. So you may fail to properly unroll nested groups.
The other is that the actual user name may not be contained within the DN. if the DN includes the uid attribute as part of it does, but if the CN is used (like against AD where the samAccoutnName attribute is used as the OS user name) then you do not have the user name you need to present in the group as member. So, again, it would work only in some setups.
Another concern is about how to properly do caching. When we did the initgroups call I used to fully resolve groups, we removed that code because it would make initgroups unbearably slow as it would end up doing way too many lookups. So we need to make sure initgroups can create/manipulate groups and know when a group can be considered up to date, and yet at the same time mark it as "incomplete" or something like that for getent group <group> calls (and vice versa ?)
I guess we could have a third option to manage this, and allow people full group (all member users/groups(recursive)) lookups if they really want/need it. But it has a lot of subtle interactions we need to care about to avoid perf. side effects.
May be something like: fully_resolve_groups defaulting to false ?
Having enumerate = False but still enumerate the members of each group (when the group is looked up) would be nice. Some users got really confused with the output of getent group showed them in the right groups, but not all of the members.
Remember that while optimizing for hte 50k member group is important, it is likely not the most common usage scenario.
Ok, I was also wondering we can use a slightly different set of heuristics depending on what kind of schema is used.
So we have the following scenarios:
1) If we use rfc2307 classic with memberUid attributes, we can just create the fake/expired users and be done with it.
2) If we use rfc2307bis w/o nested groups we can count the number of members on the group entry and switch to a full user enumeration if the number of members is greater than a defined (potentially user defined) threshold. Assuming a threshold value of 100, if we have 10 members we just do 10 lookups, while, if we have > 100 member we do a full enumeration (it's a single ldap search, and if modifyTimestamp is used also highly optimized after the first search) so we are sure all users are there with the right name.
3) If we use rfc2307bis with nested groups we do the same as in 2, but we add an internal user lookup against the cache we just populated to determine if some members are groups that need further lookups in order to complete the group unrolling operation.
4) If we use IPA (rfc2307bis + nested groups + unrolled members in memberOf attribute), we can do a single search with memberOf=groupDN, as IPa's memberOf fully resolves nested groups. This would return all and only the users members of the group. So if the group has 101 members in a directory of 50k users we do not do a full 50k users enumeration just because we have a group that is 1 above the threshold.
Do I miss any scenario ?
Simo.
On Thursday 09 September 2010 18:33:26 Simo Sorce wrote:
On Thu, 9 Sep 2010 08:57:37 -0700
Jeff Schroeder jeffschroeder@computer.org wrote:
[..]
Having enumerate = False but still enumerate the members of each group (when the group is looked up) would be nice. Some users got really confused with the output of getent group showed them in the right groups, but not all of the members.
Remember that while optimizing for hte 50k member group is important, it is likely not the most common usage scenario.
Ok, I was also wondering we can use a slightly different set of heuristics depending on what kind of schema is used.
So we have the following scenarios:
- If we use rfc2307 classic with memberUid attributes, we can just
create the fake/expired users and be done with it.
Agreed.
- If we use rfc2307bis w/o nested groups we can count the number of
members on the group entry and switch to a full user enumeration if the number of members is greater than a defined (potentially user defined) threshold. Assuming a threshold value of 100, if we have 10 members we just do 10 lookups, while, if we have > 100 member we do a full enumeration (it's a single ldap search, and if modifyTimestamp is used also highly optimized after the first search) so we are sure all users are there with the right name.
I think that's an optimization that could be worth exploring later. In a first iteration I'd just go for implementing the complete group lookup by looking up every single user regardless of how large the group is. (plus the possiblity to completely disable member unrolling through the configuration)
- If we use rfc2307bis with nested groups we do the same as in 2, but
we add an internal user lookup against the cache we just populated to determine if some members are groups that need further lookups in order to complete the group unrolling operation.
Why do an extra cache lookup? You could issue the addtional LDAP lookups right when you receive the entries of the group members. Provided the correct LDAP attributes where requested (i.e. user attributes + groups attributes).
- If we use IPA (rfc2307bis + nested groups + unrolled members in
memberOf attribute), we can do a single search with memberOf=groupDN, as IPa's memberOf fully resolves nested groups. This would return all and only the users members of the group. So if the group has 101 members in a directory of 50k users we do not do a full 50k users enumeration just because we have a group that is 1 above the threshold.
Do I miss any scenario ?
Simo.
regards, Ralf
On Fri, 10 Sep 2010 12:06:25 +0200 Ralf Haferkamp rhafer@suse.de wrote:
On Thursday 09 September 2010 18:33:26 Simo Sorce wrote:
So we have the following scenarios:
- If we use rfc2307 classic with memberUid attributes, we can just
create the fake/expired users and be done with it.
Agreed.
- If we use rfc2307bis w/o nested groups we can count the number of
members on the group entry and switch to a full user enumeration if the number of members is greater than a defined (potentially user defined) threshold. Assuming a threshold value of 100, if we have 10 members we just do 10 lookups, while, if we have > 100 member we do a full enumeration (it's a single ldap search, and if modifyTimestamp is used also highly optimized after the first search) so we are sure all users are there with the right name.
I think that's an optimization that could be worth exploring later. In a first iteration I'd just go for implementing the complete group lookup by looking up every single user regardless of how large the group is. (plus the possiblity to completely disable member unrolling through the configuration)
Ok, although performances will suck so much I bet we will have to implement the optimization pretty soon.
- If we use rfc2307bis with nested groups we do the same as in 2,
but we add an internal user lookup against the cache we just populated to determine if some members are groups that need further lookups in order to complete the group unrolling operation.
Why do an extra cache lookup? You could issue the addtional LDAP lookups right when you receive the entries of the group members. Provided the correct LDAP attributes where requested (i.e. user attributes + groups attributes).
True, it is necessary only when you use the optimization of doing one search for all users. When you lookup each entry individually you just need to keep a list of groups to further search, and simply repeat the operation for each of those groups (need to set a limit to recursion).
- If we use IPA (rfc2307bis + nested groups + unrolled members in
memberOf attribute), we can do a single search with memberOf=groupDN, as IPa's memberOf fully resolves nested groups. This would return all and only the users members of the group. So if the group has 101 members in a directory of 50k users we do not do a full 50k users enumeration just because we have a group that is 1 above the threshold.
Do I miss any scenario ?
Simo.
Simo.
On Friday 10 September 2010 13:48:31 Simo Sorce wrote:
On Fri, 10 Sep 2010 12:06:25 +0200
Ralf Haferkamp rhafer@suse.de wrote:
On Thursday 09 September 2010 18:33:26 Simo Sorce wrote:
So we have the following scenarios:
- If we use rfc2307 classic with memberUid attributes, we can
just create the fake/expired users and be done with it.
Agreed.
- If we use rfc2307bis w/o nested groups we can count the number
of members on the group entry and switch to a full user enumeration if the number of members is greater than a defined (potentially user defined) threshold. Assuming a threshold value of 100, if we have 10 members we just do 10 lookups, while, if we have > 100 member we do a full enumeration (it's a single ldap search, and if modifyTimestamp is used also highly optimized after the first search) so we are sure all users are there with the right name.
I think that's an optimization that could be worth exploring later. In a first iteration I'd just go for implementing the complete group lookup by looking up every single user regardless of how large the group is. (plus the possiblity to completely disable member unrolling through the configuration)
Ok, although performances will suck so much I bet we will have to implement the optimization pretty soon.
After doing some test here, I really wonder if the LDAP lookups are going to be the bottleneck. I just tested a slightly improved version of my previous patch with a group with 500 members (not really big, I know). Doing "getent group" on that group takes about 2sec on my testsystem, which is really slow (nss_ldap needs 0.5sec). But looking closer at that, the LDAP searches and the processing of the search results accounts only for about 160ms of that two seconds. Much more time (more than 1sec) is then spent in saving the user objects and the group in sysdb. Probably it would be worth looking for optimizations in that area first. Other calls (initgroups?) could benefit from that as well.
Note, I already save all users in one transaction instead of starting a new transaction for every user. Also note that, opening the sysdb with LDB_FLG_NOSYNC brings down the time to about 1.3sec.
[..]
regards, Ralf
On Tue, 14 Sep 2010 15:13:19 +0200 Ralf Haferkamp rhafer@suse.de wrote:
On Friday 10 September 2010 13:48:31 Simo Sorce wrote:
On Fri, 10 Sep 2010 12:06:25 +0200
Ralf Haferkamp rhafer@suse.de wrote:
On Thursday 09 September 2010 18:33:26 Simo Sorce wrote:
So we have the following scenarios:
- If we use rfc2307 classic with memberUid attributes, we can
just create the fake/expired users and be done with it.
Agreed.
- If we use rfc2307bis w/o nested groups we can count the
number of members on the group entry and switch to a full user enumeration if the number of members is greater than a defined (potentially user defined) threshold. Assuming a threshold value of 100, if we have 10 members we just do 10 lookups, while, if we have > 100 member we do a full enumeration (it's a single ldap search, and if modifyTimestamp is used also highly optimized after the first search) so we are sure all users are there with the right name.
I think that's an optimization that could be worth exploring later. In a first iteration I'd just go for implementing the complete group lookup by looking up every single user regardless of how large the group is. (plus the possiblity to completely disable member unrolling through the configuration)
Ok, although performances will suck so much I bet we will have to implement the optimization pretty soon.
After doing some test here, I really wonder if the LDAP lookups are going to be the bottleneck. I just tested a slightly improved version of my previous patch with a group with 500 members (not really big, I know). Doing "getent group" on that group takes about 2sec on my testsystem, which is really slow (nss_ldap needs 0.5sec). But looking closer at that, the LDAP searches and the processing of the search results accounts only for about 160ms of that two seconds. Much more time (more than 1sec) is then spent in saving the user objects and the group in sysdb. Probably it would be worth looking for optimizations in that area first. Other calls (initgroups?) could benefit from that as well.
Not that easy unfortunately.
Note, I already save all users in one transaction instead of starting a new transaction for every user. Also note that, opening the sysdb with LDB_FLG_NOSYNC brings down the time to about 1.3sec.
Yes, we have a bit of a bottlneck issue with LDB, because every transaction requires at least to fsyncs. Unfortunately we can't run with LDB_FLG_NOSYNC as that would make it too easy to corrupt the db in case of power loss.
Simo.
[Simo Sorce]
Do I miss any scenario ?
I suspect you missed the scenario we have here at the University of Oslo. We have a LDAP directory (OpenLDAP) with ~120k users, and groups with 20-30k members using rfc2307 classic schema with memberUid attributes. But user searches are limited, and will not return anything if more than some thousand objects are found (not sure but believe the limit is 5000). So looping over all users is not possible. The algorithm chosen should not assume it is possible to look up all objects in LDAP.
When trying to log in using sssd 1.2.1 for a user that is a member of one of the large groups (25k members), logging in take several minutes. :/
We have not found time to test the latest versions yet. It is slowly making its way up to the top of our todo lists. :)
Happy hacking,
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/13/2010 01:37 PM, Petter Reinholdtsen wrote:
When trying to log in using sssd 1.2.1 for a user that is a member of one of the large groups (25k members), logging in take several minutes. :/
I believe this particular slowdown was caused by our previous method of initgroups processing. Steve fixed that for 1.2.2, so you might want to give it a try..
On Mon, 13 Sep 2010 13:37:11 +0200 Petter Reinholdtsen pere@hungry.com wrote:
[Simo Sorce]
Do I miss any scenario ?
I suspect you missed the scenario we have here at the University of Oslo. We have a LDAP directory (OpenLDAP) with ~120k users, and groups with 20-30k members using rfc2307 classic schema with memberUid attributes. But user searches are limited, and will not return anything if more than some thousand objects are found (not sure but believe the limit is 5000). So looping over all users is not possible. The algorithm chosen should not assume it is possible to look up all objects in LDAP.
No this is actually accounted for in the pure rfc2307 scenario, where we do not lookup any user but just create fake entries from the memberUid attribute of the group.
When trying to log in using sssd 1.2.1 for a user that is a member of one of the large groups (25k members), logging in take several minutes. :/
This should be fixed in latest releases.
Simo.
[Simo Sorce]
No this is actually accounted for in the pure rfc2307 scenario, where we do not lookup any user but just create fake entries from the memberUid attribute of the group.
Good.
When trying to log in using sssd 1.2.1 for a user that is a member of one of the large groups (25k members), logging in take several minutes. :/
This should be fixed in latest releases.
I tested with 1.2.3, and the login was quick, but only the primary group was fetched from LDAP. None of the other groups showed up with 'id' after I logged in with a clean cache (reinstalled machine). Not quite sure if that was the intended behaviour, but it is unwanted for us that want to use sssd with NFS. :)
I have yet to test 1.3.x to see if this new problem also exist there.
CC to my debian co-maintainer and co-worker here at the University of Oslo.
Happy hacking,
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/15/2010 03:19 PM, Petter Reinholdtsen wrote:
I tested with 1.2.3, and the login was quick, but only the primary group was fetched from LDAP. None of the other groups showed up with 'id' after I logged in with a clean cache (reinstalled machine). Not quite sure if that was the intended behaviour, but it is unwanted for us that want to use sssd with NFS. :)
I have yet to test 1.3.x to see if this new problem also exist there.
Yeah, I made a mistake when I wrote that speedup and forgot to account for creating dummy entries for groups that don't currently exist on the system.
The idea is that, instead of doing a slow recursive lookup of all groups and members of those groups like we used to, we just looked at what groups this user belongs to and manually added just that entry as a member of the group on the local cache.
I just completely forgot to account for the case where that user's groups hadn't already been cached on the system in some fashion (I forgot to test from a clean cache situation).
Jakub is fixing my mistake as we speak. Expect a patch within a few days.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Thursday 09 September 2010 17:48:09 Simo Sorce wrote:
On Thu, 9 Sep 2010 16:46:35 +0200
Ralf Haferkamp rhafer@suse.de wrote:
[..]
Then it should probably be possible to disable that feature separately from enumeration. While, turning enumerations off by default makes sense to me, I think returning incomplete results when resolving groups by default goes a bit too far.
It depends on the point of view. For the specific logged in user the groups he is interested in are fully update wrt his own membership.
It is true you may not see other members unless they log in, but why would that be a problem ? If they do not log in they are not affected by group memberships anyway.
The user currently logged in might need to see who else is member of the groups he is member of.
BTW, I just found out that the behaviour of getgrouplist()/initgroups() is similar currently. It will only return the groups that are already present in the cache. That means in many cases it will return nothing. (Or just the gid you supplied via the group argument).
Initgroups will always call the LDAP server, and will make sure all groups the user is member of are available.
Ok, there something else seems broken here:
1. When initgroups is called for a user that is currently not cached no LDAP groups are returned. (I guess that's inline with your idea about how sssd should behave) 2. When the user is already cached and rfc2307bis+memberof is used on the Serverside, nothing is returned upon the first initgroups call, but the cache is correctly populated with the groups, a second initgroups call then returns the correct results. So I guess there's a bug here somewhere. Haven't investigated this deeper yet. 3. When using rfc2307 or rfc2307bis (without memberof Attributes) initgroups() seems completely broken.
I'll create bug reports after I was able to take a deeper look. (It seems some of the issues are already reported. See e.g. https://fedorahosted.org/sssd/ticket/595)
Not only that but it would be an inefficient111 enumeration as it would be repeated multiple times for each group.
Why is that? Users should of course be saved in the sysdb after being read from LDAP. If you read another group which has that user as a member the result is taken from the cache of course. (That's what my patch tries to do at least, though I admit there is still room for optimization)
In order to save a user you have to look it up. so if you have a group with 50k users in it you will have to do 50k lookups. Multiply this for N groups.
Users that were already looked up when looking up the first group will not be looked up again (they are already cached) when resolving the second group. If the groups really have 50k distinct members you are of course right that a lot of LDAP lookups will need to be done. But I wonder how common such a scenario is and if sssd's default configuration should be optimized for such a usecases. I'd guess most enviroments that sssd operates in are much smaller. And there you'll confuse the users/admins when groups show up incomplete.
If you need enumeration completeness then you should just have enumerations turned on IMO.
Now thinking a bit more there is probably a way to avoid the lookups. You can avoid the 50k lookups by actually *not* fetching each member from LDAP but create fake entries that are marked as expired so they are immediately refreshed with the right data if they are ever requested from the user space.
Right, this how I was thinking about implementing it for rfc2307 groups, but I haven't come to that yet.
But this technique has some pretty sever limits when using rfc2307bis and nested groups. (should work just fine for classic rfc2307)
One limit is that you don't know whether a DN is a user or a Group. It is true that we can adopt some heuristics like examining the DN components to get a clue, but they would be pretty much schema dependent and would fail consistently in some scenarios. So you may fail to properly unroll nested groups.
The other is that the actual user name may not be contained within the DN. if the DN includes the uid attribute as part of it does, but if the CN is used (like against AD where the samAccoutnName attribute is used as the OS user name) then you do not have the user name you need to present in the group as member. So, again, it would work only in some setups.
I think it is not worth to go that path for rfc2307bis groups. It wrong to rely too much on such specific setups.
Another concern is about how to properly do caching. When we did the initgroups call I used to fully resolve groups, we removed that code because it would make initgroups unbearably slow as it would end up doing way too many lookups.
Agreed. For initgroups the groups should not be completely resolved. OTOH when dealing with nested groups (in a scenario without "memberof") you'd have to look up all group members anyways. (IMO a reason why nested groups should be disabled by default when using a standard rfc2307bis setup.)
So we need to make sure initgroups can create/manipulate groups and know when a group can be considered up to date, and yet at the same time mark it as "incomplete" or something like that for getent group <group> calls (and vice versa ?)
Yes. Probably something like this: After initgroups has finished successfully for a specific user. The sysdb object of that user should be "marked" that the initgroups result for that user in completely cached. All groups that needed to be loaded from LDAP during that initgroups call need to be marked as incomplete. When getgr(gid|nam) is called for such a group the group will be resolved and the "imcomplete" mark can be removed.
I guess we could have a third option to manage this, and allow people full group (all member users/groups(recursive)) lookups if they really want/need it. But it has a lot of subtle interactions we need to care about to avoid perf. side effects.
May be something like: fully_resolve_groups defaulting to false ?
Simo.
regards, Ralf
On Fri, 10 Sep 2010 11:31:48 +0200 Ralf Haferkamp rhafer@suse.de wrote:
On Thursday 09 September 2010 17:48:09 Simo Sorce wrote:
On Thu, 9 Sep 2010 16:46:35 +0200
It is true you may not see other members unless they log in, but why would that be a problem ? If they do not log in they are not affected by group memberships anyway.
The user currently logged in might need to see who else is member of the groups he is member of.
Ok, I know of no application that needs it, but if users request it its enough.
BTW, I just found out that the behaviour of getgrouplist()/initgroups() is similar currently. It will only return the groups that are already present in the cache. That means in many cases it will return nothing. (Or just the gid you supplied via the group argument).
Initgroups will always call the LDAP server, and will make sure all groups the user is member of are available.
Ok, there something else seems broken here:
- When initgroups is called for a user that is currently not cached
no LDAP groups are returned. (I guess that's inline with your idea about how sssd should behave)
No, initgroups should always fetch the user's groups, please open a bug.
- When the user is already cached and rfc2307bis+memberof is used on
the Serverside, nothing is returned upon the first initgroups call, but the cache is correctly populated with the groups, a second initgroups call then returns the correct results. So I guess there's a bug here somewhere. Haven't investigated this deeper yet.
Yes, and is probably connected with the above issue.
- When using rfc2307 or rfc2307bis (without memberof Attributes) initgroups() seems completely broken.
Sigh, please open bug ....
I'll create bug reports after I was able to take a deeper look. (It seems some of the issues are already reported. See e.g. https://fedorahosted.org/sssd/ticket/595)
Thank you.
In order to save a user you have to look it up. so if you have a group with 50k users in it you will have to do 50k lookups. Multiply this for N groups.
Users that were already looked up when looking up the first group will not be looked up again (they are already cached) when resolving the second group. If the groups really have 50k distinct members you are of course right that a lot of LDAP lookups will need to be done. But I wonder how common such a scenario is and if sssd's default configuration should be optimized for such a usecases. I'd guess most enviroments that sssd operates in are much smaller. And there you'll confuse the users/admins when groups show up incomplete.
SSSD was initially born as IPA's client. In IPA (at least in v1) every user you create is automatically made member of the ipausers groups. So that group always is as big as the whole userbase ...
If you need enumeration completeness then you should just have enumerations turned on IMO.
Now thinking a bit more there is probably a way to avoid the lookups. You can avoid the 50k lookups by actually *not* fetching each member from LDAP but create fake entries that are marked as expired so they are immediately refreshed with the right data if they are ever requested from the user space.
Right, this how I was thinking about implementing it for rfc2307 groups, but I haven't come to that yet.
But this technique has some pretty sever limits when using rfc2307bis and nested groups. (should work just fine for classic rfc2307)
One limit is that you don't know whether a DN is a user or a Group. It is true that we can adopt some heuristics like examining the DN components to get a clue, but they would be pretty much schema dependent and would fail consistently in some scenarios. So you may fail to properly unroll nested groups.
The other is that the actual user name may not be contained within the DN. if the DN includes the uid attribute as part of it does, but if the CN is used (like against AD where the samAccoutnName attribute is used as the OS user name) then you do not have the user name you need to present in the group as member. So, again, it would work only in some setups.
I think it is not worth to go that path for rfc2307bis groups. It wrong to rely too much on such specific setups.
Yes, I am not really suggesting we do that, but I thought it was important to explain limitations in this area.
Another concern is about how to properly do caching. When we did the initgroups call I used to fully resolve groups, we removed that code because it would make initgroups unbearably slow as it would end up doing way too many lookups.
Agreed. For initgroups the groups should not be completely resolved. OTOH when dealing with nested groups (in a scenario without "memberof") you'd have to look up all group members anyways. (IMO a reason why nested groups should be disabled by default when using a standard rfc2307bis setup.)
Yes, a good reason. We should probably have a config option to enable/disable nested groups rolling at this point.
So we need to make sure initgroups can create/manipulate groups and know when a group can be considered up to date, and yet at the same time mark it as "incomplete" or something like that for getent group <group> calls (and vice versa ?)
Yes. Probably something like this: After initgroups has finished successfully for a specific user. The sysdb object of that user should be "marked" that the initgroups result for that user in completely cached. All groups that needed to be loaded from LDAP during that initgroups call need to be marked as incomplete. When getgr(gid|nam) is called for such a group the group will be resolved and the "imcomplete" mark can be removed.
Right, requires a few modifications but shouldn't be too difficult.
Steve, should we put this into a bug/rfe ?
Simo.
Simo Sorce wrote:
On Fri, 10 Sep 2010 11:31:48 +0200 Ralf Haferkamp rhafer@suse.de wrote:
On Thursday 09 September 2010 17:48:09 Simo Sorce wrote:
On Thu, 9 Sep 2010 16:46:35 +0200
It is true you may not see other members unless they log in, but why would that be a problem ? If they do not log in they are not affected by group memberships anyway.
The user currently logged in might need to see who else is member of the groups he is member of.
Ok, I know of no application that needs it, but if users request it its enough.
BTW, I just found out that the behaviour of getgrouplist()/initgroups() is similar currently. It will only return the groups that are already present in the cache. That means in many cases it will return nothing. (Or just the gid you supplied via the group argument).
Initgroups will always call the LDAP server, and will make sure all groups the user is member of are available.
Ok, there something else seems broken here:
- When initgroups is called for a user that is currently not cached
no LDAP groups are returned. (I guess that's inline with your idea about how sssd should behave)
No, initgroups should always fetch the user's groups, please open a bug.
- When the user is already cached and rfc2307bis+memberof is used on
the Serverside, nothing is returned upon the first initgroups call, but the cache is correctly populated with the groups, a second initgroups call then returns the correct results. So I guess there's a bug here somewhere. Haven't investigated this deeper yet.
Yes, and is probably connected with the above issue.
- When using rfc2307 or rfc2307bis (without memberof Attributes) initgroups() seems completely broken.
Sigh, please open bug ....
I'll create bug reports after I was able to take a deeper look. (It seems some of the issues are already reported. See e.g. https://fedorahosted.org/sssd/ticket/595)
Thank you.
In order to save a user you have to look it up. so if you have a group with 50k users in it you will have to do 50k lookups. Multiply this for N groups.
Users that were already looked up when looking up the first group will not be looked up again (they are already cached) when resolving the second group. If the groups really have 50k distinct members you are of course right that a lot of LDAP lookups will need to be done. But I wonder how common such a scenario is and if sssd's default configuration should be optimized for such a usecases. I'd guess most enviroments that sssd operates in are much smaller. And there you'll confuse the users/admins when groups show up incomplete.
SSSD was initially born as IPA's client. In IPA (at least in v1) every user you create is automatically made member of the ipausers groups. So that group always is as big as the whole userbase ...
If you need enumeration completeness then you should just have enumerations turned on IMO.
Now thinking a bit more there is probably a way to avoid the lookups. You can avoid the 50k lookups by actually *not* fetching each member from LDAP but create fake entries that are marked as expired so they are immediately refreshed with the right data if they are ever requested from the user space.
Right, this how I was thinking about implementing it for rfc2307 groups, but I haven't come to that yet.
But this technique has some pretty sever limits when using rfc2307bis and nested groups. (should work just fine for classic rfc2307)
One limit is that you don't know whether a DN is a user or a Group. It is true that we can adopt some heuristics like examining the DN components to get a clue, but they would be pretty much schema dependent and would fail consistently in some scenarios. So you may fail to properly unroll nested groups.
The other is that the actual user name may not be contained within the DN. if the DN includes the uid attribute as part of it does, but if the CN is used (like against AD where the samAccoutnName attribute is used as the OS user name) then you do not have the user name you need to present in the group as member. So, again, it would work only in some setups.
I think it is not worth to go that path for rfc2307bis groups. It wrong to rely too much on such specific setups.
Yes, I am not really suggesting we do that, but I thought it was important to explain limitations in this area.
Another concern is about how to properly do caching. When we did the initgroups call I used to fully resolve groups, we removed that code because it would make initgroups unbearably slow as it would end up doing way too many lookups.
Agreed. For initgroups the groups should not be completely resolved. OTOH when dealing with nested groups (in a scenario without "memberof") you'd have to look up all group members anyways. (IMO a reason why nested groups should be disabled by default when using a standard rfc2307bis setup.)
Yes, a good reason. We should probably have a config option to enable/disable nested groups rolling at this point.
So we need to make sure initgroups can create/manipulate groups and know when a group can be considered up to date, and yet at the same time mark it as "incomplete" or something like that for getent group <group> calls (and vice versa ?)
Yes. Probably something like this: After initgroups has finished successfully for a specific user. The sysdb object of that user should be "marked" that the initgroups result for that user in completely cached. All groups that needed to be loaded from LDAP during that initgroups call need to be marked as incomplete. When getgr(gid|nam) is called for such a group the group will be resolved and the "imcomplete" mark can be removed.
Right, requires a few modifications but shouldn't be too difficult.
Steve, should we put this into a bug/rfe ?
We should. Steve is not in today but I can do it.
Is this the right summary: 1a) Initgroups do not fetch groups from LDAP - bug 1b) When the user is already cached and rfc2307bis+memberof is used on the Serverside, nothing is returned upon the first initgroups call (bug above), but the cache is correctly populated with the groups, a second initgroups call then returns the correct results (task to test related to the bug above). 2) When using rfc2307 or rfc2307bis (without memberof Attributes) initgroups() seems completely broken. (this is probably related to ticket 595) 3) Have a config option to enable/disable nested groups rolling with default enabled. We should document the scenarios when it makes sense to enable or disable it. 4) Add marking to the objects. "Complete" mark is put on the user object when all groups he is a member of are fetched. The groups that were fetched and were not in the cache are marked as "incomplete". When the group members are enumerated for a group all users for a group should be fetched and "group" should be marked as "complete", users fetched by this lookup that were not in the cache are marked as "incomplete". In the implementation there is probably no difference between the case when entry does not exist or or has "incomplete" status. In both cases same action needs to be taken. In both cases we need to fetch an entry. So I guess we really need just the "complete" mark.
Is this the correct summary of all the issues?
Thanks Dmitri
Simo.
On Fri, 10 Sep 2010 09:06:29 -0400 Dmitri Pal dpal@redhat.com wrote:
Is this the right summary: 1a) Initgroups do not fetch groups from LDAP - bug
I am not sure this is correct. It normally works (we have tests) but it may not work properly in some conditions.
Ralf can you open a bug with logs and all ?
1b) When the user is already cached and rfc2307bis+memberof is used on the Serverside, nothing is returned upon the first initgroups call (bug above), but the cache is correctly populated with the groups, a second initgroups call then returns the correct results (task to test related to the bug above).
I never seen this myself, but I haven't tested the code lately either, I'll let Ralf comment as he is the one seeing it.
- When using rfc2307 or rfc2307bis (without memberof Attributes)
initgroups() seems completely broken. (this is probably related to ticket 595)
Yeah maybe just add note to 595 if necessary.
- Have a config option to enable/disable nested groups rolling with
default enabled. We should document the scenarios when it makes sense to enable or disable it.
We actually proposed 2 different options in the mail thread.
Option 1. Fetch/Do not fetch, all group members (default disabled)
Option 2. Resolve Nested Groups, here I think the default should depend on what the server offers. If it is IPA with its memberof we can always properly resolve nested groups with almost no effort, so we should have nested groups enable by default as we encourage using them in the server. For other servers I am not sure what is the best default. In general having it on by default is not a bad idea, unless the server has been managed so badly that nesting causes issues.
Another way would be to make this a property of the schema you choose, and differentiate between rfc2307bis and rfc2307bis+nested groups by having a different name.
- Add marking to the objects. "Complete" mark is put on the user
object when all groups he is a member of are fetched. The groups that were fetched and were not in the cache are marked as "incomplete".
This is only on initgroups calls, and I think the first part (properly marking the user) is already implemented, we only need to mark the groups differently.
When the group members are enumerated for a group all users for a group should be fetched and "group" should be marked as "complete", users fetched by this lookup that were not in the cache are marked as "incomplete".
I think we could manage by simply marking users as expired, although this may cause issues if we go offline, as we do not have uid/gid and other fields ... probably we can simply leave those fields off, and this will automatically make them "incomplete". We would have to make sure the rest of the code can cope (and filter out) these users when we are offline.
In the implementation there is probably no difference between the case when entry does not exist or or has "incomplete" status.
Indeed, "incomplete" entries need to be revalidated (if online) or just filtered (when offline).
In both cases same action needs to be taken. In both cases we need to fetch an entry. So I guess we really need just the "complete" mark.
The difference is between the online and the offline case, but in the end yes, the complete mark is what will validate the entry for use.
Simo.
On Friday 10 September 2010 15:34:22 Simo Sorce wrote:
On Fri, 10 Sep 2010 09:06:29 -0400
Dmitri Pal dpal@redhat.com wrote:
Is this the right summary: 1a) Initgroups do not fetch groups from LDAP - bug
I am not sure this is correct. It normally works (we have tests) but it may not work properly in some conditions.
Does you testcases probably call getpwnam() before calling initgroups/getgrouplist()?
Ralf can you open a bug with logs and all ?
This is now #620.
1b) When the user is already cached and rfc2307bis+memberof is used on the Serverside, nothing is returned upon the first initgroups call (bug above), but the cache is correctly populated with the groups, a second initgroups call then returns the correct results (task to test related to the bug above).
I never seen this myself, but I haven't tested the code lately either, I'll let Ralf comment as he is the one seeing it.
I created #621 for this.
- When using rfc2307 or rfc2307bis (without memberof Attributes)
initgroups() seems completely broken. (this is probably related to ticket 595)
Yeah maybe just add note to 595 if necessary.
The rfc2307bis issue is #595, yes. sssd just assumes that "memberof" is in place when rfc2307bis is configured.
It seems that the rfc2307 problem is something different though. I created #622 for that.
- Have a config option to enable/disable nested groups rolling with
default enabled. We should document the scenarios when it makes sense to enable or disable it.
We actually proposed 2 different options in the mail thread.
Option 1. Fetch/Do not fetch, all group members (default disabled)
Option 2. Resolve Nested Groups, here I think the default should depend on what the server offers. If it is IPA with its memberof we can always properly resolve nested groups with almost no effort, so we should have nested groups enable by default as we encourage using them in the server. For other servers I am not sure what is the best default. In general having it on by default is not a bad idea, unless the server has been managed so badly that nesting causes issues.
The main reason for turning it off by default for rfc2307bis is IMO that it causes lots of unneeded LDAP lookups even when no nested groups are used. In initgroups() you'll have to read every groupmember to check if it probably is another group.
Another way would be to make this a property of the schema you choose, and differentiate between rfc2307bis and rfc2307bis+nested groups by having a different name.
- Add marking to the objects. "Complete" mark is put on the user
object when all groups he is a member of are fetched. The groups that were fetched and were not in the cache are marked as "incomplete".
This is only on initgroups calls, and I think the first part (properly marking the user) is already implemented, we only need to mark the groups differently.
When the group members are enumerated for a group all users for a group should be fetched and "group" should be marked as "complete", users fetched by this lookup that were not in the cache are marked as "incomplete".
Why? Or do you mean incomplete wrt to the initgroups() call for that user.
I think we could manage by simply marking users as expired, although this may cause issues if we go offline, as we do not have uid/gid and other fields ... probably we can simply leave those fields off, and this will automatically make them "incomplete". We would have to make sure the rest of the code can cope (and filter out) these users when we are offline.
Are you talking about the rfc2307 case, were "dummy" user object could be created? In the other cases (IPA, rfc2307bis) the complete user objects (including gid, uid,...) should be read from LDAP IMO. Reading only a subset there has no real advantage IMO.
In the implementation there is probably no difference between the case when entry does not exist or or has "incomplete" status.
Indeed, "incomplete" entries need to be revalidated (if online) or just filtered (when offline).
In both cases same action needs to be taken. In both cases we need to fetch an entry. So I guess we really need just the "complete" mark.
The difference is between the online and the offline case, but in the end yes, the complete mark is what will validate the entry for use.
Ralf
[snip]
- Add marking to the objects. "Complete" mark is put on the user
object when all groups he is a member of are fetched. The groups that were fetched and were not in the cache are marked as "incomplete".
This is only on initgroups calls, and I think the first part (properly marking the user) is already implemented, we only need to mark the groups differently.
When the group members are enumerated for a group all users for a group should be fetched and "group" should be marked as "complete", users fetched by this lookup that were not in the cache are marked as "incomplete".
Why? Or do you mean incomplete wrt to the initgroups() call for that user.
I think we could manage by simply marking users as expired, although this may cause issues if we go offline, as we do not have uid/gid and other fields ... probably we can simply leave those fields off, and this will automatically make them "incomplete". We would have to make sure the rest of the code can cope (and filter out) these users when we are offline.
Are you talking about the rfc2307 case, were "dummy" user object could be created? In the other cases (IPA, rfc2307bis) the complete user objects (including gid, uid,...) should be read from LDAP IMO. Reading only a subset there has no real advantage IMO.
Incomplete user is the user that is fully fetched as user entry but the list of the groups he is a member of is not fetched. This makes sense for the cases when there is no memberOf and we do not want for each user in a group go fetch all the groups this user is a member of. We already had a performance issue with this approach and we had to fix it recently. If memberOf is present we can already mark it as complete. I am not talking about reading parts of user object. Group is complete is we know all its members. User is complete if we know all groups he is a member of. This is how I define completeness in this context. Sorry for confusion.
Hi,
On Thursday 09 September 2010 15:14:10 Ralf Haferkamp wrote: [..]
I have started working on a patch to let sssd look up the non-cached users via LDAP (and save them into the cache). Find it attached. Note: That patch is not really complete (e.g. it doesn't handle rfc2307 groups correctly). But before putting more effort into this I like to make sure that I am not trying to fix a "feature" here.
Find a newer version of my patch attached. Actually it's 3 patches now. Please review.
Patch1: This just adds a new flag to save_groups() to indicate that the group's member attribute is already populated with the members' sysdb DN (instead on LDAP DNs). As I need to lookup the group members in sysdb anyway, when processing the group, this saves some redundant sysdb lookups when storing the group.
Patch2: This is a somewhat improved version of my last patch. - better error handling - limit the number of LDAP requests that are issued before starting to process the results. This is especially needed when dealing with large groups, otherwise the server might choke on us (e.g. OpenLDAP has a (configurable) limit of 100 pending operations per anonymous connection and 1000 per authenticated connection). OTOH sending multiple LDAP request at once will speed up things a bit compared to just sending the next request after processing the result of the previous. - populate the "member" attribute with the correct sysdb DNs to utilize Patch1. - limit the group unrolling to rfc2307bis for now. rfc2307 and IPA need to be treated differently as discussed previously in this thread.
Patch3: This adds a new config option to "ldap_unroll_group_members" to enable/disable group unrolling
regards, Ralf
On Thu, 16 Sep 2010 17:50:28 +0200 Ralf Haferkamp rhafer@suse.de wrote:
Hi,
On Thursday 09 September 2010 15:14:10 Ralf Haferkamp wrote: [..]
I have started working on a patch to let sssd look up the non-cached users via LDAP (and save them into the cache). Find it attached. Note: That patch is not really complete (e.g. it doesn't handle rfc2307 groups correctly). But before putting more effort into this I like to make sure that I am not trying to fix a "feature" here.
Find a newer version of my patch attached. Actually it's 3 patches now. Please review.
Patch1: This just adds a new flag to save_groups() to indicate that the group's member attribute is already populated with the members' sysdb DN (instead on LDAP DNs). As I need to lookup the group members in sysdb anyway, when processing the group, this saves some redundant sysdb lookups when storing the group.
This looks like a good idea.
Patch2: This is a somewhat improved version of my last patch.
- better error handling
- limit the number of LDAP requests that are issued before starting to process the results. This is especially needed when dealing with large groups, otherwise the server might choke on us (e.g. OpenLDAP has a (configurable) limit of 100 pending
operations per anonymous connection and 1000 per authenticated connection). OTOH sending multiple LDAP request at once will speed up things a bit compared to just sending the next request after processing the result of the previous.
- populate the "member" attribute with the correct sysdb DNs to utilize Patch1.
- limit the group unrolling to rfc2307bis for now. rfc2307 and IPA need to be treated differently as discussed previously in this thread.
This patch makes the main function very complex, I suggest that you at least create separate functions for each new tevent request you want to create, that is sort of a rule for sssd. (And it makes code digestible more often than not).
As for group unrolling I have also started working on it (ticket #625), although I am doing that in the 1.2.x branch as we need the functionality there too. I will try to post a patch soon so that we can compare relative approaches and merge the effort, ok ?
Patch3: This adds a new config option to "ldap_unroll_group_members" to enable/disable group unrolling
Can we use the followin patch instead ? http://fedorapeople.org/gitweb?p=simo/public_git/sssd.git;a=commitdiff;h=fed...
This patch assumes the code will consider a nesting level of 0 as "no nesting". therefore it will embed in a single option both a way to enable disable unrolling and a limit on the level of nesting we will allow on the client (to avoid loops or very long delay on pathological cases).
Simo.
Hi,
On Thursday 16 September 2010 20:16:56 Simo Sorce wrote:
On Thu, 16 Sep 2010 17:50:28 +0200
Ralf Haferkamp rhafer@suse.de wrote:
Hi,
On Thursday 09 September 2010 15:14:10 Ralf Haferkamp wrote:
[..]
Find a newer version of my patch attached. Actually it's 3 patches now. Please review.
Patch1: This just adds a new flag to save_groups() to indicate that the group's member attribute is already populated with the members' sysdb DN (instead on LDAP DNs). As I need to lookup the group members in sysdb anyway, when processing the group, this saves some redundant
sysdb lookups when storing the group.
This looks like a good idea.
Patch2: This is a somewhat improved version of my last patch.
- better error handling
- limit the number of LDAP requests that are issued before
[..]
This patch makes the main function very complex, I suggest that you at least create separate functions for each new tevent request you want to create, that is sort of a rule for sssd. (And it makes code digestible more often than not).
Find a new version attached. Does that look better? If that is not what you were referring to lets discuss it in IRC on monday.
Note, I needed to implement sdap_process_group_send() slightly differnent than the other _send functions in sdap_async_accounts(). Instead of returning as tevent_req* it does return a error code. The tevent_req* is returned via the argument list. I did this because there are cases where sdap_process_group_send() does not need to create a new tevent_req*, e.g. when the groups doesn't have any members or when all groupmembers are already cached.
As for group unrolling I have also started working on it (ticket #625), although I am doing that in the 1.2.x branch as we need the functionality there too. I will try to post a patch soon so that we can compare relative approaches and merge the effort, ok ?
Fine. Feel free to take what you need from my patch.
Patch3: This adds a new config option to "ldap_unroll_group_members" to enable/disable group unrolling
Can we use the followin patch instead ? http://fedorapeople.org/gitweb?p=simo/public_git/sssd.git;a=commitdiff ;h=fedf324be284de71e5dbf22f0135e9f681a15bde
This patch assumes the code will consider a nesting level of 0 as "no nesting". therefore it will embed in a single option both a way to enable disable unrolling and a limit on the level of nesting we will allow on the client (to avoid loops or very long delay on pathological cases).
Hm, just to make sure I understand your approach: "ldap_group_nesting_level = 0" would be equal to "ldap_unroll_group_members = false" -> resulting in the current getgrnam behavior of only returning cached members.
"ldap_group_nesting_level = 1" would mean "ldap_unroll_group_members = true" -> return only direct members of the group and ignore mested groups.
while "ldap_group_nesting_level > 1" allow nesting up to a certain depth.
If that is correct what does "ldap_group_nesting_level = 0" mean for the initgroups() call? Return only groups that are already cached? I wonder if that would be a good idea.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/17/2010 12:16 PM, Ralf Haferkamp wrote:
Find a new version attached. Does that look better? If that is not what you were referring to lets discuss it in IRC on monday.
Note, I needed to implement sdap_process_group_send() slightly differnent than the other _send functions in sdap_async_accounts(). Instead of returning as tevent_req* it does return a error code. The tevent_req* is returned via the argument list. I did this because there are cases where sdap_process_group_send() does not need to create a new tevent_req*, e.g. when the groups doesn't have any members or when all groupmembers are already cached.
In situations like this, the correct behavior is still to return the request, but inside the _send() function call:
tevent_req_done(req); tevent_req_post(req, ev); return req;
This way it will just call the finish handler immediately after the parent function returns. Then you don't need to have separate codepaths for functions that do or do not have to make subrequests.
(Internally what this does is set up a tevent_immediate callback that fires as soon as processing reenters the mainloop, rather than waiting or looping again)
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Friday 17 September 2010 19:56:15 Stephen Gallagher wrote:
On 09/17/2010 12:16 PM, Ralf Haferkamp wrote:
Find a new version attached. Does that look better? If that is not what you were referring to lets discuss it in IRC on monday.
Note, I needed to implement sdap_process_group_send() slightly differnent than the other _send functions in sdap_async_accounts(). Instead of returning as tevent_req* it does return a error code. The tevent_req* is returned via the argument list. I did this because there are cases where sdap_process_group_send() does not need to create a new tevent_req*, e.g. when the groups doesn't have any members or when all groupmembers are already cached.
In situations like this, the correct behavior is still to return the request, but inside the _send() function call:
tevent_req_done(req); tevent_req_post(req, ev); return req;
This way it will just call the finish handler immediately after the parent function returns. Then you don't need to have separate codepaths for functions that do or do not have to make subrequests.
Nice, that makes the code a little cleaner, thanks. New patches attached.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/20/2010 11:13 AM, Ralf Haferkamp wrote:
On Friday 17 September 2010 19:56:15 Stephen Gallagher wrote:
On 09/17/2010 12:16 PM, Ralf Haferkamp wrote:
Find a new version attached. Does that look better? If that is not what you were referring to lets discuss it in IRC on monday.
Note, I needed to implement sdap_process_group_send() slightly differnent than the other _send functions in sdap_async_accounts(). Instead of returning as tevent_req* it does return a error code. The tevent_req* is returned via the argument list. I did this because there are cases where sdap_process_group_send() does not need to create a new tevent_req*, e.g. when the groups doesn't have any members or when all groupmembers are already cached.
In situations like this, the correct behavior is still to return the request, but inside the _send() function call:
tevent_req_done(req); tevent_req_post(req, ev); return req;
This way it will just call the finish handler immediately after the parent function returns. Then you don't need to have separate codepaths for functions that do or do not have to make subrequests.
Nice, that makes the code a little cleaner, thanks. New patches attached.
Patch 0001: Ack. This looks fine to me.
Patch 0002: Nack. There are still a few style issues. Please move the struct sdap_process_group_state definition to right before the sdap_get_groups_process() implementation.
I'm not a huge fan of firing off all of the sdap_process_group_state() calls in a loop for (effectively) simultaneous operation. I'd prefer if you implemented this with our _step() approach (where you handle each one serially until they're all finished). It's quite a bit easier to follow in a debugger. This will also eliminate the need for GROUPMEMBER_REQ_PARALLEL. Believe it or not, it won't be any slower because the LDAP id op operations will serialize the requests anyway so we don't open large numbers of separate connections to the LDAP server.
There's a memory hierarchy bug in sdap_process_group_send(). You allocate sysdb_dn on memctx, but it really should be a child of grp_state->sysdb_dns->values[grp_state->sysdb_dns->num_values].
I think there's a lot of potential here. We just need to work on cleaning things up a little bit further.
Patch 0003: Nack. I agree with Simo that it would make more sense to use a nesting level rather than a boolean for this task.
Also, any changes to the LDAP options need to be added to src/config/etc/sssd.api.d/sssd-ldap.conf and have a translatable string added to src/config/SSSDConfig.py and also have the same option mirrored in the IPA backend. This latter you will be alerted to if you attempt to run 'make check'.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Thursday 23 September 2010 20:02:20 Stephen Gallagher wrote:
On 09/20/2010 11:13 AM, Ralf Haferkamp wrote:
[..]
Nice, that makes the code a little cleaner, thanks. New patches attached.
Patch 0001: Ack. This looks fine to me.
Patch 0002: Nack. There are still a few style issues. Please move the struct sdap_process_group_state definition to right before the sdap_get_groups_process() implementation.
will do
I'm not a huge fan of firing off all of the sdap_process_group_state() calls in a loop for (effectively) simultaneous operation. I'd prefer if you implemented this with our _step() approach (where you handle each one serially until they're all finished). It's quite a bit easier to follow in a debugger. This will also eliminate the need for GROUPMEMBER_REQ_PARALLEL. Believe it or not, it won't be any slower because the LDAP id op operations will serialize the requests anyway so we don't open large numbers of separate connections to the LDAP server.
I agree that the parallel approach is a bit more complex. But it was significantly faster in the tests I did compared to serially processing the group members. Otherwise I wouldn't have done that implementation. BTW, you can see the difference by setting GROUPMEMBER_REQ_PARALLEL to 0.
The goal wasn't to have multiple LDAP connections opened (that would be insane). But sending multiple request at once to the LDAP server (through a single connection) and then starting processing the results of the requests is of course faster than sending sending a single request then processing the result of that request and then sending the next request. Simply because the LDAP server can a) process multiple requests at once and b) has a some request queued to process while the client is busy with retrieving and processing the results. The only drawback I see here is that the LDAP Server might limit the number of LDAP request it queues on a single connection therefor it would probably be good to replace GROUPMEMBER_REQ_PARALLEL with a value configurable via sssd.conf.
There's a memory hierarchy bug in sdap_process_group_send(). You allocate sysdb_dn on memctx, but it really should be a child of grp_state->sysdb_dns->values[grp_state->sysdb_dns->num_values].
Ok, I'll check that.
I think there's a lot of potential here. We just need to work on cleaning things up a little bit further.
Patch 0003: Nack. I agree with Simo that it would make more sense to use a nesting level rather than a boolean for this task.
Yeah, I fine with taking Simo's approach here.
Also, any changes to the LDAP options need to be added to src/config/etc/sssd.api.d/sssd-ldap.conf and have a translatable string added to src/config/SSSDConfig.py and also have the same option mirrored in the IPA backend. This latter you will be alerted to if you attempt to run 'make check'.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/24/2010 11:27 AM, Ralf Haferkamp wrote:
I agree that the parallel approach is a bit more complex. But it was significantly faster in the tests I did compared to serially processing the group members. Otherwise I wouldn't have done that implementation. BTW, you can see the difference by setting GROUPMEMBER_REQ_PARALLEL to 0.
The goal wasn't to have multiple LDAP connections opened (that would be insane). But sending multiple request at once to the LDAP server (through a single connection) and then starting processing the results of the requests is of course faster than sending sending a single request then processing the result of that request and then sending the next request. Simply because the LDAP server can a) process multiple requests at once and b) has a some request queued to process while the client is busy with retrieving and processing the results. The only drawback I see here is that the LDAP Server might limit the number of LDAP request it queues on a single connection therefor it would probably be good to replace GROUPMEMBER_REQ_PARALLEL with a value configurable via sssd.conf.
Hi Ralf,
I think this part of your work overlaps with what I've been doing in order to fix #621 and #625. My approach was a little bit different and I'm wondering whether we could merge our patches.
I've been implementing Simo's proposal so that for every user that is not cached in sysdb we add a kind of fake "skeleton" user - just a DN, name and marked as expired - so that getgrnam is able to return the list of users but when additional details about the user are requested via getpwnam/getpwuid, the entry is requested. For more details, please see Simo's breakdown here: https://fedorahosted.org/sssd/ticket/625
My proposal is to modify your patch #2 to not lookup the users in LDAP, but rather save these fake expired entries.
I have implemeted this for initgroups (adding fake group entries) and started on getgrname (or rather sdap_save_group). I have pushed patches into my fedorapeople git repository (master branch), if you'd like to take a look.
Jakub
Am Freitag 24 September 2010, 16:29:33 schrieb Jakub Hrozek:
On 09/24/2010 11:27 AM, Ralf Haferkamp wrote:
I agree that the parallel approach is a bit more complex. But it was significantly faster in the tests I did compared to serially processing the group members. Otherwise I wouldn't have done that implementation. BTW, you can see the difference by setting GROUPMEMBER_REQ_PARALLEL to 0.
The goal wasn't to have multiple LDAP connections opened (that would be insane). But sending multiple request at once to the LDAP server (through a single connection) and then starting processing the results of the requests is of course faster than sending sending a single request then processing the result of that request and then sending the next request. Simply because the LDAP server can a) process multiple requests at once and b) has a some request queued to process while the client is busy with retrieving and processing the results. The only drawback I see here is that the LDAP Server might limit the number of LDAP request it queues on a single connection therefor it would probably be good to replace GROUPMEMBER_REQ_PARALLEL with a value configurable via sssd.conf.
Hi Ralf,
I think this part of your work overlaps with what I've been doing in order to fix #621 and #625. My approach was a little bit different and I'm wondering whether we could merge our patches.
I've been implementing Simo's proposal so that for every user that is not cached in sysdb we add a kind of fake "skeleton" user - just a DN, name and marked as expired - so that getgrnam is able to return the list of users but when additional details about the user are requested via getpwnam/getpwuid, the entry is requested.
Yeah, we discussed that already on the list here at the beginning of this thread, but the fake user approach will only work reliably for RFC2307 setups, where the user names of the group members are stored in the "memberUid" Attribute. My patch currently only addresses the RFC2307bis approach, and there you need to read the actualy LDAP objects in order to find out the username (ok there are corner cases where you can derive the user names from the DN but I think we agreed during the discussion not to do such hacks). And it then it is better to actually request the complete users objects and populate the cache with those instead of fake entries.
For more details, please see Simo's breakdown here: https://fedorahosted.org/sssd/ticket/625
My proposal is to modify your patch #2 to not lookup the users in LDAP, but rather save these fake expired entries.
For rfc2307bis those lookups are needed unfortunately. But for rfc2307 setups that's of course the right way to go.
I have implemeted this for initgroups (adding fake group entries) and started on getgrname (or rather sdap_save_group). I have pushed patches into my fedorapeople git repository (master branch), if you'd like to take a look.
Yes, I will.
Hi,
find updated patches attached. (Rebased against current master)
Am Donnerstag 23 September 2010, 20:02:20 schrieb Stephen Gallagher:
On 09/20/2010 11:13 AM, Ralf Haferkamp wrote:
[..]
Patch 0001: Ack. This looks fine to me.
Patch 0002: Nack. There are still a few style issues. Please move the struct sdap_process_group_state definition to right before the sdap_get_groups_process() implementation.
Fixed.
I'm not a huge fan of firing off all of the sdap_process_group_state() calls in a loop for (effectively) simultaneous operation. I'd prefer if you implemented this with our _step() approach (where you handle each one serially until they're all finished). It's quite a bit easier to follow in a debugger. This will also eliminate the need for GROUPMEMBER_REQ_PARALLEL. Believe it or not, it won't be any slower because the LDAP id op operations will serialize the requests anyway so we don't open large numbers of separate connections to the LDAP server.
I didn't change anything here. For the reasons detailed in my other mail.
There's a memory hierarchy bug in sdap_process_group_send(). You allocate sysdb_dn on memctx, but it really should be a child of grp_state->sysdb_dns->values[grp_state->sysdb_dns->num_values].
I guess you ment grp_state->sysdb_dns->values here? Because grp_state->sysdb_dns->values[grp_state->sysdb_dns->num_values] is a struct ldb_message_element (no pointer), so it's not usable as a talloc context. If yes -> Fixed.
I think there's a lot of potential here. We just need to work on cleaning things up a little bit further.
Patch 0003: Nack. I agree with Simo that it would make more sense to use a nesting level rather than a boolean for this task.
I've dropped this patch for now.
Also, any changes to the LDAP options need to be added to src/config/etc/sssd.api.d/sssd-ldap.conf and have a translatable string added to src/config/SSSDConfig.py and also have the same option mirrored in the IPA backend. This latter you will be alerted to if you attempt to run 'make check'.
On Fri, 24 Sep 2010 16:31:24 +0200 Ralf Haferkamp rhafer@suse.de wrote:
Hi,
find updated patches attached. (Rebased against current master)
Am Donnerstag 23 September 2010, 20:02:20 schrieb Stephen Gallagher:
On 09/20/2010 11:13 AM, Ralf Haferkamp wrote:
[..]
Patch 0001: Ack. This looks fine to me.
Uhmm I think I see an issue in patch 1.
It looks to me that we fail the operation if we have no members in a group as when calling sysdb_attrs_get_el() sdap_save_group() now treats ENOENT just like a fatal error.
Although uncommon I think we should handle empty groups.
Also I have a cosmetic request. It took sometimes to me to understand what the name sysdb_member_dns meant. Would it be possible to rename it to something like populate_members/resolve/members/fetch_members or something similar ? It would make it easier to understand it is an option that does something like store_members, just different.
Simo.
(still reviewing patch 2)
Am Montag 27 September 2010, 16:37:14 schrieb Simo Sorce:
On Fri, 24 Sep 2010 16:31:24 +0200
Ralf Haferkamp rhafer@suse.de wrote:
Hi,
find updated patches attached. (Rebased against current master)
Am Donnerstag 23 September 2010, 20:02:20 schrieb Stephen Gallagher:
On 09/20/2010 11:13 AM, Ralf Haferkamp wrote:
[..]
Patch 0001: Ack. This looks fine to me.
Uhmm I think I see an issue in patch 1.
It looks to me that we fail the operation if we have no members in a group as when calling sysdb_attrs_get_el() sdap_save_group() now treats ENOENT just like a fatal error.
Hm, I am not sure what call to sysdb_attrs_get_el() you are referring to, but if I understand the sysdb_attrs_get_el() code correctly it will never return ENOENT. It just calls sysdb_attrs_get_el_int with alloc=true in that case the only error that can return is ENOMEN. Did I overlook something? IIRC I explicitly tested the code with empty groups.
Although uncommon I think we should handle empty groups.
Of course.
Also I have a cosmetic request. It took sometimes to me to understand what the name sysdb_member_dns meant. Would it be possible to rename it to something like populate_members/resolve/members/fetch_members or something similar ? It would make it easier to understand it is an option that does something like store_members, just different.
Ok. Find updated patch attached.
On Mon, 27 Sep 2010 21:37:18 +0200 Ralf Haferkamp rhafer@suse.de wrote:
Am Montag 27 September 2010, 16:37:14 schrieb Simo Sorce:
On Fri, 24 Sep 2010 16:31:24 +0200
Ralf Haferkamp rhafer@suse.de wrote:
Hi,
find updated patches attached. (Rebased against current master)
Am Donnerstag 23 September 2010, 20:02:20 schrieb Stephen Gallagher:
On 09/20/2010 11:13 AM, Ralf Haferkamp wrote:
[..]
Patch 0001: Ack. This looks fine to me.
Uhmm I think I see an issue in patch 1.
It looks to me that we fail the operation if we have no members in a group as when calling sysdb_attrs_get_el() sdap_save_group() now treats ENOENT just like a fatal error.
Hm, I am not sure what call to sysdb_attrs_get_el() you are referring to, but if I understand the sysdb_attrs_get_el() code correctly it will never return ENOENT. It just calls sysdb_attrs_get_el_int with alloc=true in that case the only error that can return is ENOMEN. Did I overlook something? IIRC I explicitly tested the code with empty groups.
Uhm I guess I was seeing something that's not there then, sorry :/
Although uncommon I think we should handle empty groups.
Of course.
Also I have a cosmetic request. It took sometimes to me to understand what the name sysdb_member_dns meant. Would it be possible to rename it to something like populate_members/resolve/members/fetch_members or something similar ? It would make it easier to understand it is an option that does something like store_members, just different.
Ok. Find updated patch attached.
ack.
Hi,
find yet another release of the patches attached. It adresses the remaining issues we discussed in IRC:
- included the Simo's style fixes and rearrangements to better match the overall sssd code style - Errors detected in sdap_process_group_members() are not considered fatal (apart from ENOMEM conditions, where it doesn't seem to make a lot of sense to continue). We just log an error message an skip to the next member. This could be improved a bit by stopping to send new LDAP request when sdap_get_generic_recv() error code e.g. indicates that the LDAP connection is permanently broken. What error code would that be, EIO? But this should probably better addressed together with #633.
On Friday 01 October 2010 15:22:09 Ralf Haferkamp wrote:
Hi,
find yet another release of the patches attached. It adresses the remaining issues we discussed in IRC:
- included the Simo's style fixes and rearrangements to better match
the overall sssd code style
- Errors detected in sdap_process_group_members() are not considered fatal (apart from ENOMEM conditions, where it doesn't seem to make a lot of sense to continue). We just log an error message an skip to the next member. This could
be improved a bit by stopping to send new LDAP request when sdap_get_generic_recv() error code e.g. indicates that the LDAP connection is permanently broken. What error code would that be, EIO? But this should probably better addressed together with #633.
Any news on this? Is the patch now acceptable?
regards, Ralf
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/11/2010 10:15 AM, Ralf Haferkamp wrote:
Any news on this? Is the patch now acceptable?
regards, Ralf
Hi Ralf,
I'm sorry for the delay but we have been swamped releasing the 1.2.4 version. We will get back to fixing the issues in the master branch this week.
As per your patches, I based quite some of my 1.2 patch and all of my RFC2307 work in master on them, so I know they work fine.
They were two additional minor comments during the review of the 1.2 patch - https://fedorahosted.org/pipermail/sssd-devel/2010-October/004773.html
The first (changing the bool parameter) one was later withdrawn as it is quite invasive change esp. for the 1.2 branch, but the latter (memory contexts) might be something we should fix in master, too.
Jakub
On Monday 11 October 2010 10:30:01 Jakub Hrozek wrote: [..]
Hi Ralf,
I'm sorry for the delay but we have been swamped releasing the 1.2.4 version. We will get back to fixing the issues in the master branch this week.
As per your patches, I based quite some of my 1.2 patch and all of my RFC2307 work in master on them, so I know they work fine.
They were two additional minor comments during the review of the 1.2 patch - https://fedorahosted.org/pipermail/sssd-devel/2010-October/004773.html
The first (changing the bool parameter) one was later withdrawn as it is quite invasive change esp. for the 1.2 branch, but the latter (memory contexts) might be something we should fix in master, too.
Ok. The attached patches have the memctx issues addressed (similar to what you did in the 1.2 branch).
regards, Ralf
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/11/2010 05:17 PM, Ralf Haferkamp wrote:
On Monday 11 October 2010 10:30:01 Jakub Hrozek wrote: [..]
Hi Ralf,
I'm sorry for the delay but we have been swamped releasing the 1.2.4 version. We will get back to fixing the issues in the master branch this week.
As per your patches, I based quite some of my 1.2 patch and all of my RFC2307 work in master on them, so I know they work fine.
They were two additional minor comments during the review of the 1.2 patch - https://fedorahosted.org/pipermail/sssd-devel/2010-October/004773.html
The first (changing the bool parameter) one was later withdrawn as it is quite invasive change esp. for the 1.2 branch, but the latter (memory contexts) might be something we should fix in master, too.
Ok. The attached patches have the memctx issues addressed (similar to what you did in the 1.2 branch).
regards, Ralf
Thank you.
I have no more comments, so ACK from me.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/12/2010 06:37 AM, Jakub Hrozek wrote:
On 10/11/2010 05:17 PM, Ralf Haferkamp wrote:
On Monday 11 October 2010 10:30:01 Jakub Hrozek wrote: [..]
Hi Ralf,
I'm sorry for the delay but we have been swamped releasing the 1.2.4 version. We will get back to fixing the issues in the master branch this week.
As per your patches, I based quite some of my 1.2 patch and all of my RFC2307 work in master on them, so I know they work fine.
They were two additional minor comments during the review of the 1.2 patch - https://fedorahosted.org/pipermail/sssd-devel/2010-October/004773.html
The first (changing the bool parameter) one was later withdrawn as it is quite invasive change esp. for the 1.2 branch, but the latter (memory contexts) might be something we should fix in master, too.
Ok. The attached patches have the memctx issues addressed (similar to what you did in the 1.2 branch).
regards, Ralf
Thank you.
I have no more comments, so ACK from me.
Pushed to master. Great work, guys!
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
On Thu, 23 Sep 2010 14:02:20 -0400 Stephen Gallagher sgallagh@redhat.com wrote:
I'm not a huge fan of firing off all of the sdap_process_group_state() calls in a loop for (effectively) simultaneous operation. I'd prefer if you implemented this with our _step() approach (where you handle each one serially until they're all finished). It's quite a bit easier to follow in a debugger. This will also eliminate the need for GROUPMEMBER_REQ_PARALLEL. Believe it or not, it won't be any slower because the LDAP id op operations will serialize the requests anyway so we don't open large numbers of separate connections to the LDAP server.
Steve, I agree with Ralf's approach here. Yes it makes the code a bit more complex, but it will kill latency big time. And latency is something that hurts us really bad.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/25/2010 09:12 AM, Simo Sorce wrote:
On Thu, 23 Sep 2010 14:02:20 -0400 Stephen Gallagher sgallagh@redhat.com wrote:
I'm not a huge fan of firing off all of the sdap_process_group_state() calls in a loop for (effectively) simultaneous operation. I'd prefer if you implemented this with our _step() approach (where you handle each one serially until they're all finished). It's quite a bit easier to follow in a debugger. This will also eliminate the need for GROUPMEMBER_REQ_PARALLEL. Believe it or not, it won't be any slower because the LDAP id op operations will serialize the requests anyway so we don't open large numbers of separate connections to the LDAP server.
Steve, I agree with Ralf's approach here. Yes it makes the code a bit more complex, but it will kill latency big time. And latency is something that hurts us really bad.
After a weekend's thought, I agree as well. The value is worth the cost.
- -- Stephen Gallagher RHCE 804006346421761
Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/
sssd-devel@lists.fedorahosted.org