| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-14 | |||
| 16:58:05 | openstackgerrit | John Garbutt proposed openstack/nova-specs master: Add locked-by-service spec https://review.openstack.org/438134 | |
| 17:03:01 | johnthetubaguy | siva_krishnan: thinking about this spec: https://review.openstack.org/#/c/347161 are we not now back to basically what the original spec said now? | |
| 17:04:08 | mdbooth | johnthetubaguy: What's a service locked instance? | |
| 17:04:39 | johnthetubaguy | mdbooth: its writing up the old story we had around trove "locking" and instance from a user deleting it | |
| 17:05:14 | siva_krishnan | johnthetubaguy: yeah only with one change. instead of generating unrescue.xml from database we are copying it to destination from source | |
| 17:05:39 | johnthetubaguy | siva_krishnan: ah, OK, could we add a not in the git commit saying thats the only change? | |
| 17:05:50 | johnthetubaguy | s/not/note/ | |
| 17:06:04 | openstackgerrit | Sujitha proposed openstack/nova master: Add description to policies in aggregates.py https://review.openstack.org/445230 | |
| 17:06:09 | johnthetubaguy | mdbooth: yeah, I hadn't noticed that overloaded term, damm | |
| 17:06:20 | mdbooth | What's a service, then? | |
| 17:06:51 | johnthetubaguy | mdbooth: in that context, is the service token in the context | |
| 17:07:19 | johnthetubaguy | mdbooth: so trove has a service token, it should be in the spec, although something tells me its probably not clear in there | |
| 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 | melwitt | local delete | |
| 17:40:48 | openstackgerrit | Juan Antonio Osorio Robles proposed openstack/nova master: Make versioned notifications topics configurable https://review.openstack.org/444947 | |
| 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/ | |