| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-08 | |||
| 16:01:42 | openstack | Launchpad bug 1741575 in OpenStack Compute (nova) "creating a VM without IP (ip_allocation=None)" [Undecided,New] | |
| 16:02:05 | mriedem | matrohon: yeah https://bugs.launchpad.net/nova/+bug/1741575 is something else | |
| 16:02:52 | mdbooth | lyarwood: Ah, yes | |
| 16:03:18 | mdbooth | lyarwood: So, we call _connect_volume in _get_guest_xml, which is pretty unambigously a bug | |
| 16:03:32 | mdbooth | But it's a bug we rely on in a couple of places | |
| 16:03:54 | mdbooth | I have some notes I made before the break about how to unwind that | |
| 16:05:06 | mdbooth | Apart from that, we'd need context in swap_volume | |
| 16:05:07 | matrohon | mriedem: I briefly discussed with carl_baldwin a long time ago, how told me he didn't finished the job on the nova side. At least, he didn't submit anything related to the case where "ip_allocation=none". | |
| 16:05:22 | lyarwood | mdbooth: for the bugfix I'm now providing that | |
| 16:05:23 | mdbooth | lyarwood: Which... we do anyway. Doesn't look like we're handling encryptors there. | |
| 16:05:47 | mdbooth | _create_domain_setup_lxc() | |
| 16:06:45 | mdbooth | lyarwood: pre_live_migration() ? | |
| 16:07:01 | mdbooth | I don't immediately see where we're attaching encryptors there | |
| 16:07:37 | mdbooth | So my vote would be: | |
| 16:08:00 | mdbooth | 1. unpick _connect_volume() in _get_guest_xml() | |
| 16:08:11 | mdbooth | 2. pass context to swap_volume() | |
| 16:08:31 | mdbooth | 3. Implement ajttach/detach encryptors in LibvirtDriver._connect/disconnect_volume | |
| 16:09:03 | lyarwood | urgh, 1 would make this a risk to backport | |
| 16:09:04 | mdbooth | Then we can be reasonably confident that we caught all the cases, and nobody will introduce any more in the future. | |
| 16:09:25 | mdbooth | lyarwood: I don't think it's as bad as you think | |
| 16:09:26 | lyarwood | also another reason not to do this for the bugfix would be the change in error handling | |
| 16:09:56 | lyarwood | we often don't connect/disconnect in the same try block as attaching/detaching encryptors | |
| 16:10:17 | mdbooth | Well we should... | |
| 16:10:28 | lyarwood | yeah agreed, but that's not part of this bugfix | |
| 16:10:32 | mdbooth | That's only going to simplify error handling, no? | |
| 16:10:37 | dansmith | mriedem: okay I'm going through bauzas' gpu set and then will circle back to that | |
| 16:11:34 | lyarwood | mdbooth: yeah but the _get_guest_xml() cleanup and merging attach/detach encryptors into connect/disconnect volume just seems way over the top for a simple, easily backportable fix for swap_volume tbh | |
| 16:11:38 | mdbooth | lyarwood: I don't think a 'proper' fix is all that messy in this case. My concern is that if we apply 4 more bits of band-aid we won't ever fix it. | |
| 16:12:02 | mdbooth | What about pre_live_migration()? | |
| 16:12:02 | cdent | efried: i'm back, for a while | |
| 16:12:06 | lyarwood | mdbooth: it's hardly band-aid | |
| 16:12:08 | mdbooth | Are we handling that somewhere else? | |
| 16:12:21 | efried | cdent I'm writing up a summary and adding it to the review (https://review.openstack.org/#/c/526539/) | |
| 16:12:35 | cdent | efried++ | |
| 16:15:12 | lyarwood | mdbooth: https://bugs.launchpad.net/nova/+bug/1633033 - sorry that took a little digging | |
| 16:15:15 | openstack | Launchpad bug 1633033 in OpenStack Compute (nova) "live migration with encrypted volume fails" [Undecided,In progress] - Assigned to Lee Yarwood (lyarwood) | |
| 16:16:02 | mdbooth | lyarwood: Ok, so looks like that's broken too, same root cause | |
| 16:16:07 | lyarwood | yup | |
| 16:16:21 | mriedem | lyarwood: mdbooth: why don't you do the short-term backportable fix and then do the refactor on master which isn't backported? | |
| 16:16:32 | mriedem | don't refactor a bunch of code that you're going to backport | |
| 16:16:33 | mdbooth | Looking back over my notes, actually I think the _get_guest_xml fix is really easy | |
| 16:17:14 | lyarwood | mriedem: I'm trying to suggest that, I really don't want to play around with _get_guest_xml in backports | |
| 16:17:42 | mdbooth | lyarwood: The reason is that everywhere that calls _get_guest_xml either subsequently calls _create_domain_and_network, or doesn't pass block_device_info, so doesn't call _connect_volume | |
| 16:18:08 | mdbooth | Which means that all you need to do is move _connect_volume from _get_guest_xml to _create_domain_and_network | |
| 16:18:15 | mdbooth | Which is also totally intuitive | |
| 16:18:28 | mdbooth | I've audited ^^^, but feel free to do the same | |
| 16:18:51 | mdbooth | _create_domain_and_network already has context, which also makes sense | |
| 16:19:03 | mdbooth | So that change isn't going to be invasive at all | |
| 16:19:04 | matrohon | mriedem: I'm not sure networks=none would help me. My VM needs a port, but a port without IP. | |
| 16:19:29 | mriedem | matrohon: yeah it won't | |
| 16:19:34 | mriedem | networks=none means no ports | |
| 16:19:48 | mriedem | i didn't know about ip_allocation='none' | |
| 16:20:01 | mdbooth | lyarwood: Whatever way you look at it, we need context in swap_volume anyway | |
| 16:20:09 | mdbooth | So that part needs to be done anyway | |
| 16:20:14 | lyarwood | mdbooth: and is in the bugfix | |
| 16:20:31 | mdbooth | lyarwood: I don't see it in that review stack | |
| 16:20:47 | lyarwood | mdbooth: https://review.openstack.org/#/c/531233/3/nova/virt/libvirt/driver.py | |
| 16:20:50 | mdbooth | NP, now I do | |
| 16:21:03 | matrohon | mriedem: ok, I can work on fixing 1741575, with the attached patch as a first step | |
| 16:21:11 | mdbooth | lyarwood: Ok, that bit's already done. | |
| 16:21:37 | mdbooth | With those bits in place, moving the encryptor call into _connect/_disconnect is trivial | |
| 16:21:46 | mdbooth | And no harder to backport | |
| 16:21:52 | mdbooth | And also fixes pre_live_migration | |
| 16:22:04 | mdbooth | And anything else we missed | |
| 16:22:33 | mdbooth | Does lxc have encryptors? | |
| 16:23:01 | matrohon | mriedem: do you think I need something else than the bug report to start submitting code review? | |
| 16:23:14 | lyarwood | mdbooth: well, the frontend encryptors are host level so yeah, I can't see why not. | |
| 16:23:23 | lyarwood | mdbooth: not that they are tested anywhere. | |
| 16:23:30 | mdbooth | lyarwood: Ok, that's probably a bug too, then | |
| 16:23:52 | mdbooth | So it's a 3 for 1 fix | |
| 16:24:02 | openstackgerrit | Chris Dent proposed openstack/nova master: Optional separate database for placement API https://review.openstack.org/362766 | |
| 16:24:22 | lyarwood | mdbooth: right, I'll respin without moving _connect_volume out of _get_guest_xml | |
| 16:24:35 | mdbooth | I don't think you can do that | |
| 16:24:54 | lyarwood | right, context | |
| 16:25:02 | mdbooth | Actually it's already there | |
| 16:25:19 | mdbooth | But you'd have to remove the encryptor stuff from _create_domain_and_network | |
| 16:25:28 | mdbooth | Which is just backwards | |
| 16:25:32 | mriedem | matrohon: no | |
| 16:25:35 | mdbooth | Might as well go the other way | |
| 16:25:37 | mriedem | bug fix is fine | |
| 16:25:50 | matrohon | mriedem: ack, thanks | |
| 16:25:58 | mriedem | matrohon: but i looked at the patch and it doesn't really make sense to me, unless a port with ip_allocation='none' results in port.get('ip_allocation') == None? | |
| 16:27:12 | efried | jaypipes cdent What is the behavior when I try to delete a provider that has "stuff" associated with it? | |
| 16:27:34 | efried | I think I've seen some cases where we automatically delete the "stuff" and some where we bounce. | |
| 16:27:44 | cdent | fails, if there are allocations we bounce | |
| 16:27:48 | matrohon | mriedem: port.get('ip_allocation') == None is the unexpected result, I'm currently trying to find the reason why. I probably need to fix that first on the neutron side | |
| 16:27:55 | cdent | I think that's the only one, but let confirm | |
| 16:28:28 | efried | cdent I'm talking about inventory, aggregates, traits - but most especially aggregates at the moment. | |
| 16:29:09 | jaypipes | efried: we just delete. | |
| 16:29:27 | jaypipes | efried: allocations are the only thing that prevents deletion (and child providers) | |
| 16:29:28 | mriedem | matrohon: if neutron needs changes to support this case, then things get sticky with regards to interoperability | |
| 16:29:39 | cdent | efried: https://github.com/openstack/nova/blob/master/nova/objects/resource_provider.py#L816 | |
| 16:29:44 | efried | jaypipes Okay. So this is tricky. | |
| 16:29:55 | mriedem | because if you upgrade nova to handle this none scenario but neutron doesn't support it, things fail, and vice-versa | |
| 16:30:22 | efried | cdent Thanks, it's all right there! | |
| 16:31:36 | edleafe | mriedem: just getting caught up (sitting in traffic is *so* productive!) | |
| 16:31:43 | edleafe | mriedem: your changes look fine | |
| 16:32:43 | edleafe | mriedem: some of the issues you asked about on the review were from the original test, which was supposed to test alternate host usage; hence the 4 computes and such | |
| 16:33:02 | matrohon | mriedem: but ip_allocation | |
| 16:33:04 | edleafe | I'm going to add that test to the resize patch | |