| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-15 | |||
| 23:43:15 | mriedem | unless i also stub out the placement stuff | |
| 23:43:32 | melwitt | I think chance_scheduler can't retry. you'd have to check | |
| 23:43:58 | melwitt | it has to have some other way to skip hosts it already tried | |
| 23:44:23 | mriedem | i can try using the PlacementFixture | |
| 23:44:52 | melwitt | I had to use that recently. I can't remember the context | |
| 23:48:15 | mriedem | this time i got one attempt, so getting further | |
| 23:49:01 | melwitt | /o/ | |
| 23:50:17 | mriedem | is that the hey ho thing? | |
| 23:52:03 | dansmith | melwitt: hmm, yea, so I think you'll have to make some compatible changes to the quota object (and thus the quota driver) at the bottom of the stack, then use them, and remove the old stuff at the top | |
| 23:52:23 | dansmith | I could split this out in front: https://review.openstack.org/#/c/416521/22/nova/objects/quotas.py | |
| 23:52:46 | dansmith | except that the changes to the quota driver have to go with it, but I think a lot of that has to go nearish to the end? | |
| 23:53:42 | melwitt | mriedem: I was imagining it as cheering you on. I don't know what the hey ho thing is | |
| 23:54:43 | melwitt | dansmith: yeah, the check_deltas thing was a suggestion from johnthetubaguy as a way to put count/limit_check together behind an abstraction, in hopes that later on we could do something to make the count/check atomic | |
| 23:54:59 | melwitt | so it's just grouping thing really | |
| 23:55:04 | mriedem | melwitt: https://www.youtube.com/watch?v=vr-klwxKhow | |
| 23:55:37 | melwitt | and it makes the code look a lot nicer and be more clear at the call sites | |
| 23:55:49 | dansmith | melwitt: right but it makes all the things that use that interface depend on the changes to the quota driver | |
| 23:55:56 | dansmith | I mean that's obvious, that they depend on them, | |
| 23:56:10 | dansmith | but it makes it hard to merge any of those things early, which are just setup for the main event at the end | |
| 23:56:28 | melwitt | dansmith: yeah | |
| 23:56:31 | dansmith | if instead of modifying the existing quota limit_check thing, you had a new one that did the new behavior, we could merge that and the things that need it, | |
| 23:56:42 | dansmith | then cut over, then remove the old stuff | |
| 23:56:55 | melwitt | oh, okay, I see what you're saying now | |
| 23:57:44 | dansmith | unless the limit check is still usable by old code in it's modified form, | |
| 23:57:54 | dansmith | but so much change was made and new params and stuff I can't really eyeball it | |
| 23:58:26 | dansmith | like, with proj_values=None is it still compatible with the way it was before? | |
| 23:58:31 | melwitt | lemme see. this has been going on for awhile | |
| 23:59:15 | melwitt | I think so. that was what I was thinking, let limit_check work the way it always did, just that you can add project-scoping if you want. which we need for all the new counting stuff | |
| 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 | |