| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-05-15 | |||
| 18:44:00 | mnaser | explain in my case shows scanning 1.2m rows. yes, we could use an archive, but.. | |
| 18:44:06 | mnaser | https://www.irccloud.com/pastebin/vmPO2rNN/ | |
| 18:45:23 | mnaser | it sounds like this is the root: https://github.com/openstack/nova/blob/master/nova/objects/cell_mapping.py#L275-L285 | |
| 18:46:16 | mnaser | which i guess comes from https://github.com/openstack/nova/blob/master/nova/compute/instance_list.py#L150-L153 | |
| 18:52:40 | mnaser | which leads to more interesting things. instance_mappings seem to be not soft deleted yet i have 2586748 records... | |
| 19:23:32 | dansmith | mnaser: instance mappings are not soft deleted, | |
| 19:23:38 | dansmith | and are only ever removed when you archive | |
| 19:24:29 | mnaser | dansmith: ah. i'll try and add archive to happen more often because i think teh annoying thing is once a cloud grows big enough, the archive really hurt in terms of performance | |
| 19:25:09 | dansmith | yeah, it's not a very lightweight thing | |
| 19:25:40 | dansmith | mnaser: I guess we need to filter that instance mappings query by deleted eh? | |
| 19:25:51 | dansmith | er, no we can't | |
| 19:25:52 | dansmith | duh | |
| 19:25:54 | mnaser | yeah :( | |
| 19:26:03 | mnaser | i mean there's the queued_for_delete, i saw a lot of records with that | |
| 19:26:13 | dansmith | you'd think it would be pretty fast though | |
| 19:26:27 | dansmith | becase it's just distinct cell_id, and then filtered by project | |
| 19:26:34 | dansmith | maybe we're missing an index | |
| 19:27:08 | mnaser | the select distinct is def what hurts, im trying to run it manually and see how long it takes.. | |
| 19:27:21 | mnaser | 16.02 seconds | |
| 19:27:29 | dansmith | there is an index on project_id, but not on cell_id | |
| 19:27:42 | dansmith | so maybe that is making the distinct take a long time? | |
| 19:27:55 | mnaser | dansmith: i think the other problem is that t his code is also used for quota checks, so thats ~16s per *single* request to boot an instance | |
| 19:28:16 | mnaser | when we'd see a big surge of instance creations, that's 16s*20 or so and it starts getting worse | |
| 19:28:34 | mnaser | specifically here https://github.com/openstack/nova/blob/575a91ff5be79ac35aef4b61d84c78c693693304/nova/quota.py#L1194-L1195 | |
| 19:28:55 | dansmith | apparently mysql always indexes foreign key columns, so cell_id should be indexed | |
| 19:29:08 | mnaser | https://www.irccloud.com/pastebin/MftvYe3g/ | |
| 19:29:10 | mnaser | thats the EXPLAIN | |
| 19:29:23 | dansmith | mnaser: you specifically have the subset conf turned on, is that intentional? | |
| 19:29:53 | mnaser | dansmith: sorry, nto following, subset conf? | |
| 19:30:09 | dansmith | mnaser: see the comment just above the code in instance_list ? | |
| 19:30:16 | dansmith | query_cell_subset = CONF.api.instance_list_per_project_cells | |
| 19:30:25 | dansmith | if that is off then it won't do this | |
| 19:31:01 | dansmith | defaults to false | |
| 19:31:36 | dansmith | heh, the conf text (written by me I'm sure) says turning that on "causes one more (fast) query to the API database" | |
| 19:31:48 | mnaser | heh, let me check if its enabled | |
| 19:31:55 | dansmith | maybe I should edit that comment or say "assuming you're archiving regularly" :) | |
| 19:32:08 | mnaser | instance_list_per_project_cells is not set to anything so its probably set to default False | |
| 19:32:26 | dansmith | mnaser: okay well you see the code, not sure how you could be runningthat | |
| 19:32:41 | mnaser | dansmith: https://github.com/openstack/nova/blob/575a91ff5be79ac35aef4b61d84c78c693693304/nova/quota.py#L1194-L1195 ? | |
| 19:32:55 | mnaser | that doesnt seem to respect it | |
| 19:33:04 | dansmith | mnaser: no, the instance list one | |
| 19:33:17 | dansmith | maybe you're not actually hitting the instance_list code, just the quota one? | |
| 19:33:29 | mnaser | dansmith: yes, sorry, should have clarified, i think that's the running theory | |
| 19:33:36 | dansmith | okay I thought you assumed both | |
| 19:33:44 | dansmith | I wrote instance_list so was focusing there | |
| 19:33:44 | mnaser | i'm looking at logs and os_volume-boot is taking some ~45-60s-ish to respond | |
| 19:33:56 | mnaser | yeah, initially that was my two findings, but i agree, it seems quota code might be what is hurting | |
| 19:34:03 | dansmith | gotcha | |
| 19:34:05 | dansmith | melwitt: ^ | |
| 19:34:33 | dansmith | mnaser: maybe we could make the quota code check that same conf, as for <=1 real cell, it's probably quicker not to do the query anywa | |
| 19:35:21 | mnaser | dansmith: yes, given we want to backport though, i'd be tempted to actually change get_by_project_id behaviour to respect that config value so its not possible for someone to miss it later | |
| 19:35:39 | dansmith | no, we definitely don't want that | |
| 19:35:40 | mnaser | (for the future, but backport just fixing the behaviour in quota.py) | |
| 19:36:03 | mnaser | im sure there's details im missing why its not a good idea :) | |
| 19:36:07 | dansmith | if a caller is looking for a set of instance mappings, you want it to get what it asked for, not altered by some random config flag | |
| 19:37:54 | mnaser | yep makes sense | |
| 19:38:12 | mnaser | the patch to fix this seems trivial, so ill push something up | |
| 19:38:17 | dansmith | that's a very low-level database query routine.. the "should we optimize for lots of cells or not" is a very high-level decision | |
| 19:38:38 | dansmith | mnaser: cool, add me and melwitt, but the latter is the important review I thnk | |
| 19:39:50 | mnaser | dansmith: will do. i'll write up a bug right now too | |
| 19:40:02 | dansmith | thanks | |
| 19:40:06 | mnaser | dansmith: i could also workaronud this by enabling quota from placement, but yeah | |
| 19:40:08 | mnaser | let's get it fixed | |
| 19:40:22 | dansmith | mnaser: you could also archive your damned database now and then :D | |
| 19:40:32 | mnaser | dansmith: haha, always me bringing up those massive db issues :p | |
| 19:40:57 | dansmith | I guess the benefit of not archiving is giving us some data on these things, so ... your choice :D | |
| 19:53:27 | mnaser | it's friday and i did a bad job at writing this probably: https://bugs.launchpad.net/nova/+bug/1878979 | |
| 19:53:27 | openstack | Launchpad bug 1878979 in OpenStack Compute (nova) "Quota code does not respect [api]/instance_list_per_project_cells" [Undecided,New] | |
| 20:08:59 | mriedem | mnaser: related https://bugs.launchpad.net/nova/+bug/1857306 | |
| 20:08:59 | openstack | Launchpad bug 1857306 in OpenStack Compute (nova) "_bury_in_cell0 could not handle instance duplicate exception" [Low,Triaged] | |
| 20:09:10 | mriedem | wait a minute, that's not right | |
| 20:09:34 | mriedem | https://review.opendev.org/#/c/700456/ | |
| 20:12:38 | mnaser | mriedem: ehhhh, that's a lot to think about for friday :( | |
| 20:13:46 | mriedem | you're stuck inside, what else do you have to do | |
| 20:37:23 | mnaser | welp, _instances_cores_ram_count_legacy has zero tests | |
| 20:47:03 | mriedem | i think those are covered with functional tests | |
| 20:47:11 | mriedem | depending on if the placement flag is configured to use placement for quota or not | |
| 20:47:28 | mnaser | mriedem: yeah i found some unit test ones but they mostly mock that function out | |
| 20:47:45 | mriedem | yeah it's functional tests i'm pretty sure | |
| 20:47:53 | mriedem | more reliable for those kind of tests hitting the dbs | |
| 20:47:57 | mnaser | mriedem: ok thanks ill go dig there | |
| 20:48:24 | mnaser | mriedem: my test is mostly "if flag X is enabled then make sure you call with all cells otherwise make sure you retrieve list of cells | |
| 20:49:25 | openstackgerrit | Mohammed Naser proposed openstack/nova master: WIP: Make quotas respect instance_list_per_project_cells https://review.opendev.org/728575 | |
| 20:49:26 | mnaser | mriedem: ^ for context | |
| 20:49:54 | mnaser | yeah that code is broken now that i look at it again | |
| 20:50:23 | mnaser | ah no nvm, python lets vars defined in if get accessed outside | |
| 20:52:42 | openstackgerrit | Lin Yang proposed openstack/os-traits master: CPU: add traits for new AVX512 support https://review.opendev.org/728576 | |
| 21:15:29 | openstackgerrit | Lin Yang proposed openstack/os-traits master: CPU: add traits for new AVX512 support https://review.opendev.org/728576 | |
| 21:24:06 | openstackgerrit | Mohammed Naser proposed openstack/nova master: Make quotas respect instance_list_per_project_cells https://review.opendev.org/728575 | |
| 21:24:16 | mnaser | dansmith, melwitt: ^ cooked up and tested locally :) thanks for the hints | |
| 21:28:36 | openstackgerrit | Gage Hugo proposed openstack/nova stable/stein: [DNM] - testing numa placement https://review.opendev.org/728579 | |
| 22:32:48 | melwitt | mnaser: here's the func test for cores ram count https://github.com/openstack/nova/blob/master/nova/tests/functional/db/test_quota.py#L142 | |
| 22:33:21 | melwitt | oh you found it. nevermind | |
| 22:35:52 | mnaser | melwitt: :D | |
| 22:36:25 | mnaser | I tried to make it as backportable as possible | |
| 22:37:00 | melwitt | good call. this doesn't look too bad at all, so thank you for that | |
| 22:38:09 | melwitt | mnaser: I assume you tried out this patch? what was the perf result, just curious | |
| 22:39:50 | ikla | I'm trying to get pci_passthrough to work and followed the documents. I keep seeing Pci claimed failed. Anyway to drill this down. Config looks perfect. | |
| 22:40:34 | mnaser | melwitt: I have not yet actually! It’s Friday and the load dropped so it wasn’t hitting as hard | |
| 22:41:07 | mnaser | I can try on Monday though but I imagine it’ll drop scanning instance mappings and I only have 2 cells really (0 and 1) | |