| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-03 | |||
| 21:48:32 | aspiers | I don't think so | |
| 21:49:10 | aspiers | https://opendev.org/openstack/nova/src/branch/master/nova/virt/libvirt/utils.py#L543 | |
| 21:49:11 | efried | sorry, but if I'm understanding this correctly, I'm afraid that has to be the right call. | |
| 21:49:26 | efried | what are the reasons you would specify a machine type in your image? | |
| 21:49:37 | efried | possibly more importantly, what are the reasons you wouldn't? | |
| 21:49:53 | aspiers | Well, having the choice of SEV is one reason not to | |
| 21:50:09 | aspiers | Well I suppose it could be set to q35 and work non-SEV | |
| 21:50:32 | efried | and crucially, are we breaking anyone by setting a default different than the default they're expecting? | |
| 21:50:55 | aspiers | There are different levels of default | |
| 21:51:09 | aspiers | There's arch-specific defaults hardcoded into nova | |
| 21:51:16 | aspiers | There's the default per arch in nova.conf | |
| 21:51:56 | aspiers | There's even a QEMU default, which is changing to q35 at some point | |
| 21:52:04 | aspiers | (hence https://bugs.launchpad.net/nova/+bug/1780138) | |
| 21:52:05 | openstack | Launchpad bug 1780138 in OpenStack Compute (nova) "Don't assume the guest machine type to be of 'pc'" [Medium,Confirmed] - Assigned to Kashyap Chamarthy (kashyapc) | |
| 21:52:38 | efried | forgive me for having swapped all of this out, but do we have some flavor/image cross-validation code for this in place? | |
| 21:53:10 | aspiers | That's what I was in the process of adding | |
| 21:53:11 | efried | So if their image explicitly says machine type X and their flavor asks for SEV, we can punt? | |
| 21:53:22 | aspiers | Yes, that's what I agreed this morning with kashyap and sean-k-mooney | |
| 21:53:29 | efried | okay | |
| 21:53:33 | aspiers | I'm adding an InvalidMachineType exception | |
| 21:53:53 | aspiers | as per https://review.opendev.org/#/c/644565/49/nova/virt/libvirt/driver.py@5105 | |
| 21:54:24 | efried | and this is one where we're doing a request filtery thing (except no longer in request filter - now it's a RequestSpec loader thing?) to translate/add placement-isms | |
| 21:54:42 | aspiers | No, that's a different set of checks | |
| 21:54:58 | efried | yeah, I know, but I mean that's another part of this feature | |
| 21:55:19 | aspiers | It's part of the same series, yeah | |
| 21:55:49 | efried | and how many machine types will sev work with? | |
| 21:55:54 | aspiers | just q35 | |
| 21:55:56 | efried | beaut | |
| 21:56:06 | efried | so why don't we do this: | |
| 21:56:06 | efried | In the request spec loader thingy, if image meta doesn't specify a machine type, shove one in there. | |
| 21:56:11 | aspiers | I suppose SEV-capable images already need hw_firmware_type=uefi, so adding a requirement for another property isn't terrible | |
| 21:56:26 | aspiers | Yeah could do | |
| 21:56:38 | aspiers | See https://review.opendev.org/#/c/666616/41/doc/source/admin/configuration/hypervisor-kvm.rst@585 | |
| 21:56:39 | efried | that doesn't affect the image at large, does it? Just our local copy of it? | |
| 21:56:57 | aspiers | I'd add documentation of the extra constraint to that paragraph | |
| 21:57:03 | aspiers | Not sure I understand that question. At large? | |
| 21:57:36 | efried | I mean if you change RequestSpec.image.properties, it only affects this one request, not the real image. | |
| 21:57:48 | aspiers | It's somewhat unfortunate because it's not q35 per se which SEV requires, but rather virtio 1.0. But 'pc' only offers virtio 0.9, hence the need for q35 | |
| 21:59:10 | aspiers | Oh wait, you're suggesting that hw_machine_type=q35 should be added *on the fly*, not to the image via glance API? | |
| 21:59:44 | aspiers | I think that would be really bad, because it would override any hw_machine_type set on the real image, or any default in nova.conf | |
| 22:00:03 | aspiers | That overriding is what kashyap and sean-k-mooney were objecting to this morning, and I'm inclined to agree with them | |
| 22:00:28 | efried | okay, I'm happy to accept that that's a bad idea. | |
| 22:00:35 | efried | so back to this other thing | |
| 22:00:41 | aspiers | although this all feels a bit bikesheddy, because I have a hard time imagining that anyone would want to insist on 'pc', given that 'q35' has already been around for 11 years | |
| 22:00:46 | efried | you pointed out that there's already a requirement for the uefi thing in the image. | |
| 22:00:50 | aspiers | Yep | |
| 22:01:04 | aspiers | So we could just document an extra requirement for hw_machine_type=q35 | |
| 22:01:06 | efried | is that a property they have to set (to some value) regardless? | |
| 22:01:16 | efried | or does it also default if you don't set it? | |
| 22:01:25 | aspiers | It defaults via the code in libvirt/utils.py | |
| 22:01:37 | aspiers | which looks in nova.conf first, and then falls back to the hardcoded mapping | |
| 22:01:51 | aspiers | https://opendev.org/openstack/nova/src/branch/master/nova/virt/libvirt/utils.py#L537 | |
| 22:02:14 | aspiers | CONF.libvirt.hw_machine_type I mean | |
| 22:02:21 | efried | o-queso, I don't see the problem with requiring them to set two image props instead of one. | |
| 22:02:30 | aspiers | Yeah, that sounds reasonable to me too | |
| 22:02:44 | efried | whee | |
| 22:02:45 | aspiers | It's kind of in the same category as the UEFI requirement anyway | |
| 22:02:51 | efried | cool | |
| 22:02:55 | aspiers | "virtual hardware requirements" | |
| 22:03:23 | aspiers | OK that was very helpful, thanks a lot! | |
| 22:04:09 | efried | neat | |
| 22:04:10 | aspiers | BTW your "I don't know enough about libvirt to be helpful" stance just lost all credibility ;-) | |
| 22:04:27 | aspiers | not that I ever believed it anyway X-D | |
| 22:04:37 | efried | hah, read the transcript again. It should be even more clear that I know jack about libvirt. | |
| 22:04:40 | sean-k-mooney | efried: you can set the default machine_type in the nova.conf too | |
| 22:05:00 | aspiers | sean-k-mooney: that's what I just said ;-) | |
| 22:05:02 | sean-k-mooney | efried:so you dont have to set it in the image always | |
| 22:05:03 | efried | yeah, that ^ sounds like it would be a good option for deployers wishing to enable SEV on their hosts. | |
| 22:05:16 | efried | can you do the uefi thing in nova.conf too? | |
| 22:05:25 | sean-k-mooney | no | |
| 22:05:28 | efried | bummer | |
| 22:05:35 | sean-k-mooney | at least not that im aware of | |
| 22:05:41 | aspiers | that might be a nice feature to add | |
| 22:06:05 | sean-k-mooney | eventrually we do want to change the defualt machie type to Q35 by the way | |
| 22:06:08 | aspiers | that would actually be really easy | |
| 22:06:24 | aspiers | sean-k-mooney: yeah I mentioned that too | |
| 22:06:26 | sean-k-mooney | adding the bios/uefi | |
| 22:06:33 | sean-k-mooney | ya it should not be hard | |
| 22:06:54 | aspiers | sean-k-mooney: https://bugs.launchpad.net/nova/+bug/1780138 | |
| 22:06:55 | openstack | Launchpad bug 1780138 in OpenStack Compute (nova) "Don't assume the guest machine type to be of 'pc'" [Medium,Confirmed] - Assigned to Kashyap Chamarthy (kashyapc) | |
| 22:07:12 | sean-k-mooney | although you do need to have the firmware ectra but i can see people wanting to eventurally mvoe the Q35+uefi only | |
| 22:08:31 | sean-k-mooney | ya we shoudl avoid assumign pc but its also wrong to assume q35 currently | |
| 22:09:22 | sean-k-mooney | we have a function that returns the correct machine type taking into account the default + config + image | |
| 22:09:43 | sean-k-mooney | if we dont know what to use we should call that rather then guessing | |
| 22:10:48 | sean-k-mooney | aspiers: by the way https://review.opendev.org/#/c/666915/16/nova/tests/functional/libvirt/test_report_cpu_traits.py were the changes i need to make the sev code work with my series | |
| 22:11:37 | aspiers | sean-k-mooney: resetting the traits cache makes sense | |
| 22:11:46 | aspiers | sean-k-mooney: two questions about resetting domain caps cache | |
| 22:11:56 | aspiers | 1. how did my tests pass without that before? | |
| 22:12:01 | sean-k-mooney | technically i think you should have always been doing self.compute.driver._host._domain_caps = None in your orginal code | |
| 22:12:11 | sean-k-mooney | aspiers: i think it should not or it was a race | |
| 22:12:14 | aspiers | 2. why not do it on both tests? | |
| 22:12:44 | aspiers | AFAICS it can't be a race because we're only talking about a single thread here | |
| 22:12:53 | sean-k-mooney | the second test does not modify the capablities but it would be valid to do it for both | |
| 22:12:57 | openstackgerrit | Merged openstack/nova master: libvirt: Fold in argument to '_update_provider_tree_for_vgpu' https://review.opendev.org/676729 | |
| 22:13:25 | sean-k-mooney | aspiers: i ment between fucntional test and the order they execute in | |
| 22:13:34 | aspiers | oh ok the second test doesn't look up caps because it gives up as soon as it sees the kernel parameter file missing | |
| 22:13:42 | sean-k-mooney | yep | |
| 22:14:07 | aspiers | but the compute host isn't shared between the tests surely? | |
| 22:14:26 | sean-k-mooney | it depens on how your mocking. | |
| 22:14:39 | sean-k-mooney | it should not be but i have seen wierd issue | |