| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-14 | |||
| 17:07:44 | siva_krishnan | johnthetubaguy: sure. will update the commit message | |
| 17:08:01 | mdbooth | I've read the description and use cases sections a couple of times and hadn't managed to infer it from context. | |
| 17:08:08 | mdbooth | That could just be me, though. | |
| 17:09:30 | openstackgerrit | John Garbutt proposed openstack/nova-specs master: Add locked-by-service spec https://review.openstack.org/438134 | |
| 17:10:21 | johnthetubaguy | mdbooth: not sure, the folks reading that so far have a lot of shared context around the problem | |
| 17:12:29 | openstackgerrit | Gábor Antal proposed openstack/nova master: Add json style checking for sample notifications https://review.openstack.org/443677 | |
| 17:18:57 | johnthetubaguy | siva_krishnan: thanks | |
| 17:19:20 | mdbooth | johnthetubaguy: Incidentally, you mentioned (last week, I think) you'd have another look at https://review.openstack.org/#/c/383859/ | |
| 17:20:02 | johnthetubaguy | mdbooth: damm, yes, still not got out of the specs really | |
| 17:20:54 | mdbooth | I'd really like to get that landed, or at least know what needs doing to it. | |
| 17:21:08 | mdbooth | It's a bit in limbo right now. | |
| 17:21:53 | openstackgerrit | Sivasathurappan Radhakrishnan proposed openstack/nova-specs master: Live Migration of Rescued Instances https://review.openstack.org/347161 | |
| 17:22:11 | jaypipes | mriedem, dansmith: your reviews on https://review.openstack.org/#/c/441544/ would be great. thanks in advance! | |
| 17:22:15 | johnthetubaguy | siva_krishnan: acutally that spec is really different from the previously approved one: http://specs.openstack.org/openstack/nova-specs/specs/newton/approved/live-migrate-rescued-instances.html | |
| 17:22:31 | mriedem | jaypipes: ack | |
| 17:22:46 | johnthetubaguy | mdbooth: understood, I hope to get there really soon | |
| 17:22:47 | sdague | mriedem: you are going to have to explain the flavor param on https://review.openstack.org/#/c/445236/1to me | |
| 17:22:55 | jaypipes | dansmith: what's your most pressing patch for me to review? | |
| 17:23:11 | sdague | because I'm not seeing any logic that makes that a different value in the new code than the old code | |
| 17:23:34 | dansmith | sdague: it's because of lazy loading | |
| 17:23:40 | dansmith | sdague: needing to go to the right cell when that happens | |
| 17:24:00 | dansmith | jaypipes: I'm currently moving code around so.. nothing right now :( | |
| 17:24:16 | mriedem | sdague: must did | |
| 17:24:18 | mriedem | *just did | |
| 17:24:20 | mdbooth | johnthetubaguy: Thanks! | |
| 17:24:25 | dansmith | jaypipes: you can look at this if you want: https://review.openstack.org/#/c/427903 | |
| 17:24:28 | mriedem | sdague: it's all about cell context | |
| 17:24:35 | jaypipes | dansmith: cool, will do. | |
| 17:24:40 | dansmith | but it's not super pressing | |
| 17:25:13 | dansmith | jaypipes: not sure I have enough context in those commit messages for you to know why that's important actually.. unless you were paying attention at the PTG :) | |
| 17:25:31 | siva_krishnan | johnthetubaguy: we have almost changed all the textual content of previously approved spec but functionality wise this was the only change we agreed on. | |
| 17:25:32 | mriedem | dansmith: nor the bp description :) | |
| 17:26:00 | dansmith | I know | |
| 17:26:03 | jaypipes | dansmith: I'll ask if I don't understand :) | |
| 17:26:16 | mriedem | i'll add notes to the bp | |
| 17:29:51 | mriedem | dansmith: there https://blueprints.launchpad.net/nova/+spec/discover-hosts-faster | |
| 17:30:25 | dansmith | mriedem: <3 | |
| 17:30:27 | dansmith | jaypipes: ^ | |
| 17:30:32 | sdague | mriedem / dansmith ok, so you end up having to change context to look up the flavor, because it's only in the the cell db ? | |
| 17:30:44 | mriedem | sdague: the instance is in the cell0 db | |
| 17:30:45 | sdague | but then you are making all the quota changes at the api db? | |
| 17:30:49 | mriedem | the context, by default, looks in the nova db | |
| 17:30:58 | jaypipes | bueno. | |
| 17:31:01 | mriedem | no, the quota changes are in the nova db | |
| 17:31:13 | mriedem | quotas aren't in the api db yet, which is why we have this bug | |
| 17:31:19 | dansmith | well, | |
| 17:31:20 | dansmith | not really | |
| 17:31:26 | dansmith | but if he buys that, then run with it | |
| 17:31:29 | dansmith | oops, was that public? | |
| 17:31:34 | mriedem | ha | |
| 17:31:42 | mriedem | you typed too fast | |
| 17:32:05 | dansmith | sdague: api hedged and put the quota reservation in the non-cell0 cell the instance was destined for, | |
| 17:32:14 | dansmith | but then we put it in cell0 when we couldn't schedule it | |
| 17:32:20 | sdague | so, I guess I'm not sure I understand why the internals interface doesn't make instance.flavor just go lazy load from the right place | |
| 17:32:25 | dansmith | sdague: once our quotas are "fixed for cells" then this won't be a problem | |
| 17:32:53 | sdague | or is that just because this was a late breaking bug, and this was the smallest backportable thing? | |
| 17:33:09 | dansmith | sdague: because the instance is now in cell0, | |
| 17:33:17 | dansmith | sdague: but the reservations were created in cellN in a previous request | |
| 17:34:36 | mriedem | "why the internals interface doesn't make instance.flavor just go lazy load from the right place" the instance object would have to lookup it's own instance mapping to find the cell it's in, and then do a lookup using that context i believe, | |
| 17:34:48 | dansmith | no, it's worse | |
| 17:34:49 | mriedem | which could probably work, but wasn't something i thought of doing, at least in the mainline path | |
| 17:34:58 | dansmith | because the reservations are in the cell it's not in, right? | |
| 17:35:05 | mriedem | correct, | |
| 17:35:18 | sdague | I don't know, I just see you did a lookup call to figure out what cell the instance is in right above it | |
| 17:35:25 | mriedem | but i think sdague is asking why i have to be explicit about looking up the instance.flavor in the proper cell, rather than instance.flavor lazy-load doing that for us implicitly | |
| 17:35:32 | sdague | so from a naive pov, it seems like that could be stashed in instance itself | |
| 17:36:15 | mriedem | we could handle InstanceNotFound in the obj_load_attr in the Instance object and if the context isn't targeted for a cell (no db_connection attr) we could do an instance mapping lookup on ourselves | |
| 17:36:16 | dansmith | sdague: because that's not how it works right now | |
| 17:36:21 | dansmith | :) | |
| 17:36:26 | mriedem | but that's not going to happen in the backport | |
| 17:37:08 | mriedem | dansmith: do you think doing a notfound / lookup / re-query in the instance.obj_load_attr is worth looking into so we can avoid explicit context switching like this? | |
| 17:37:18 | dansmith | no | |
| 17:37:39 | dansmith | the instance object should never know about instancemapping | |
| 17:37:41 | mriedem | too magical and hard to debug if it's wrong? | |
| 17:37:42 | mriedem | ok | |
| 17:37:56 | dansmith | they're in different databases, and that thing will fail if we hit that notfound if we're on a compute for example | |
| 17:38:42 | mriedem | yeah | |
| 17:39:10 | sdague | ok, well this feels kind of fragile, but if that's what we got, so be it | |
| 17:40:02 | mriedem | i don't disagree that this is gorpy, the instance delete path has always been bad and this isn't helping | |
| 17:40:19 | dansmith | the instance delete path is like some of the worst code we have | |
| 17:40:26 | dansmith | right behind BDMs probably :) | |
| 17:40:34 | mriedem | what about quotas? | |
| 17:40:36 | mriedem | this is quotas + delete | |
| 17:40:38 | mriedem | worst ever | |
| 17:40:48 | openstackgerrit | Juan Antonio Osorio Robles proposed openstack/nova master: Make versioned notifications topics configurable https://review.openstack.org/444947 | |
| 17:40:48 | melwitt | local delete | |
| 17:40:54 | mriedem | i do think we have some stuff we can cleanup in the delete flow that was meant for the transition to cells v2 | |
| 17:41:13 | mriedem | i started trying that cleanup here https://review.openstack.org/#/c/443405/ | |
| 17:41:25 | mriedem | but apparently part of it is still needed | |
| 17:41:28 | melwitt | that reminds me, I still have a patch up for one of the local delete problems https://review.openstack.org/340614 | |
| 17:41:42 | mriedem | melwitt: the 10th revert of the 10th revert? | |
| 17:42:05 | melwitt | yeah | |
| 17:43:06 | mriedem | dansmith: if you can hit the 2 remaining ocata patches https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:stable/ocata then i can prep the release | |
| 17:43:16 | mriedem | i have a date with a 4th notary | |
| 17:44:33 | mriedem | jaypipes: that ironic inventory patch is on my list for the afternoon once i get rejected by this other notary | |
| 17:45:11 | johnthetubaguy | bauzas: I think there are quite a few unresolved comments from previous patch sets on this one: https://review.openstack.org/#/c/440580 | |
| 17:45:23 | johnthetubaguy | bauzas: am I missing context on another discussion that resolved them? | |
| 17:53:12 | jaypipes | mriedem: cool, thx | |
| 17:56:04 | melwitt | dansmith: on the host_manager thing, I don't think you need to change anything. I'm just missing how the double iterating would have to happen if it were changed | |
| 17:56:18 | dansmith | I just explained | |