| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 02:18:52 | openstack | Launchpad bug 1670627 in OpenStack Compute (nova) "quota is always in-use after delete the ERROR instances " [Undecided,In progress] - Assigned to huangtianhua (huangtianhua) | |
| 02:18:52 | Kevin_Zheng | Hi, is this a known issue about quotas and cells? https://bugs.launchpad.net/nova/+bug/1670627 | |
| 02:19:04 | Kevin_Zheng | Looks critical | |
| 02:23:14 | melwitt | Kevin_Zheng: we have seen some quota bugs on the multicell patches that are currently in progress, but I hadn't heard about the specific issue with ERROR instances until now | |
| 02:23:54 | Kevin_Zheng | melwitt: Thanks for the reply | |
| 02:24:19 | melwitt | Kevin_Zheng: and I think your analysis is right. we've been working on quotas in regard to cells right now, which will address the issues you're seeing. just didn't realize there were already problems in the wild with the single cell case | |
| 02:24:26 | Kevin_Zheng | melwitt: shouldn't all quota calculated from nova_api db? | |
| 02:24:41 | melwitt | Kevin_Zheng: yes, see this series https://review.openstack.org/#/c/410945/ | |
| 02:25:25 | Kevin_Zheng | melwiit: yeah, I know you are working on quota related issues so I ask here to see if it is already known :) | |
| 02:25:42 | melwitt | it isn't in nova_api db yet, that series moves it there and gets rid of "usages" and "reservations" | |
| 02:26:52 | Kevin_Zheng | Ok, thanks, I will have a look | |
| 02:27:04 | Kevin_Zheng | will it fix the problem we meet? | |
| 02:27:16 | melwitt | ah, cool. yeah, we didn't know about that one yet and I agree it sounds critical. our intention was that things should be okay in the single cell deployment | |
| 02:28:21 | mriedem | Kevin_Zheng: we'll have to fix the bug separately | |
| 02:28:24 | melwitt | yes, it will fix that problem. but I think we'll have to do a small different fix for now, that we can backport | |
| 02:28:26 | mriedem | because we have to backport the fix | |
| 02:28:29 | mriedem | jinx | |
| 02:28:45 | mriedem | so when we bury the instance in cell0, | |
| 02:28:45 | Kevin_Zheng | yeah sounds reasonable | |
| 02:28:46 | melwitt | hah, yeah | |
| 02:28:56 | mriedem | we have to remove the reservation from cell0 | |
| 02:28:59 | mriedem | i think? | |
| 02:29:06 | mriedem | we must be making a claim in cell0 or how does that work? | |
| 02:29:14 | mriedem | s/claim/reservation/ | |
| 02:30:09 | melwitt | thinking ... we're not doing anything to target the reservation/usage/etc | |
| 02:30:41 | melwitt | so it should be in cell0 I think. and that's not getting removed ... so I think you're right | |
| 02:33:24 | mriedem | the quota reservation happens in nova-api here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L312 | |
| 02:33:50 | mriedem | but since that's not targeted to cell0, wouldn't that be in the main cell, i.e. nova database? | |
| 02:34:21 | mriedem | the commit is here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1059 | |
| 02:35:22 | mriedem | https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L836 | |
| 02:35:22 | mriedem | there is no rollback of the quota in _bury_in_cell0 | |
| 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 | |