| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-08 | |||
| 15:19:37 | mriedem | s/finish/even spend time on/ | |
| 15:19:39 | efried | cdent Buzz me when you get back on, want to continue discussion. | |
| 15:19:40 | cdent | mriedem: can you say that while simultaneously affirming that we merge more code than any other openstack project :) | |
| 15:20:10 | cdent | efried: will do | |
| 15:20:35 | mriedem | i don't know how much code we merge relative to other projects | |
| 15:21:17 | efried | Perpetual challenge to strike the right balance. I don't think it's a systemic problem in either direction; just needs to be managed on a case-by-case. | |
| 15:23:37 | mriedem | i'm just commenting from the sidelines as i haven't been involved in coding or reviewing the NRP series, | |
| 15:24:04 | mriedem | i'm just concerned that we're spending a lot of time designing the end thing right now and we'll miss the boat on getting anything functional in queens | |
| 15:24:49 | efried | mriedem Well, we've already landed a *lot* of functional stuff in queens. And I think we're on track to get the rest done. (That was specifically brought up and agreed in the sched meeting.) | |
| 15:25:04 | mriedem | i'm going to try and wrap up the series of changes i've been pushing/reviewing for the last few weeks to get done this week b/c i'm out next week | |
| 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 | |