| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-14 | |||
| 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 | |
| 17:56:36 | melwitt | from the context in that file, I just wasn't seeing it | |
| 17:56:40 | dansmith | and I've already changed it | |
| 17:57:09 | dansmith | there's not a double iteration if I put the conditional in the caller, I just thought the point was to avoid the conditional, | |
| 17:57:21 | dansmith | and if not, then I don't get the point of moving | |
| 17:57:37 | dansmith | but it's done anyway | |
| 17:58:57 | melwitt | hm, okay. I thought the point was to avoid the skip all but one in the specific cell case. but I'm probably missing something else | |
| 17:59:52 | johnthetubaguy | sfinucan: did you have some pbr fix for this: https://review.openstack.org/#/c/407364 | |
| 18:00:39 | openstack | Launchpad bug 1672624 in OpenStack Compute (nova) "Ceph volumes attached to local deleted instance could not be correctly handled" [Undecided,New] | |
| 18:00:39 | melwitt | mriedem: it looks like https://bugs.launchpad.net/nova/+bug/1672624 is another regression. I'm working on a local repro | |
| 18:03:39 | dansmith | melwitt: should we halt this and put it in the issues like the other? https://review.openstack.org/#/c/445575/ | |
| 18:05:29 | melwitt | dansmith: like the other "fix coming soon" one? argh, maybe. I'm not 100% of when this regressed and I haven't repro-d it yet. from the code, it doesn't look like it should be happening | |
| 18:05:43 | dansmith | yeah, that | |
| 18:06:45 | melwitt | I guess I hesitate about mentioning it in there when I haven't repro'd it yet | |
| 18:07:24 | melwitt | I just tried it on an older devstack ceph I have and the local delete worked fine. working on a try with master | |
| 18:20:16 | johnthetubaguy | cdent: jaypipes: I guess we don't have an api-ref site for placement yet? | |
| 18:20:37 | jaypipes | johnthetubaguy: not yet, no. one is needed for sure. | |
| 18:24:00 | johnthetubaguy | mriedem: would be nice to get this policy spec moving, folks seem generally happy with the approach now: https://review.openstack.org/#/c/433010/ | |
| 18:24:41 | dansmith | jaypipes: if I make that server_default=0 thing can I assume that just reading existing records will give me zero? | |
| 18:24:52 | dansmith | jaypipes: and, does that require writing the table again, or is it smart? | |
| 18:25:20 | jaypipes | dansmith: adding server_default="0" will allow you to make the field non-nullable from the start. | |
| 18:25:56 | dansmith | jaypipes: I assume that means "yes" to the first part | |
| 18:26:15 | jaypipes | dansmith: yes | |
| 18:26:17 | jaypipes | :) | |
| 18:26:31 | jaypipes | dansmith: and no on the second (no rewriting table) | |
| 18:26:40 | dansmith | okay | |
| 18:27:43 | openstackgerrit | Pushkar Umaranikar proposed openstack/nova-specs master: Stop using Neutron admin token https://review.openstack.org/439890 | |
| 18:36:31 | openstackgerrit | Anusha Unnam proposed openstack/nova master: Add keypairs field to InstancePayload https://review.openstack.org/419730 | |
| 18:45:25 | openstackgerrit | Maxim Nestratov proposed openstack/nova-specs master: libvirt: reintroduce runtime image type detection https://review.openstack.org/363547 | |