| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-03 | |||
| 21:30:51 | dansmith | heh | |
| 21:37:00 | aspiers | sean-k-mooney: the comments which explicitly say it's simulating agent restart are correct | |
| 21:37:15 | aspiers | https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/libvirt/test_report_cpu_traits.py#L105 | |
| 21:37:22 | aspiers | https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/libvirt/test_report_cpu_traits.py#L163 | |
| 21:38:14 | aspiers | efried: you around? need some quick advice on https://review.opendev.org/#/c/644565 | |
| 21:40:15 | efried | aspiers: ... | |
| 21:40:22 | aspiers | I think https://review.opendev.org/#/c/644565/49/nova/virt/libvirt/driver.py@5089 needs to be added to utils.py instead | |
| 21:40:46 | aspiers | since it turns out that blockinfo.py also needs to call it | |
| 21:41:14 | aspiers | or more precisely, blockinfo.py needs to know the same machine type, and in both cases SEV needs to be taken into account | |
| 21:41:37 | efried | aspiers: You want my advice for this why? | |
| 21:42:08 | aspiers | Well, just a sanity check that sounds reasonable, because that would mean passing more info into blockinfo.py | |
| 21:42:19 | aspiers | Adding SEV checks to the machine type calculation requires extra info | |
| 21:42:28 | aspiers | e.g. the Host object to check whether SEV is supported | |
| 21:42:41 | aspiers | or the sev_enabled variable if already calculated by the caller | |
| 21:43:07 | efried | At a glance, I agree it looks weird for a method with this generic a name to need anything sev-specific. | |
| 21:43:38 | efried | but I've repeatedly punted on this machine type stuff for a reason. | |
| 21:43:56 | aspiers | The key case here is when there is no machine type specified in the image | |
| 21:44:08 | aspiers | Maybe I can hassle kashyap about it | |
| 21:44:26 | efried | you need to default the machine type differently depending on whether SEV is enabled?? | |
| 21:44:30 | aspiers | Yes | |
| 21:44:38 | efried | that... doesn't sound right | |
| 21:44:40 | aspiers | Normally 'pc' for x86_64, but 'q35' if SEV is enabled | |
| 21:44:53 | aspiers | That's in the spec | |
| 21:45:21 | aspiers | Ahh, actually the spec is out of date on this point | |
| 21:45:47 | efried | What sounds more likely to me is: | |
| 21:45:47 | efried | If the machine type is not specified, we default it to X. | |
| 21:45:47 | efried | X doesn't work for SEV, so if you do that, SEV will be disabled. | |
| 21:45:47 | efried | SEV requires machine type Y, so if you want to SEV, you have to specify machine type Y as well as {other stuff to get SEV} | |
| 21:45:48 | aspiers | No, the spec is technically correct | |
| 21:46:34 | aspiers | AFAICS your proposal would have a big (negative) impact on usability of SEV | |
| 21:46:37 | efried | put another way, the "is SEV enabled" check gets an additional condition for machine type. And the above is how that's reflected in the documentation. | |
| 21:46:55 | aspiers | It would make it impossible to turn SEV on or off just by choosing a different flavor | |
| 21:47:09 | aspiers | You'd have to also choose an SEV-specific image to go alongside it | |
| 21:47:18 | efried | um | |
| 21:47:30 | efried | you don't have to do that anyway? | |
| 21:47:36 | aspiers | No | |
| 21:47:47 | aspiers | You can have one image which can be used SEV or non-SEV | |
| 21:47:54 | efried | can you not specify machine type via flavor? | |
| 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 ;-) | |