16:58:44 <knikolla> #startmeeting keystone
16:58:44 <openstack> Meeting started Tue Sep 29 16:58:44 2020 UTC and is due to finish in 60 minutes.  The chair is knikolla. Information about MeetBot at http://wiki.debian.org/MeetBot.
16:58:45 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
16:58:47 <openstack> The meeting name has been set to 'keystone'
17:00:19 <vishakha> o/
17:00:25 <lbragstad> o/
17:00:28 <cmurphy> o/
17:00:30 <knikolla> o/
17:00:32 <knikolla> hi all :)
17:01:23 <gagehugo> o/
17:02:34 <knikolla> lbragstad: we can start with your topic
17:02:40 <knikolla> #topic LDAP paging leaking memory
17:02:42 <lbragstad> knikolla sure
17:02:44 <knikolla> #link https://bugs.launchpad.net/keystone/+bug/1896125
17:02:45 <openstack> Launchpad bug 1896125 in OpenStack Identity (keystone) "LDAP paging leaks memory" [Undecided,In progress] - Assigned to Lance Bragstad (lbragstad)
17:02:45 <lbragstad> ok
17:02:57 <lbragstad> this might get long-winded
17:03:05 <lbragstad> so, apology in advance
17:03:19 <lbragstad> i've been splunking in keystone's LDAP for the last two weeks
17:03:22 <knikolla> lbragstad: no worries, glad we have some technical subjects to discuss every once in a while :)
17:03:52 <lbragstad> before i get into my thoughts, has anyone read or familiarized themselves with https://bugs.launchpad.net/keystone/+bug/1896125 ?
17:03:54 <openstack> Launchpad bug 1896125 in OpenStack Identity (keystone) "LDAP paging leaks memory" [Undecided,In progress] - Assigned to Lance Bragstad (lbragstad)
17:04:22 <cmurphy> just finished reading it
17:04:25 <knikolla> i think so, yes
17:05:24 <lbragstad> in tracking that issue down, i found a bunch of things in the ldap implementation that i can't justify because i don't understand the reasoning behind them
17:05:36 <lbragstad> and i can only make assumptions
17:06:16 <lbragstad> the implementation itself seems quite heavy and indirect, increasing the complexity
17:07:22 <lbragstad> and i think some of that complexity attributed to a non-pythonic design leading to the memory leak
17:08:04 <knikolla> it is a mess
17:08:06 <lbragstad> i have a list of code smells that i'd like to address, but it would require a non-trivial refactor
17:09:08 <lbragstad> i've been thinking about what the redesign would look like for the last 4 days, and it's still not clear, so i think even coming up with a design would be non-trivial (at least for me)
17:09:39 <knikolla> lbragstad: i was looking at more substantial work that we can target for wallaby, and this seems like a good candidate. i'd be happy to help out if you need additional hands.
17:09:54 <lbragstad> ok - cool
17:10:13 <lbragstad> i'd probably need to get sign off for it, since it's a lot of work
17:10:23 <lbragstad> and it's not something i'm going to be able to swing on random nights
17:10:43 <lbragstad> but - as a stop gap, i proposed a fix for the memory leak
17:10:56 <lbragstad> and it just kinda goes with the flow of the current implementation
17:11:19 <knikolla> ++ makes sense
17:11:20 <lbragstad> #link https://review.opendev.org/#/c/754488/
17:11:45 <lbragstad> without talking to the original author, this is my best guess at what they were trying to accomplish
17:12:04 <lbragstad> i was also concerned about changing the interface between handlers
17:12:23 <lbragstad> but - that argument kept pulling me into redesigning things outside the scope of the memory leak issue
17:12:43 <lbragstad> my question is - how do people feel about the current proposal until we can (if we decide to do so) refactor the backend
17:12:54 <lbragstad> full disclosure - i am looking to backport this
17:13:02 <lbragstad> it affects releases all the way back to queens
17:13:44 <knikolla> i haven't done an extensive review, but it seems pretty surgical so i am okay with it
17:14:19 <cmurphy> backporting a bugfix is easier than backporting a refactor
17:14:22 <lbragstad> it does use a new object instead of MsgId - so that i can hide how the context managers are handled
17:14:28 <lbragstad> agreed
17:15:00 <lbragstad> otherwise - it would look something like this https://review.opendev.org/#/c/754488/1/keystone/identity/backends/ldap/common.py
17:15:03 <knikolla> ++, the stopgap can be backported, but i think maybe not the refactor.
17:15:27 <lbragstad> yeah - i don't want to backport a refactor
17:15:41 <knikolla> so the two step approach makes sense
17:16:01 <lbragstad> ok - so folks are at least initially ok with this going into stable releases
17:16:33 <lbragstad> obviously, you're allowed to change your mind after a more thorough review
17:17:03 <cmurphy> lbragstad: can i turn it around - why wouldn't it be okay for stable?
17:17:28 <lbragstad> well - it relies on a new object or an interface change between handlers
17:17:40 <lbragstad> we treat them as internal bits.. but it's hard to say if anyone else is using that stuff
17:18:21 <lbragstad> by internal, only the PooledLDAPHandler knows or cares about MsgId
17:18:34 <lbragstad> the other implementations of LDAPHandlers don't know or care about it
17:18:52 <lbragstad> and that made me think it was relatively safe to change
17:19:21 <cmurphy> i think i'm okay with that, because this is all within the backend code, not changing the identity driver interface
17:19:29 <lbragstad> ++ agreed
17:19:53 <knikolla> ++ i don't see why someone would import common code from the ldap backend, to outside of it.
17:21:32 <lbragstad> anyone else have questions about the approach?
17:22:14 <lbragstad> fwiw - i tried to provide a jdennis-esque commit message to help get what i found out of my head https://review.opendev.org/#/c/754488/3//COMMIT_MSG
17:22:29 <cmurphy> :)
17:23:01 <lbragstad> side note: the ldap backend is an absolutely trip to go through...
17:23:09 <knikolla> thanks a lot for that. it made understanding the reasoning trivial.
17:23:20 <knikolla> haha, it is indeed.
17:25:00 * lbragstad relinquishes the floor
17:25:52 <knikolla> thanks lbragstad
17:25:56 <knikolla> #topic Open Discussion
17:26:53 <cmurphy> i have a couple of easy reviews https://review.opendev.org/753857 https://review.opendev.org/753458
17:31:39 <knikolla> anything else anyone?
17:39:37 <knikolla> irccloud died.
17:39:45 <knikolla> thanks all.
17:39:49 <knikolla> #endmeeting