| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-08 | |||
| 15:26:06 | mriedem | bauzas: are you back to help review stuff this week? | |
| 15:30:45 | lyarwood | mdbooth: https://review.openstack.org/#/c/531233/ - FYI the bugfix from before the break | |
| 15:31:19 | mdbooth | lyarwood: Yes | |
| 15:32:00 | mdbooth | lyarwood: IIRC I preferred to attach/detach encryptors in attach/detach volume? | |
| 15:32:15 | mdbooth | Because those 2 things should always happen together | |
| 15:33:45 | lyarwood | mdbooth: yeah I think the issue with that was wiring the request context into yet more places | |
| 15:34:10 | mdbooth | lyarwood: Well lets wire away, because the alternative is a trickle of bugs | |
| 15:34:16 | mdbooth | It's probably not that many | |
| 15:35:16 | mdbooth | Hmm, I thought I had some notes on this. | |
| 15:35:34 | mdbooth | lyarwood: I literally just finished what I was doing earlier. Let me grab a coffee and look hard at this again. | |
| 15:35:49 | lyarwood | mdbooth: kk, the refactor is the patch below this btw | |
| 15:37:08 | mriedem | dansmith: want to hit this backport again? https://review.openstack.org/#/c/530982/ | |
| 15:37:28 | dansmith | you know I do | |
| 15:37:54 | hrw | mriedem: hello | |
| 15:38:04 | hrw | mriedem: https://review.openstack.org/#/c/530965/ got +1 from Zuul ;) | |
| 15:39:11 | mriedem | +2 again | |
| 15:41:30 | hrw | thanks mriedem | |
| 15:41:39 | hrw | stephenfin: your turn then ;D | |
| 15:43:37 | stephenfin | hrw: and +W here | |
| 15:43:59 | stephenfin | Cheers for the quick follow-ups on that, hrw | |
| 15:44:02 | hrw | stephenfin: ;) | |
| 15:44:47 | hrw | stephenfin: like I said yesterday - it help keeping reviewers attention ;D | |
| 15:47:28 | openstackgerrit | Merged openstack/nova master: Optionalize instance_uuid in console_auth_token_get_valid() https://review.openstack.org/481700 | |
| 15:47:36 | openstackgerrit | Merged openstack/nova master: Add ConsoleAuthToken object https://review.openstack.org/320063 | |
| 15:55:40 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add regression test for resize failing during retries https://review.openstack.org/531405 | |
| 15:55:40 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fix race condition in retrying migrations https://review.openstack.org/531022 | |
| 15:55:41 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Change compute RPC to use alternates for resize https://review.openstack.org/526436 | |
| 15:55:42 | mriedem | edleafe: addressed my nits in the regression test patch, and fixed my -1 in the regression bug fix patch in the middle, rebased the series to master also ^ | |
| 15:56:38 | matrohon | mriedem: hi | |
| 15:56:47 | openstackgerrit | Stephen Finucane proposed openstack/nova master: fixup! console: introduce the VeNCrypt RFB authentication scheme https://review.openstack.org/531833 | |
| 15:56:47 | openstackgerrit | Stephen Finucane proposed openstack/nova master: console: Send bytes to sockets https://review.openstack.org/531834 | |
| 15:57:14 | stephenfin | Oops | |
| 15:58:04 | mdbooth | Hehe | |
| 15:58:25 | openstackgerrit | Stephen Finucane proposed openstack/nova master: console: introduce framework for RFB authentication https://review.openstack.org/345397 | |
| 15:58:26 | openstackgerrit | Stephen Finucane proposed openstack/nova master: console: introduce the VeNCrypt RFB authentication scheme https://review.openstack.org/345398 | |
| 15:58:26 | openstackgerrit | Stephen Finucane proposed openstack/nova master: console: Provide an RFB security proxy implementation https://review.openstack.org/345399 | |
| 15:58:27 | openstackgerrit | Stephen Finucane proposed openstack/nova master: doc: Document TLS security setup for noVNC proxy https://review.openstack.org/500544 | |
| 15:58:27 | openstackgerrit | Stephen Finucane proposed openstack/nova master: console: Send bytes to sockets https://review.openstack.org/531834 | |
| 15:58:31 | mriedem | matrohon: hi | |
| 15:58:51 | mriedem | dansmith: i'm +2 on edleafe's fix for the cold migration + reschedule regression (and the test patch below it) https://review.openstack.org/#/c/531022/ | |
| 15:59:42 | matrohon | mriedem: I was trying to boot a VM without an IP, but I can't find a way to do so | |
| 16:00:01 | mriedem | matrohon: use at least microversion 2.37 to create the server and pass networks='none' | |
| 16:00:32 | mriedem | see the 'networks' parameter here https://developer.openstack.org/api-ref/compute/#create-server | |
| 16:01:05 | matrohon | mriedem: great! I reported a related bug, but I'll try it the way you mention | |
| 16:01:41 | matrohon | mriedem: However, I'm not sur my bug is pointless : https://bugs.launchpad.net/nova/+bug/1741575 | |
| 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 | cdent | efried: i'm back, for a while | |
| 16:12:02 | mdbooth | What about pre_live_migration()? | |
| 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 | |