| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-06 | |||
| 13:25:52 | stephenfin | kashyap: Might want to talk to tonyb | |
| 13:26:40 | kashyap | stephenfin: Yep, noted | |
| 13:27:05 | kashyap | sean-k-mooney: Yeah, I was looking for 'hrw' | |
| 13:29:11 | kashyap | sean-k-mooney: What a beast this whole PCIe saga is | |
| 13:30:47 | openstackgerrit | Stephen Finucane proposed openstack/nova master: api: Remove 'Debug' middleware https://review.opendev.org/662506 | |
| 13:32:38 | sean-k-mooney | kashyap: well its in good hands. we could just do nothing and leave it to the operator to configur the config for there needs which is what nova originally opted to do | |
| 13:33:08 | sean-k-mooney | but if there is a sane default then that is also a good outcome too | |
| 13:33:16 | sean-k-mooney | brb | |
| 13:33:30 | kashyap | sean-k-mooney: Yeah, indeed. But was just exclaiming about the general subtlety involved here... | |
| 13:41:32 | lyarwood | mriedem / efried ; https://review.opendev.org/#/c/663011/ - morning, if you have time to day I'm looking for some non-RH core review on this libvirt specific bugfix. There's a change on top of this that's testing the q35 machine type in the gate. I'm looking into the extend volume failure at the moment. | |
| 13:41:53 | efried | lyarwood: looking | |
| 13:52:26 | efried | lyarwood: Are you looking to backport this? | |
| 13:53:38 | lyarwood | efried: only to stable/stein | |
| 13:53:57 | lyarwood | and the last I checked it was still clean | |
| 13:53:59 | lyarwood | just | |
| 13:54:02 | efried | orly? | |
| 13:54:21 | lyarwood | yeah I know right | |
| 13:54:23 | efried | I thought I'd seen a bunch of twiddling of test_driver that seems unavoidable considering how many places you hit | |
| 13:55:29 | openstackgerrit | Lee Yarwood proposed openstack/nova stable/stein: libvirt: Use SATA bus for cdrom devices when using Q35 machine type https://review.opendev.org/663677 | |
| 13:55:32 | lyarwood | ^ just to show I'm not making it up | |
| 13:57:13 | stephenfin | lyarwood: comments incoming on that, btw | |
| 13:57:16 | stephenfin | efried: too | |
| 13:57:21 | lyarwood | stephenfin: thanks | |
| 13:59:19 | stephenfin | lyarwood: done | |
| 13:59:54 | stephenfin | Apologies if you explored that idea already but I couldn't see any comments suggesting it | |
| 14:01:11 | openstackgerrit | Arnaud Morin proposed openstack/nova master: Refresh instance network info on deletion https://review.opendev.org/660761 | |
| 14:02:05 | efried | lyarwood: It's early, but I think you changed the logic of the extracted method. | |
| 14:07:37 | gibi | exit | |
| 14:08:05 | dansmith | no | |
| 14:09:33 | efried | you can check out any time you like, | |
| 14:09:33 | gibi | :) | |
| 14:09:34 | gibi | sorry | |
| 14:09:34 | efried | but you can never leeeeaaave | |
| 14:10:07 | dansmith | efried: kinda infringing on mriedem's turf there buddy | |
| 14:10:15 | gibi | stephenfin: which test case was too brittle to change in https://review.opendev.org/#/c/660774 ? | |
| 14:11:10 | stephenfin | gibi: | |
| 14:11:11 | stephenfin | * brault (~brault@lfbn-1-9197-156.w86-238.abo.wanadoo.fr) has joined #openstack-nova | |
| 14:11:11 | stephenfin | * ttsiouts has quit (Remote host closed the connection) | |
| 14:11:11 | stephenfin | <efried> lyarwood: It's early, but I think you changed the logic of the extracted method. | |
| 14:11:11 | stephenfin | * tbachman (~tbachman@128.107.241.188) has joined #openstack-nova | |
| 14:11:12 | stephenfin | * ttsiouts (~ttsiouts@2001:1458:204:1::101:9145) has joined #openstack-nova | |
| 14:11:18 | stephenfin | damn you HexChat | |
| 14:11:27 | stephenfin | gibi: https://review.opendev.org/#/c/660774/3/nova/tests/unit/compute/test_compute.py@12875 | |
| 14:11:49 | tbachman | stephenfin: accidental beep? | |
| 14:12:04 | lyarwood | efried: yeah you're right but I'm not sure the original logic was there tbh | |
| 14:12:04 | stephenfin | tbachman: Yup. Sorry for the noise | |
| 14:12:08 | tbachman | no worries! | |
| 14:12:18 | lyarwood | efried: that seems to suggest that the config would overwrite the image metadata | |
| 14:12:54 | lyarwood | efried: shouldn't it be (mach_type or libvirt_utils.get_default_machine_type(caps.host.cpu.arch))? | |
| 14:13:03 | efried | lyarwood: No, the image meta took first priority, then the config, then the caps (IIUC) | |
| 14:13:21 | lyarwood | ah that diff was all messed up on my screen | |
| 14:13:23 | efried | I don't know what it *should* be. I'm just parsing what it *was* vs what it is in your patch. | |
| 14:13:29 | lyarwood | ack thanks | |
| 14:15:16 | efried | ==> and the rest of the logic was in the `else`, so would be skipped and we go right to the return | |
| 14:15:16 | efried | mach_type = image_meta... | |
| 14:15:16 | efried | if image_meta... is not None: | |
| 14:16:21 | lyarwood | yup sorry the indentation was all messed up in gerrit so I missed that before | |
| 14:16:41 | efried | mach_type = get_default_machine_type... or mach_type <== i.e. get_default_machine_type gets priority here | |
| 14:16:41 | efried | and then the weirdness of: | |
| 14:16:41 | efried | mach_type = anotherthing | |
| 14:16:41 | efried | if caps.anotherthing | |
| 14:16:41 | efried | mach_type = onething | |
| 14:16:41 | efried | if caps.onething: | |
| 14:16:41 | efried | in the `else` we did: | |
| 14:17:05 | efried | This has struck me as pretty tough to follow every time I've looked at this method, so any refactor to make it more explicit would be welcomed :) | |
| 14:18:44 | gibi | stephenfin: OK. I don't think I should block your patch. We can revisit the removal of _resize() at a later point | |
| 14:19:19 | gibi | stephenfin: I will play a bit with that test case but I put a +2 on your patch | |
| 14:32:17 | lyarwood | stephenfin: okay, another way of doing this with less crazy passing of _host | |
| 14:32:43 | lyarwood | stephenfin: actually defining the two arch:machine_type configs we have in code here in nova.conf | |
| 14:33:17 | lyarwood | stephenfin: they are the only reason we need _host to fetch things anyway | |
| 14:33:39 | stephenfin | lyarwood: Left more comments there but probably easier discuss here | |
| 14:33:43 | stephenfin | Also, thanks gibi :) | |
| 14:33:54 | stephenfin | lyarwood: To which two configs do you refer? | |
| 14:35:10 | lyarwood | stephenfin: virt for fields.Architecture.ARMV7 & fields.Architecture.AARCH64 | |
| 14:35:31 | lyarwood | stephenfin: and s390-ccw-virtio for fields.Architecture.S390 & fields.Architecture.S390X | |
| 14:35:44 | stephenfin | Gotcha. Yeah, it's weird that they're there | |
| 14:36:28 | lyarwood | hmm we still pass caps.host.cpu.arch to get_default_machine_type so nvm | |
| 14:36:34 | stephenfin | At the very least, that should probably be done in the 'libvirt_utils.machine_type_mappings' function instead | |
| 14:36:40 | stephenfin | lyarwood: We pass an arch | |
| 14:36:58 | stephenfin | which we're getting from caps.host.cpu.arch but I don't think that's necessary | |
| 14:37:31 | stephenfin | lyarwood: Any reason we could pass the arch from 'libvirt_utils.get_arch' instead? | |
| 14:38:30 | stephenfin | That would probably be more correct since we surely want to retrieve the machine type for the _guest_ architecture | |
| 14:39:09 | kashyap | efried: Actually _very_ good observation on the priority of 'image_meta'... | |
| 14:39:12 | lyarwood | stephenfin:yeah that works | |
| 14:39:28 | lyarwood | stephenfin: totally missed that as an option | |
| 14:39:41 | efried | kashyap: Does it actually wind up mattering? I'll feel less nitpicky if it does. | |
| 14:39:50 | stephenfin | lyarwood: That's got to be a bug too, right? | |
| 14:40:13 | kashyap | efried: I'm not 100% sure; but from my reading, it doesn't. (Sorry for the weasel words.) | |
| 14:41:10 | lyarwood | stephenfin: hmm it's inefficient but I don't think it was a bug | |
| 14:41:12 | stephenfin | kashyap, lyarwood: So from https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L4327-L4355 we'll try to retrieve the machine type from image metadata | |
| 14:41:30 | stephenfin | If can't do that, we'll fall back to using something based on the host architecture | |
| 14:41:47 | kashyap | stephenfin: Correct | |
| 14:42:04 | stephenfin | But if the guest is e.g. x86 running on an ARMV7 host, we'll return a machine type of 'virt' | |
| 14:42:05 | kashyap | Am I wrong in insisting to do this extraction thingie in a separate change? | |
| 14:42:38 | kashyap | stephenfin: You mean, an emulated x86 guest running on an ARMv7 host? | |
| 14:42:41 | stephenfin | So the guest would have an x86 architecture but a ARM'y machine type | |
| 14:42:44 | stephenfin | kashyap: Yeah | |
| 14:42:52 | lyarwood | that would be abug | |
| 14:43:09 | kashyap | Yes, that's a bug. | |
| 14:43:14 | stephenfin | Sweet :) | |