Hi,
this thread obsoletes the previous memberof related threads into a single one to make it easier for the reviewers to apply the patches without having to guess the dependencies. The patches should apply cleanly on origin/master. In addition to bringing all the patches together, this set also addresses some of Simo's concerns, in particular the ghost users are better explained in the big comment blocks above each major memberof operation.
The patches implement the missing operations of the memberof plugin related to ghost users - delete and modify. The mod operation is given some special care because not only we need to propagate the modification to parent groups, but we also need to retain the inherited ghost attributes.
The intent is to fix issues such as #1652. Most patches also include unit tests to make sure we don't break existing memberof plugin functionality.
More information is in the patches themselves or in the commit messages.
On Tue, 2012-12-04 at 21:39 +0100, Jakub Hrozek wrote:
Hi,
this thread obsoletes the previous memberof related threads into a single one to make it easier for the reviewers to apply the patches without having to guess the dependencies. The patches should apply cleanly on origin/master. In addition to bringing all the patches together, this set also addresses some of Simo's concerns, in particular the ghost users are better explained in the big comment blocks above each major memberof operation.
The patches implement the missing operations of the memberof plugin related to ghost users - delete and modify. The mod operation is given some special care because not only we need to propagate the modification to parent groups, but we also need to retain the inherited ghost attributes.
The intent is to fix issues such as #1652. Most patches also include unit tests to make sure we don't break existing memberof plugin functionality.
More information is in the patches themselves or in the commit messages.
The first 5 patches get a tentative ack from me. They look good and they compile fine and a basic smoke test seem to show no issue whatsoever, also make check works as expected.
On the last patch I have a question. I see that you do a number of base searches for each hild of a group in order to source ghost users. But that seem a bit wasteful.
Why don't you simply do a subtree search on the whole tree with (&(objectclass=group)(memberof=<dn of the group you are handling>)) (you might also add a &(ghost=*) ) ?
This would give you all groups that are 'children' of the group you are actually handling with a single search and avoids searching user entries and then immediately discarding them as they have no ghost members.
Simo.
On Wed, Dec 05, 2012 at 12:39:33AM -0500, Simo Sorce wrote:
On Tue, 2012-12-04 at 21:39 +0100, Jakub Hrozek wrote:
Hi,
this thread obsoletes the previous memberof related threads into a single one to make it easier for the reviewers to apply the patches without having to guess the dependencies. The patches should apply cleanly on origin/master. In addition to bringing all the patches together, this set also addresses some of Simo's concerns, in particular the ghost users are better explained in the big comment blocks above each major memberof operation.
The patches implement the missing operations of the memberof plugin related to ghost users - delete and modify. The mod operation is given some special care because not only we need to propagate the modification to parent groups, but we also need to retain the inherited ghost attributes.
The intent is to fix issues such as #1652. Most patches also include unit tests to make sure we don't break existing memberof plugin functionality.
More information is in the patches themselves or in the commit messages.
The first 5 patches get a tentative ack from me. They look good and they compile fine and a basic smoke test seem to show no issue whatsoever, also make check works as expected.
On the last patch I have a question. I see that you do a number of base searches for each hild of a group in order to source ghost users. But that seem a bit wasteful.
Why don't you simply do a subtree search on the whole tree with (&(objectclass=group)(memberof=<dn of the group you are handling>)) (you might also add a &(ghost=*) ) ?
This would give you all groups that are 'children' of the group you are actually handling with a single search and avoids searching user entries and then immediately discarding them as they have no ghost members.
Simo.
Mostly because memberof is transitive so I thought I might get too many "ghost" duplicates in case the nesting went too deep which would need to be filtered later (using the muop operations).
On the other hand, in the typical case, there would probably *many* more user entries to discard when traversing direct members rather than memberofs, so you might be right..
On Wed, 2012-12-05 at 10:18 +0100, Jakub Hrozek wrote:
On Wed, Dec 05, 2012 at 12:39:33AM -0500, Simo Sorce wrote:
On Tue, 2012-12-04 at 21:39 +0100, Jakub Hrozek wrote:
Hi,
this thread obsoletes the previous memberof related threads into a single one to make it easier for the reviewers to apply the patches without having to guess the dependencies. The patches should apply cleanly on origin/master. In addition to bringing all the patches together, this set also addresses some of Simo's concerns, in particular the ghost users are better explained in the big comment blocks above each major memberof operation.
The patches implement the missing operations of the memberof plugin related to ghost users - delete and modify. The mod operation is given some special care because not only we need to propagate the modification to parent groups, but we also need to retain the inherited ghost attributes.
The intent is to fix issues such as #1652. Most patches also include unit tests to make sure we don't break existing memberof plugin functionality.
More information is in the patches themselves or in the commit messages.
The first 5 patches get a tentative ack from me. They look good and they compile fine and a basic smoke test seem to show no issue whatsoever, also make check works as expected.
On the last patch I have a question. I see that you do a number of base searches for each hild of a group in order to source ghost users. But that seem a bit wasteful.
Why don't you simply do a subtree search on the whole tree with (&(objectclass=group)(memberof=<dn of the group you are handling>)) (you might also add a &(ghost=*) ) ?
This would give you all groups that are 'children' of the group you are actually handling with a single search and avoids searching user entries and then immediately discarding them as they have no ghost members.
Simo.
Mostly because memberof is transitive so I thought I might get too many "ghost" duplicates in case the nesting went too deep which would need to be filtered later (using the muop operations).
Sorry not sure I understand what you mean here, the number of users in the typical case is much higher than groups, I wouldn't worry in processing groups that add only duplicates.
On the other hand, in the typical case, there would probably *many* more user entries to discard when traversing direct members rather than memberofs, so you might be right..
IT really depends on how many users are actually fetched, if you have many ghost you'll probably have few users, but then if you have few ghost it may be because a lot of users were actually resolved.
On the filtering of ghosts I was wondering if it wouldn't be faster to sort ghosts by name in the structure where you gather them. It would make searching for a match faster if you then use a bisection algorithm, or use a simple hash table :) This optimization doesn't need to be done right now, but maybe we open a ticket for future improvement.
Simo.
On Wed, Dec 05, 2012 at 08:30:01AM -0500, Simo Sorce wrote:
On Wed, 2012-12-05 at 10:18 +0100, Jakub Hrozek wrote:
On Wed, Dec 05, 2012 at 12:39:33AM -0500, Simo Sorce wrote:
On Tue, 2012-12-04 at 21:39 +0100, Jakub Hrozek wrote:
Hi,
this thread obsoletes the previous memberof related threads into a single one to make it easier for the reviewers to apply the patches without having to guess the dependencies. The patches should apply cleanly on origin/master. In addition to bringing all the patches together, this set also addresses some of Simo's concerns, in particular the ghost users are better explained in the big comment blocks above each major memberof operation.
The patches implement the missing operations of the memberof plugin related to ghost users - delete and modify. The mod operation is given some special care because not only we need to propagate the modification to parent groups, but we also need to retain the inherited ghost attributes.
The intent is to fix issues such as #1652. Most patches also include unit tests to make sure we don't break existing memberof plugin functionality.
More information is in the patches themselves or in the commit messages.
The first 5 patches get a tentative ack from me. They look good and they compile fine and a basic smoke test seem to show no issue whatsoever, also make check works as expected.
On the last patch I have a question. I see that you do a number of base searches for each hild of a group in order to source ghost users. But that seem a bit wasteful.
Why don't you simply do a subtree search on the whole tree with (&(objectclass=group)(memberof=<dn of the group you are handling>)) (you might also add a &(ghost=*) ) ?
This would give you all groups that are 'children' of the group you are actually handling with a single search and avoids searching user entries and then immediately discarding them as they have no ghost members.
Simo.
Mostly because memberof is transitive so I thought I might get too many "ghost" duplicates in case the nesting went too deep which would need to be filtered later (using the muop operations).
Sorry not sure I understand what you mean here, the number of users in the typical case is much higher than groups, I wouldn't worry in processing groups that add only duplicates.
On the other hand, in the typical case, there would probably *many* more user entries to discard when traversing direct members rather than memberofs, so you might be right..
IT really depends on how many users are actually fetched, if you have many ghost you'll probably have few users, but then if you have few ghost it may be because a lot of users were actually resolved.
On the filtering of ghosts I was wondering if it wouldn't be faster to sort ghosts by name in the structure where you gather them. It would make searching for a match faster if you then use a bisection algorithm, or use a simple hash table :) This optimization doesn't need to be done right now, but maybe we open a ticket for future improvement.
As it turned out, it is really needed to filter out duplicated at the time we gather them. The attached new iteration of patches uses hash table to accomplish this.
On Wed, Dec 05, 2012 at 05:41:50PM +0100, Jakub Hrozek wrote:
On Wed, Dec 05, 2012 at 08:30:01AM -0500, Simo Sorce wrote:
On Wed, 2012-12-05 at 10:18 +0100, Jakub Hrozek wrote:
On Wed, Dec 05, 2012 at 12:39:33AM -0500, Simo Sorce wrote:
On Tue, 2012-12-04 at 21:39 +0100, Jakub Hrozek wrote:
Hi,
this thread obsoletes the previous memberof related threads into a single one to make it easier for the reviewers to apply the patches without having to guess the dependencies. The patches should apply cleanly on origin/master. In addition to bringing all the patches together, this set also addresses some of Simo's concerns, in particular the ghost users are better explained in the big comment blocks above each major memberof operation.
The patches implement the missing operations of the memberof plugin related to ghost users - delete and modify. The mod operation is given some special care because not only we need to propagate the modification to parent groups, but we also need to retain the inherited ghost attributes.
The intent is to fix issues such as #1652. Most patches also include unit tests to make sure we don't break existing memberof plugin functionality.
More information is in the patches themselves or in the commit messages.
The first 5 patches get a tentative ack from me. They look good and they compile fine and a basic smoke test seem to show no issue whatsoever, also make check works as expected.
On the last patch I have a question. I see that you do a number of base searches for each hild of a group in order to source ghost users. But that seem a bit wasteful.
Why don't you simply do a subtree search on the whole tree with (&(objectclass=group)(memberof=<dn of the group you are handling>)) (you might also add a &(ghost=*) ) ?
This would give you all groups that are 'children' of the group you are actually handling with a single search and avoids searching user entries and then immediately discarding them as they have no ghost members.
Simo.
Mostly because memberof is transitive so I thought I might get too many "ghost" duplicates in case the nesting went too deep which would need to be filtered later (using the muop operations).
Sorry not sure I understand what you mean here, the number of users in the typical case is much higher than groups, I wouldn't worry in processing groups that add only duplicates.
On the other hand, in the typical case, there would probably *many* more user entries to discard when traversing direct members rather than memberofs, so you might be right..
IT really depends on how many users are actually fetched, if you have many ghost you'll probably have few users, but then if you have few ghost it may be because a lot of users were actually resolved.
On the filtering of ghosts I was wondering if it wouldn't be faster to sort ghosts by name in the structure where you gather them. It would make searching for a match faster if you then use a bisection algorithm, or use a simple hash table :) This optimization doesn't need to be done right now, but maybe we open a ticket for future improvement.
As it turned out, it is really needed to filter out duplicated at the time we gather them. The attached new iteration of patches uses hash table to accomplish this.
One more change. We don't have to store a (key,value) pairs in the hash table as the ghost user name is only stored in the key anyway.
On Wed, Dec 05, 2012 at 03:35:28PM -0500, Simo Sorce wrote:
On Wed, 2012-12-05 at 21:21 +0100, Jakub Hrozek wrote:
One more change. We don't have to store a (key,value) pairs in the hash table as the ghost user name is only stored in the key anyway.
ACK
Pushed to master and sssd-1-9
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Simo Sorce