Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-09-03
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
22:15:19 aspiers start_compute() is called by each test
22:15:37 aspiers and creates a fresh compute service
22:16:18 sean-k-mooney when i ran it under the debugger i found you were not geting to the updated domain caps because of https://github.com/openstack/nova/blob/master/nova/virt/libvirt/host.py#L759-L760
22:16:37 aspiers right
22:17:12 aspiers er
22:17:48 sean-k-mooney i dont really know why it passed before but self.compute.driver._host._domain_caps = None seams to fix it
22:18:09 aspiers actually I think it's https://github.com/openstack/nova/blob/master/nova/virt/libvirt/host.py#L1191
22:18:23 aspiers if the kernel param file is missing, it won't cache any caps
22:18:45 aspiers so when it appears later, it will call the fake getDomCaps API for the first time
22:18:47 sean-k-mooney no i steped throug it in the debugger
22:18:53 aspiers that's in the off_on case
22:19:26 sean-k-mooney it might be becasue of my traits code actully
22:19:32 aspiers in the on_off case, it wouldn't even care what caps were cached when it gets to off
22:19:53 aspiers but technically I think it's good to reset both caches in both tests
22:20:09 aspiers in case the order of tests ever got changed in the future
22:20:23 sean-k-mooney im calling this https://review.opendev.org/#/c/666915/16/nova/virt/libvirt/driver.py@6832
22:20:38 aspiers sorry, I mean the order of these https://github.com/openstack/nova/blob/master/nova/virt/libvirt/host.py#L1188-L1193
22:21:06 sean-k-mooney sure ill add that
22:21:28 sean-k-mooney it make it symetic and really we dont want the order of those check to break this

Earlier   Later