| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-06 | |||
| 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 :) | |
| 14:43:32 | kashyap | HOWEVER | |
| 14:43:41 | stephenfin | I imagine no one would ever see this because what sane operator would run fully-emulated guests | |
| 14:43:55 | kashyap | stephenfin: Who in their right mind would do that for any production workload? | |
| 14:44:01 | stephenfin | kashyap: Jinx | |
| 14:44:08 | kashyap | _Exactly_, that was my "HOWEVER" | |
| 14:44:48 | kashyap | So, we can't have 100% guards for people willingly sticking knives in thier necks. | |
| 14:44:58 | kashyap | If that's an analogy at all :D | |
| 14:45:21 | mriedem | stephenfin: i know of an operator in the ML asking for that, to run powervm guests on an x86 host | |
| 14:45:30 | kashyap | lyarwood: Sorry, your good deed is getting punished, is it? | |
| 14:45:37 | stephenfin | Ultimately though, this seems to suggest we can remove the 'caps' argument to '_get_machine_type' and retrieve the *guest* architecture via the 'libvirt_utils.get_arch' function instead | |
| 14:45:45 | stephenfin | and lyarwood gets to fix two bugs in one | |
| 14:45:51 | stephenfin | for the win | |
| 14:45:56 | sean-k-mooney | stephenfin: operator that want to support cross arch developemnt | |
| 14:45:58 | mriedem | i'm pretty sure danpb was also ok (in the ML) with the fully emulated thing | |
| 14:45:59 | kashyap | But in two separate changes, obviously. | |
| 14:46:10 | kashyap | sean-k-mooney: Yeah, for development, yes... | |
| 14:46:22 | stephenfin | mriedem: In production?? | |
| 14:46:36 | sean-k-mooney | yes so its also a valid usecase for build farms | |
| 14:46:37 | mriedem | let me get the thread | |
| 14:46:49 | stephenfin | If so, can they share some of the cash they're burning with me? | |
| 14:47:21 | sean-k-mooney | e.g. if you are a software company uing the cloud to bulid your product for mulitple target archatecure full emulation it totally vailid | |
| 14:47:23 | mriedem | this guy is funded by the US military so they have infinite funds | |
| 14:47:29 | kashyap | sean-k-mooney: I contend that anyone who *REALLY* cares about multiple archs, they will get a devel box for that arch. | |
| 14:47:35 | stephenfin | Called it :) | |
| 14:47:57 | sean-k-mooney | kashyap: that is not what i think the majority of people do | |
| 14:48:11 | mriedem | http://lists.openstack.org/pipermail/openstack-operators/2018-August/thread.html#15617 | |
| 14:48:14 | mriedem | ^ is the thread | |
| 14:48:20 | sean-k-mooney | most use qemu to develop and test locally and only get real hardware if they ware writing low level software | |
| 14:49:40 | mriedem | danpb's reply is in this one http://lists.openstack.org/pipermail/openstack-operators/2018-August/015653.html | |
| 14:49:47 | kashyap | Yeah, I'm not denying there's no devel/test use case | |
| 14:49:48 | mriedem | "> Yes, it should do exactly that IMHO !" | |