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