| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 23:59:26 | melwitt | prior to this patch, all of the limit_checks were user-scoped only | |
| #openstack-nova - 2017-03-16 | |||
| 00:00:07 | dansmith | okay then I'll put that and the quota object change up front | |
| 00:01:12 | melwitt | mriedem: oh yeah, I know that song. I guess people must wave their arms when they listen to it | |
| 00:02:52 | mriedem | i know i do | |
| 00:04:59 | melwitt | \o\ | |
| 00:06:03 | melwitt | dansmith: ignore my comment on the incompatible signature, just read your newest comment | |
| 00:06:26 | dansmith | well, I jumped to conclusions since it looked like you change the order, which is only half true | |
| 00:06:28 | dansmith | but yeah | |
| 00:06:34 | dansmith | *changed | |
| 00:07:29 | melwitt | yeah. agreed it would be better to add a new one instead of wedge functionality into the old one | |
| 00:07:53 | dansmith | I'm getting a bunch of quota errors with that limit check in front of basically unchanged code | |
| 00:08:15 | dansmith | like things going negative or something | |
| 00:09:18 | melwitt | well, that's not expected :( | |
| 00:09:39 | dansmith | about to get a report | |
| 00:10:19 | dansmith | http://pastebin.com/raw/BZWnPULS | |
| 00:10:22 | dansmith | for example | |
| 00:12:02 | melwitt | okay, that's because limit_check can't be used on ReservableResources, which is part of the change in nova/quota.py | |
| 00:12:23 | melwitt | sigh | |
| 00:13:01 | dansmith | is that because the last hunk at the bottom of quota.py thinks everything should be reservable? | |
| 00:13:19 | dansmith | i.e., that list should be augmented in each patch in this new list? | |
| 00:14:08 | melwitt | so, the last hunk is the types of the resources, old stuff has a mix of Countable and Reservable. Countable uses limit_check, Reservable uses quota.reserve | |
| 00:14:20 | melwitt | so the third patch changes everything to Countable and limit_check | |
| 00:14:43 | dansmith | right, so each patch in my new series should add its resource type to the list of countable things | |
| 00:14:55 | dansmith | and the limit_check patch being in front should basically have no change in that hunk, right? | |
| 00:15:00 | melwitt | if each patch is changing a reserve to a limit_check, then yeah | |
| 00:15:23 | melwitt | they have to go in lock-step | |
| 00:16:07 | dansmith | melwitt: right, so this is one patch in the series: http://pastebin.com/raw/4Lwcw7EN | |
| 00:16:12 | dansmith | (minus its tests) | |
| 00:16:39 | dansmith | so that one should add CountableResource('server_groups', _server_group_count, 'server_groups'), | |
| 00:16:47 | dansmith | right? | |
| 00:16:58 | melwitt | dansmith: ah yes, that makes it clear. Yeah, not add, but change | |
| 00:17:20 | melwitt | change it from ReservableResource to CountableResource and include it's new counting function | |
| 00:17:24 | melwitt | *its | |
| 00:17:29 | dansmith | ah yeah I see | |
| 00:23:16 | dansmith | okay removing that chunk from the first patch to add the new limit check got me much farther | |
| 00:23:20 | dansmith | 195 fails down to 46 | |
| 00:24:04 | dansmith | pretty much all the rest look like this: AttributeError: <nova.quota.ReservableResource object at 0x7fcbda866310> does not have the attribute 'count' | |
| 00:24:30 | dansmith | which seems to be something in the _get_usages() change that expects something from ReservableResource? | |
| 00:24:51 | melwitt | the opposite, expecting there to be a count attr | |
| 00:25:14 | melwitt | like, it's assuming everything is a CountableResource | |
| 00:25:18 | dansmith | _get_usage hard codes which things it thinks are countable I guess | |
| 00:25:53 | melwitt | yeah, since they were part of the same change. to do both, it would have to check the resource type and do each thing accordingly | |
| 00:26:36 | melwitt | I think. I don't remember what _get_usage did originally. looking | |
| 00:27:15 | melwitt | oh, it's new | |
| 00:27:16 | dansmith | it should probably look at the resource class to see if it's countable, and maybe more details need to be in there to know if it's per-project or something? | |
| 00:27:17 | dansmith | definitely looks like that could be generalized | |
| 00:27:39 | melwitt | okay, yeah. I wrote that to "get counts" when it used to pull usages from the usages table in the db | |
| 00:28:23 | dansmith | okay well I think if that was generalized then this first patch would pass | |
| 00:28:27 | melwitt | like here it used to be a db.quota_usage_get_all_by_project_and_user() call https://review.openstack.org/#/c/416521/22/nova/quota.py@219 | |
| 00:29:08 | melwitt | dansmith: cool, thanks. please feel free to upload as-is and I can fix them. I feel bad you're spending too much time on it | |
| 00:29:30 | dansmith | good, you should feel bad | |
| 00:29:39 | melwitt | :( | |
| 00:29:55 | melwitt | that's proof, the unsmiley face | |
| 00:30:49 | melwitt | looks like we're not out of the woods on this bug either https://bugs.launchpad.net/nova/+bug/1670627 | |
| 00:30:49 | openstack | Launchpad bug 1670627 in OpenStack Compute (nova) ocata "quota is always in-use after delete the ERROR instances " [Critical,In progress] - Assigned to Matt Riedemann (mriedem) | |
| 00:30:56 | melwitt | I dunno if you saw the latest comment | |
| 00:31:38 | dansmith | melwitt: do you really want me to upload this on top of your stack? if so I will, I just don't want to explode your set | |
| 00:31:39 | mriedem | gfdi | |
| 00:31:54 | dansmith | I'm about to retire for the evening anyway | |
| 00:32:04 | melwitt | dansmith: I dunno, is there any other option? | |
| 00:32:06 | mriedem | we do a rollback on the quota change if the instance destroy fails | |
| 00:32:33 | dansmith | melwitt: I can push it up to github or something, but I kinda think we should do this anyway, so that's just indirection | |
| 00:32:45 | dansmith | melwitt: I was just going to try to get it a little closer before pushing, but.. it's up to you | |
| 00:33:25 | melwitt | dansmith: yeah, I guess there's a small chance it could get more gnarly. but maybe that's gonna have to be dealt with anyway. yolo | |
| 00:33:55 | dansmith | having spent hours on the patch the way it is, I don't think it can get much gnarly-er | |
| 00:34:19 | dansmith | I didn't really start grok'ing most of it until I was ass-deep in trying to split it up | |
| 00:34:22 | melwitt | mriedem: if we rollback on a destroy fail, then things should be working right I thought | |
| 00:34:39 | melwitt | dansmith: hah, that's fair. yeah, go ahead and upload | |
| 00:34:54 | mriedem | melwitt: only if we hit InstanceNotFound | |
| 00:34:59 | mriedem | melwitt: they didn't say what error they hit though | |
| 00:39:25 | melwitt | hm, well in _delete_while_booting, it quota.commits immediately, before trying to delete, so that seems wrong. the rollback for InstanceNotFound also wouldn't do anything if commit already happened. but I didn't think we touched _delete_while_booting | |
| 00:40:09 | melwitt | yeah, these are all committing immediately, which I think would make rollback do nothing | |
| 00:40:24 | openstackgerrit | Dan Smith proposed openstack/nova master: Count resources to check quota for cells https://review.openstack.org/416521 | |
| 00:40:25 | openstackgerrit | Dan Smith proposed openstack/nova master: Make Quotas object favor the API database https://review.openstack.org/410945 | |
| 00:40:25 | openstackgerrit | Dan Smith proposed openstack/nova master: Add online migration to move quotas to API database https://review.openstack.org/410946 | |
| 00:40:26 | openstackgerrit | Dan Smith proposed openstack/nova master: Add Quotas.check_deltas() and set the stage for magic to happen https://review.openstack.org/446239 | |
| 00:40:26 | openstackgerrit | Dan Smith proposed openstack/nova master: Count server group quotas https://review.openstack.org/446240 | |
| 00:40:27 | openstackgerrit | Dan Smith proposed openstack/nova master: Count tenant_networks quotas https://review.openstack.org/446241 | |
| 00:40:27 | openstackgerrit | Dan Smith proposed openstack/nova master: Count used_limits quotas https://review.openstack.org/446242 | |
| 00:40:28 | openstackgerrit | Dan Smith proposed openstack/nova master: Remove useless quota_usage_refresh from nova-manage https://review.openstack.org/446243 | |
| 00:40:28 | openstackgerrit | Dan Smith proposed openstack/nova master: Add get_count_by_vm_state() to Instance object https://review.openstack.org/446244 | |
| 00:40:29 | openstackgerrit | Dan Smith proposed openstack/nova master: Add SecurityGroup.get_counts() https://review.openstack.org/446245 | |
| 00:40:29 | openstackgerrit | Dan Smith proposed openstack/nova master: Add FixedIP.get_count_by_project() https://review.openstack.org/446246 | |
| 00:40:30 | openstackgerrit | Dan Smith proposed openstack/nova master: Add FloatingIP.get_count_by_project() https://review.openstack.org/446247 | |
| 00:40:48 | melwitt | thanks dansmith | |
| 00:40:54 | dansmith | the final patch is like half the size | |
| 00:41:29 | melwitt | woohoo | |
| 00:41:42 | dansmith | fwiw, this makes it so much more obvious why this is good: | |
| 00:41:43 | dansmith | https://review.openstack.org/#/c/446239/1 | |
| 00:41:52 | dansmith | in just quota stuff, +500,-1000 | |
| 00:42:20 | dansmith | anyway, I've made my mess, time for dinner | |
| 00:42:22 | melwitt | yeah, that's a win for sure | |
| 00:42:38 | melwitt | haha, o/ | |
| 00:44:00 | melwitt | mriedem: quotas.commit and quotas.rollback are mutually exclusive and I should have noticed that on the review. rollback won't work if commit already happened | |
| 00:49:25 | mriedem | melwitt: then wouldn't it also be wrong in _delete_while_booting? https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1807 | |
| 00:49:28 | mriedem | that's what i copied this from | |
| 00:50:06 | melwitt | yes, I think it's wrong there too | |
| 00:51:18 | melwitt | I'm looking to see what rollback does if commit already occurred | |
| 00:51:55 | melwitt | I think it's just a no-op because it would find no reservation record | |
| 00:52:37 | melwitt | commit would have deleted the reservation record. then rollback comes along and reads the reservation, finds nothing, and then does nothing | |