On Mon, Jun 24, 2013 at 08:53:24AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Mon 24 Jun 2013 08:48:57 AM EDT, Jakub Hrozek wrote:
> > On Mon, Jun 24, 2013 at 08:04:34AM -0400, Stephen Gallagher wrote:
> >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> >>
> >> On 06/23/2013 03:12 PM, Jakub Hrozek wrote:
> >>> The attached patch applies on both master and sssd-1-9 and
> >>> fixes:
https://fedorahosted.org/sssd/ticket/1806
> >>>
> >>> The IPA provider attempted to store the original value of
> >>> member attribute to the cache. That caused the memberof plugin
> >>> to process the values which was really CPU intensive.
> >>>
> >>> We don't use the values anywhere and rely on the originalDN
> >>> instead, so it's safe to avoid even downloading them.
> >>>
> >>> Many thanks to Andreas and Simo for their help debugging the
> >>> problem.
> >>>
> >>
> >> Nack
> >>
> >> I was thinking that we might consider replacing SYSDB_MEMBER
> >> with SYSDB_ORIG_MEMBER (so it gets translated to originalMember
> >> instead), but if we're really not consuming it anywhere at all, I
> >> agree that it makes sense to save bandwidth and storage. So the
> >> patch is fine as it is, except for one thing.
> >>
> >
> > I couldn't find any place where we would read either SYSDB_MEMBER
> > or the map entry.
> >
> >>
> >> As a one-time event, we should also update the DB version and
> >> purge these entries that already exist. The reason for that is
> >> that in the future if we had to re-process the member/memberOf
> >> relations in another update, we don't want to force these to be
> >> re-evaluated.
> >
> > Sorry, I don't understand. Why should we purge the old database?
> > The member attributes were never saved because they contained the
> > original values, not the sysdb DN. So ldb just skipped them after
> > attempting to save them.
>
> Oh, I didn't realize it would refuse to save them. I was assuming they
> were there, but dangling. If they're not saved, then Ack to this patch.
Yeah, that was also the reason I couldn't see the bug initially even
though I had the resulting cache available. I didn't even notice we were
attempting to save the member attributes until we saw the sysdb_attrs
array in gdb..
Just to complete the picture let me add that the time spent was in
trying to parse the DNs.
When memberof tries to handle members it needs to convert them from
plain strings to actual ldb_dn structures so we can get back a
canonicalized DN and can do comparisons properly. To do that it needs to
validate DNs through ldb_dn_expand() which is a very CPU intensive task
and also uses a ton of mallocs as every component is split in
attribute/value pairs and stored in the structure.
All this and at the the end the memberof plugin would try to find the
actual target in our own database, it would find out that no such DN
exist and discard the value and not save it at all (referential
integrity is built into the memberof plugin).
For roughly 2000 members it was taking between 5 and 10 seconds on a
beefy CPU.
Simo.
--
Simo Sorce * Red Hat, Inc * New York