| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-08 | |||
| 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 | |
| 16:33:09 | matrohon | mriedem: sorry | |
| 16:33:17 | efried | ggregate) so we'll... drop it from the local provider tree at that time, somehow. | |
| 16:33:17 | efried | cdent jaypipes So this might be okay. A virt that's "leaving" an aggregate should only delete the aggregate association from self, not delete it from the aggregate or delete the shared provider. The sharing provider will still show up in the ProviderTree, so report client won't delete it. Then the next time we pull the compute RP and its associations, that sharing provider will be absent (because no longer associated by a | |
| 16:33:22 | mriedem | edleafe: add the test to the resize patch? | |
| 16:33:31 | mriedem | edleafe: the test is already there since it comes before it in the series | |
| 16:33:51 | edleafe | mriedem: the regression test really doesn | |
| 16:34:06 | edleafe | doesn't check that the alternate hosts stuff is working as expected | |
| 16:34:16 | edleafe | just that it doesn't blow up :) | |
| 16:34:23 | cdent | efried: I think that sounds right, but as your patient, I'm going to the bath, will bbl | |
| 16:34:28 | mriedem | so you're going to modify the regresssion test to add to it in the resize patch | |
| 16:34:42 | efried | cdent Enjoy | |
| 16:35:29 | edleafe | mriedem: no, more or less copy parts to make a new functional test that's alternate host-specific | |
| 16:35:30 | ktibi | dansmith Hi do you know why consecutive_build_service_disable_threshold default value is 10 ? | |
| 16:35:36 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add pagination and Changes-since filter support for os-migrations. https://review.openstack.org/330406 | |
| 16:35:47 | ktibi | because I found a method for disable all compute service with a simple user :/ | |