| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 02:34:21 | mriedem | the commit is here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1059 | |
| 02:35:22 | mriedem | there is no rollback of the quota in _bury_in_cell0 | |
| 02:35:22 | mriedem | https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L836 | |
| 02:35:50 | mriedem | which probably makes sense because don't we want to keep the reservation until the instance is actually deleted? | |
| 02:35:53 | melwitt | there shouldn't be because ERROR instances occupy quota | |
| 02:36:23 | melwitt | but when the user deletes it, we need to do a reverse reserve and commit and somehow that's going to the wrong db | |
| 02:37:18 | mriedem | we put the instance in cell0 here https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L881 | |
| 02:37:19 | melwitt | it's probably something like, we target cell0 to delete the instance and the quota reserve/commit for the freeing of quota goes to cell0 in the same target and we didn't mean to | |
| 02:37:31 | melwitt | or something | |
| 02:38:19 | mriedem | yeah, so we made the reservation in the nova db, but put the instance in the nova_cell0 db, and right when we delete i bet we find it in cell0 via the instance mapping | |
| 02:40:11 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1708 | |
| 02:40:12 | melwitt | yeah, I'm looking in compute/api.py and I'm not seeing any freeing of quota for local deletes | |
| 02:40:15 | mriedem | i think deleted would be False there | |
| 02:40:19 | mriedem | because conductor deleted the build request | |
| 02:40:40 | mriedem | we delete the build request in _bury_in_cell0 here https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L888 | |
| 02:41:09 | melwitt | yeah. so far it's looking to me like we're missing a quotas.commit() for the local delete cases | |
| 02:41:48 | melwitt | like if we're deleting from here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1799 | |
| 02:42:10 | melwitt | this would be if it never landed on a host | |
| 02:43:14 | melwitt | we're not doing anything with quotas until L1842 but you have to have reserve and commit for a delete too | |
| 02:43:18 | mriedem | yeah i was looking at _lookup_instance https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1660 | |
| 02:43:56 | mriedem | and we pull it from cell0 here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1690 | |
| 02:44:47 | mriedem | looks like we need something like this https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1729-L1756 | |
| 02:44:56 | melwitt | I see that _delete_while_booting takes care of quotas correctly, it's just we're missing any quota handling at all at L1799 | |
| 02:45:12 | melwitt | yes, agree | |
| 02:47:51 | mriedem | should be relatively straightforward | |
| 02:48:43 | melwitt | yeah, I think so too | |
| 02:50:42 | melwitt | thanks Kevin_Zheng for alerting about that | |
| 02:51:27 | Kevin_Zheng | melwitt: your are welcome, still try to catch all you guys discussed :) | |
| 02:52:02 | melwitt | Kevin_Zheng: mriedem added a comment to the lp bug to explain what was concluded | |
| 02:52:20 | melwitt | it turned out to be something pretty simple | |
| 02:53:07 | melwitt | that particular local delete case is missing quota cleanup | |
| 02:53:40 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Decrement quota when deleting an instance in cell0 https://review.openstack.org/443395 | |
| 02:53:41 | mriedem | melwitt: Kevin_Zheng: ^ w/o any tests run or anything | |
| 02:56:38 | Kevin_Zheng | https://review.openstack.org/#/c/443003 | |
| 02:56:54 | Kevin_Zheng | ^ seems we have already tried this but didnt work? | |
| 02:58:02 | melwitt | gah, I didn't look at the patch first | |
| 02:59:02 | Kevin_Zheng | we tested manually | |
| 02:59:30 | Kevin_Zheng | fix like this actually decrease usage in cell0 | |
| 02:59:36 | melwitt | I'm trying to understand why that code is going to cell0 | |
| 03:01:20 | melwitt | mriedem: ^ | |
| 03:01:42 | mriedem | yeah idk | |
| 03:01:51 | mriedem | are we mutating the context somewhere? | |
| 03:03:24 | melwitt | I thought probably not. I'm not sure if we yet merged the thing method that targets a context and lets it continue on targeted | |
| 03:03:32 | melwitt | and also I didn't think it would be used in this case | |
| 03:08:04 | mriedem | that definitely came later | |
| 03:08:07 | mriedem | set_target_cell | |
| 03:09:02 | melwitt | grr this doesn't make sense | |
| 03:12:12 | mriedem | i would have to recreate locally and tinker with this, | |
| 03:12:15 | mriedem | which i don't have time tonight | |
| 03:12:29 | mriedem | Kevin_Zheng: the instance is in cell0 though correct? | |
| 03:12:33 | mriedem | and the build_request is deleted? | |
| 03:15:30 | Kevin_Zheng | yes, we are have meeting now, but I will try to dig more if you guys didn't have tien | |
| 03:15:32 | Kevin_Zheng | Time | |
| 03:15:57 | mriedem | melwitt: i think i found the problem | |
| 03:16:06 | melwitt | do tell | |
| 03:16:18 | mriedem | gather round children | |
| 03:16:24 | melwitt | heh | |
| 03:16:26 | mriedem | so when the REST API looks up the instance via id | |
| 03:16:34 | mriedem | it goes down to compute.API.get() | |
| 03:16:59 | mriedem | eventually to _get_instance | |
| 03:17:12 | mriedem | we get the cell0 instance mapping here https://github.com/openstack/nova/blob/fa2b4a82648101826566da68dd56d204e269853f/nova/compute/api.py#L2292 | |
| 03:17:14 | Kevin_Zheng | Ah | |
| 03:17:22 | mriedem | and then we mutate the context here https://github.com/openstack/nova/blob/fa2b4a82648101826566da68dd56d204e269853f/nova/compute/api.py#L2294 | |
| 03:17:33 | mriedem | and forever after, that context object is targeted to cell0 | |
| 03:17:59 | melwitt | argh, so we are already using set_target_cell | |
| 03:18:43 | Kevin_Zheng | Seems right | |
| 03:20:06 | mriedem | dansmith: ^ set_target_cell might be screwing us here | |
| 03:20:15 | mriedem | melwitt: on master yes | |
| 03:20:17 | mriedem | but not ocata | |
| 03:20:58 | melwitt | I wonder if we should avoid using it until everything that needs to move to api db moves, or try to just patch around it for now? | |
| 03:21:00 | mriedem | we still have the bug in ocata | |
| 03:21:11 | mriedem | but i think that's the reason why the guys patch didn't work | |
| 03:21:17 | mriedem | and decremented from cell0 | |
| 03:21:34 | dansmith | the only time I've experienced this is if the main db for the control services is set to cell0 | |
| 03:21:51 | dansmith | it shouldn't happen with a single cell and database set to that db right? | |
| 03:22:47 | mriedem | i believe this is single node devstack | |
| 03:22:48 | Kevin_Zheng | We meet this in devstack env, didn't test in other cases yet | |
| 03:22:48 | melwitt | dansmith: this is the quota_usages and reservations tables, so it's updating usages in cell0 | |
| 03:23:05 | mriedem | dansmith: yeah see https://review.openstack.org/443395 | |
| 03:23:08 | melwitt | after set_target_cell targets the context to cell0 | |
| 03:23:25 | mriedem | which doesn't work because it tries to decrement from cell0 because the context is mutated to use cell0 when we lookup the instance here https://github.com/openstack/nova/blob/fa2b4a82648101826566da68dd56d204e269853f/nova/compute/api.py#L2294 | |
| 03:24:47 | dansmith | I'm confused, nothing ever moves between cell0 and another cell.. | |
| 03:24:48 | mriedem | we make the reservation on quota in the nova db in the api | |
| 03:25:00 | mriedem | then conductor fails to schedule and creates the instance in cell0 | |
| 03:25:03 | mriedem | and deletes the build request | |
| 03:25:10 | dansmith | ah | |
| 03:25:11 | mriedem | when you go to delete the instance, we dont find the build request, | |
| 03:25:18 | mriedem | but we find the instance mapping to cell0 | |
| 03:25:22 | melwitt | dansmith: this is a delete of an ERROR instance in cell0. the REST API does a get of the instance, which set_target_cell to cell0 on the context, then that same context is passed along to quotas.commit() and it decrements usage in the wrong db | |
| 03:25:22 | mriedem | and we delete the instance from the cell0 db | |
| 03:25:27 | mriedem | but we don't decrement the quota | |
| 03:25:47 | mriedem | melwitt: well, w/o https://review.openstack.org/#/c/443395/ we don't even attempt to decrement quota | |
| 03:25:47 | melwitt | dansmith: (after the person tried to add the missing quota cleanup) | |
| 03:25:49 | dansmith | and this is because we don't have the reservations in the api I guess? | |
| 03:25:53 | mriedem | dansmith: yup | |
| 03:26:18 | dansmith | awesome | |
| 03:27:27 | melwitt | mriedem, dansmith: right. so 1) we're missing quota cleanup 2) when we add quota cleanup, it cleans the wrong db because the context was targeted to cell0 early on | |
| 03:27:28 | dansmith | has melwitt pointed out that counting fixes all this nonsense? | |
| 03:27:28 | mriedem | yes | |
| 03:27:28 | dansmith | okay good | |