19:14:03 <lbragstad> #startmeeting keystone-office-hours
19:14:04 <openstack> Meeting started Tue Jun 27 19:14:03 2017 UTC and is due to finish in 60 minutes.  The chair is lbragstad. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:14:06 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
19:14:08 <openstack> The meeting name has been set to 'keystone_office_hours'
19:14:45 <hrybacki> o/
19:14:51 <gagehugo> o/
19:14:53 <lbragstad> o/
19:15:05 <hrybacki> cool. what now?
19:15:14 <lamt> o/
19:16:06 <lbragstad> next - let's go through what we want to try and get done
19:16:10 <lbragstad> this can be patches in review
19:16:15 <lbragstad> or bugs in LP
19:16:34 <hrybacki> what needs help the most right now?
19:17:33 <lbragstad> i use https://goo.gl/ZgrrT7
19:17:35 <lbragstad> #link https://goo.gl/ZgrrT7
19:17:52 <gagehugo> lbragstad oh nice
19:18:03 <hrybacki> same
19:18:17 <lbragstad> the idea came from dstanek
19:18:35 <lbragstad> but the objective was to draw attention to the patches we have in review that already close bugs
19:18:49 <lbragstad> instead of automatically going to launchpad to find something to fix
19:19:26 <lbragstad> not to shamelessly plug my own patch but..
19:19:33 <lbragstad> #link https://review.openstack.org/#/c/475472/ seems to be close
19:20:01 <lbragstad> #link https://review.openstack.org/#/c/475929/ is good
19:20:07 <lbragstad> i can review ^
19:20:20 * lbragstad can't do much, be he can review 7 lines of code
19:20:30 <hrybacki> I'll peak at your commit now lbragstad
19:20:38 <hrybacki> /s/commit/review/
19:20:45 <lbragstad> hrybacki: thanks!
19:20:47 <gagehugo> same
19:21:20 <gagehugo> lbragstad I went with a more generic message for https://review.openstack.org/#/c/475929/
19:21:50 <gagehugo> not sure if we want to tell the user that the backend is ldap
19:24:02 <lbragstad> gagehugo: ok - i think that's fine
19:26:58 <lbragstad> #action review https://review.openstack.org/#/c/475472/
19:27:06 <lbragstad> #action review https://review.openstack.org/#/c/475929/
19:27:20 <lbragstad> #link https://review.openstack.org/#/c/475100/
19:27:31 <lbragstad> ^ that's another one that's ready for review and closes a bug
19:27:54 <lbragstad> #undo
19:27:55 <openstack> Removing item from minutes: #link https://review.openstack.org/#/c/475100/
19:28:09 <lbragstad> maybe not - triaging the bug
19:29:06 <hrybacki> lbragstad: gagehugo general question -- when do we add tests?
19:29:29 <lbragstad> hrybacki: unit tests?
19:29:33 <hrybacki> e.g. for gagehugo's review, would we want to add a test that checks the proper exception is caught?
19:29:35 * hrybacki nods
19:29:55 <gagehugo> hrybacki yeah should probably do that
19:30:28 <lbragstad> hrybacki: yeah - that'd be good
19:30:38 <hrybacki> do we have a general policy for that though?
19:30:39 <lbragstad> gagehugo: think you can find a seem to test that?
19:31:01 <lbragstad> hrybacki: typically every patch should be accompanied with tests
19:31:25 <hrybacki> ack
19:31:31 <lbragstad> some cases where that's not always the case is ldap stuff (because of integration issues and the slippery slope of mocking and cli stuff)
19:31:33 <gagehugo> I'll find where we are testing invalid credentials
19:42:16 <gagehugo> I don't think there are any current ldap unit tests for authing
19:42:44 <hrybacki> that is a bug in and of itself haha
19:43:00 <lamt> it might be in the functional test
19:43:00 <hrybacki> confused why steve gave +2 here: https://review.openstack.org/#/c/477638/ but left corrections in his comments
19:43:07 <lamt> if deals with ldap
19:46:28 <gagehugo> nope
19:46:43 <cmurphy> hey guys
19:46:47 <cmurphy> how is bug day going?
19:46:51 <lbragstad> cmurphy: o/
19:47:01 <hrybacki> o/
19:48:25 <hrybacki> lots of patches to review here cmurphy: https://goo.gl/H86e37
19:48:47 <lbragstad> #link https://review.openstack.org/#/c/473245/3
19:48:54 <lbragstad> ^ that's a good one, too
19:49:09 <cmurphy> hrybacki: lbragstad sweet
20:10:04 <lbragstad> #link https://review.openstack.org/#/c/473245/3 has some good oauth tests in it if anyone is interested
20:10:21 <hrybacki> lbragstad: reviewing that now
20:10:48 <lbragstad> hrybacki: awesome
20:14:26 <hrybacki> I love how documented these test are
20:19:40 <lbragstad> yeah - that's nice
20:20:43 <lbragstad> samueldmq: it'd be good to get your opinion on https://review.openstack.org/#/c/475472/ when you have a minute
20:46:49 <openstackgerrit> Lance Bragstad proposed openstack/keystone master: Validate rolling upgrade is run in order  https://review.openstack.org/437441
20:47:17 <lbragstad> cc cmurphy hrybacki gagehugo ^
20:54:12 <openstackgerrit> Lance Bragstad proposed openstack/keystone master: Validate rolling upgrade is run in order  https://review.openstack.org/437441
21:00:54 * samueldmq is back
21:02:16 <samueldmq> lbragstad: looking
21:04:03 <hrybacki> Have to head to my next meeting -- will finish looking over ^ first thing in the morning!
21:09:02 <morgan> I saw a ping of my name in the meetings ng
21:09:08 <sjmc7> afternoon, folks. we’ve been doing some scale/stress testing and seen high request times listing projects (specifically, GET /users/<userid>/projects) when a user has access to a lot of them (0.1 seconds with a couple of assignments up to 4-5 seconds with 500). obviously there’s much more data being pulled out of the database, but flat out listing the projects is very fast, so my guess is it’s the role assignment calculations taking the time
21:09:15 <morgan> Something about options?
21:09:35 <sjmc7> does anyone have any suggestions if there’s something i can tweak or optimize? i know it’s not a common scenario
21:10:11 <morgan> sjmc7: yep it is the role calculation. Unfortunately that is going to be somewhat slow :(
21:10:34 <sjmc7> no magic SQL index i can add? :)
21:10:54 <morgan> Not really. We can't do joins because that information could be in different backends
21:11:28 <lbragstad> morgan: ah - we were wondering when we can remove https://github.com/openstack/keystone/blob/9070172084fe31c9564de38886662fb198de68cb/keystone/conf/eventlet_server.py
21:11:30 <morgan> User might be LDAP, project might be database, and roles could be something else. :(
21:11:37 <sjmc7> yeah, that makes sense
21:11:38 <morgan> lbragstad: remove it
21:11:46 <morgan> lbragstad: eventlet is dead
21:12:00 <morgan> For keystone *
21:12:04 <lbragstad> morgan: ok - so those weren't there for backwards compat?
21:12:35 <morgan> Might have been, we used eventlet for testing for a while after we stopped supporting it for deployment
21:12:44 <morgan> Might have been that. We don't do that anymore
21:12:49 <morgan> (or shouldn't)
21:12:53 <lbragstad> we still have the eventlet entry points
21:13:20 <morgan> Eventlet should be dead, no?
21:13:35 <morgan> We don't import it, we don't depend on ot
21:13:44 <lbragstad> morgan: well - i can run keystone locally using keystone-wsgi-admin
21:13:53 <morgan> Again, I say make the last vestige disappear.
21:15:06 <openstackgerrit> Lance Bragstad proposed openstack/keystone master: Document and add release note for HEAD APIs  https://review.openstack.org/478284
21:15:47 <openstackgerrit> Lance Bragstad proposed openstack/keystone master: Document and add release note for HEAD APIs  https://review.openstack.org/478284
21:16:03 <lbragstad> morgan: ok
21:18:07 <lbragstad> cc cmurphy samueldmq hrybacki gagehugo ^ that one closes another bug
21:19:34 <sjmc7> morgan: do you know where the time’s getting spent? querying role assignments is also very fast - is  there a lot of manipulation of the result going on in code?
21:20:20 <lbragstad> sjmc7: do you happen to have caching enabled?
21:21:04 <sjmc7> no, we don’t
21:21:33 <lbragstad> sjmc7: that may have a significant impact on your results
21:22:13 <sjmc7> yeah, i imagine it would. i’ll give that a go, thanks
21:26:26 <openstackgerrit> Lance Bragstad proposed openstack/keystone master: Remove duplicate list() call when list projects  https://review.openstack.org/478286
21:26:30 <lbragstad> sjmc7: ^ looking at the code now
21:27:05 <sjmc7> lbragstad: thanks. i had a look, and it looked pretty straightforward (retrieve the assignments, retrieve all the projects by id)
21:28:01 <sjmc7> but there’s something goign on - listing assignments with GET /role_assignments?user.id takes around 0.1 seconds and GET /projects is about 0.2 (including outputting to the terminal)
21:28:33 <sjmc7> again, i know this isn’t exactly a common case that’d be optimized for
21:28:46 <lbragstad> we seem to jump from https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/assignment/core.py#L232
21:28:58 <lbragstad> right to the resource driver https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/resource/backends/sql.py#L85-L93
21:29:33 <sjmc7> yeah
21:29:46 <lbragstad> sjmc7: do you have any insight or ability to time each line in https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/assignment/core.py#L226 ?
21:30:56 <sjmc7> yeah, i can do that
21:31:56 <lbragstad> sjmc7: hmmmm
21:32:07 <sjmc7> i think this might be an old version (newton?) but it doesn’t look like it’s changed much
21:32:15 <lbragstad> sjmc7: since we don't define that method in the resource manager, i wonder if it's even possible to cache it ?
21:32:20 <samueldmq> lbragstad: done in the GET/HEAD reviews
21:32:38 <sjmc7> # TODO(henry-nash): We might want to consider list limiting this at some
21:32:39 <sjmc7> # point in the future.
21:32:40 <samueldmq> I will need a bit of time to test that one in the db_sync check in my environment
21:32:41 <sjmc7> someoen was here before me :)
21:32:53 <lbragstad> samueldmq: awesome - thanks for the feedback
21:32:59 <samueldmq> sure
21:34:23 <lbragstad> sjmc7: we typically implement caching based on method arguments
21:34:26 <openstackgerrit> Gage Hugo proposed openstack/keystone master: Clarify LDAP invalid credentials exception  https://review.openstack.org/475929
21:34:27 <lbragstad> sjmc7: like this https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/token/provider.py#L180
21:35:09 <gagehugo> lbragstad hrybacki ^
21:35:18 <lbragstad> gagehugo: running tests on it now
21:35:33 <gagehugo> also I noticed that one added test is being ran 8 times
21:36:10 <lbragstad> gagehugo: it's probably because that test module is being inherited in other places
21:36:17 <gagehugo> lbragstad yeah
21:36:40 <gagehugo> I wonder if that could be cleaned up?
21:36:42 <lbragstad> i have a hard time wrapping my head around the dependencies of ldap/sql test modules
21:36:49 <lbragstad> gagehugo: oh - i'm sure it could
21:37:23 <lbragstad> sjmc7: since we don't have those methods defined in the resource manager, i'm not sure it's possible to cache them
21:37:34 <sjmc7> that’s sad :(
21:37:39 <lbragstad> sjmc7: i'd be curious to see if your results change with caching enabled
21:37:47 <lbragstad> sjmc7: if not - that's certainly a bug
21:38:20 <lbragstad> sjmc7: and we can get a fix up easy enough and possibly backported to ocata (we won't be able to backport to newton though)
21:38:51 <lbragstad> sjmc7: it was likely missed because the method is defined in the driver and there isn't much business logic to move it up to the manager
21:40:56 <sjmc7> ok. just generating timing info now
21:42:53 <cmurphy> I don't really know what to make of https://review.openstack.org/#/c/466567 - I'm trying to curl those APIs and they're broken because there's no pki_setup
21:44:18 <sjmc7> lbragstad: looks like all the time’s taken inside list_role_assignments
21:44:21 <openstackgerrit> Lance Bragstad proposed openstack/keystone master: Move list projects from ids to manager and cache  https://review.openstack.org/478293
21:45:02 <lbragstad> sjmc7: that's a total wild guess
21:45:04 <lbragstad> ^
21:45:55 <lbragstad> sjmc7: here - https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/assignment/core.py#L227 ?
21:46:05 <sjmc7> yep
21:46:11 <sjmc7> i’ll jam some timing info in there
21:47:02 <lbragstad> sjmc7: hmm - https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/assignment/backends/sql.py#L190-L247 doesn't look trivial
21:47:39 <sjmc7> no, but calling GET /role_assignments?user.id= *is* quick
21:50:55 <lbragstad> sjmc7: hmm - which eventually calls into https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/assignment/core.py#L882
21:53:13 <sjmc7> those role assignment functions do get complicated quick
21:53:45 <lbragstad> sjmc7: both of those API end up calling the same driver method
21:53:50 <sjmc7> yeah :(
21:54:32 <gagehugo> lbragstad I'm getting 2 failures, I wonder if I can move this test to another class where it won't be ran 8 times
21:54:45 <lbragstad> gagehugo: yeah - i got a couple, too
21:55:29 <lbragstad> sjmc7: i wonder if it's a difference of using `effective` or not
21:55:32 <lbragstad> sjmc7: https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/assignment/core.py#L228
21:55:56 <sjmc7> yeah, there’s a branch on that
21:56:00 <sjmc7> gonna put some more timing around it
21:56:09 <lbragstad> the GET /role_assignment?user.id= API doesn't calculate effective role assignments unless you ask it to
21:56:25 <lbragstad> sjmc7: but... list_projects_for_users does no matter what
21:56:32 <sjmc7> ah, just spits them out from the DB
21:57:19 <sjmc7> the list_effective function i can see would be expensive
21:58:37 <lbragstad> here - https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/assignment/core.py#L931-L935
21:58:49 <lbragstad> yeah - maybe time that?
21:59:07 <sjmc7> yep
21:59:44 <lbragstad> sjmc7: https://github.com/openstack/keystone/blob/a1976aa2c9b70de30ea6f646b430bd49f82dbcc1/keystone/assignment/core.py#L692 looks intense, too
22:03:12 <sjmc7> yep :)
22:05:12 <sjmc7> yeah, so calling /users/abc/projects list_effective is taking around 4 seconds. list_direct is very fast
22:05:39 <lbragstad> sjmc7: so - sounds like effectively role assignments could be improved
22:06:24 <sjmc7> yeah, looks like it. it seems it’s unlikely to be the database since wit a few hundred rows even a full table scan will be very quick
22:06:53 <lbragstad> yeah - effective role assignment probably has a lot of marshalling in python
22:07:02 <lbragstad> which is more likely the case
22:08:37 <lbragstad> sjmc7: would you be able to open a bug against keystone and include your timing?
22:08:55 <sjmc7> yeah, will do
22:09:05 <lbragstad> sjmc7: thanks
22:09:47 <lbragstad> hrybacki: gagehugo cmurphy lamt that about does it for office hours
22:09:50 <lbragstad> thanks for coming!
22:09:54 <lbragstad> #endmeeting