| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-12 | |||
| 17:26:26 | stephenfin | gibi: Thanks for the review. That should be good to go now ^ | |
| 17:29:37 | gmann | lyarwood: sure checking | |
| 17:33:07 | lyarwood | gmann: thanks | |
| 17:34:59 | gmann | lyarwood: +A | |
| 17:42:42 | openstackgerrit | Stephen Finucane proposed openstack/nova master: docs: Document UEFI secure boot feature https://review.opendev.org/c/openstack/nova/+/776684 | |
| 17:43:26 | stephenfin | melwitt: lyarwood: Collective nits addressed ^ | |
| 17:48:08 | lyarwood | stephenfin: ack LGTM | |
| 17:48:13 | stephenfin | thanks | |
| 17:50:12 | kashyap | stephenfin: How come emulated-tpm.rst got touched? | |
| 17:50:37 | kashyap | This bit https://review.opendev.org/c/openstack/nova/+/776684/9/doc/source/admin/emulated-tpm.rst#b41 | |
| 17:51:20 | stephenfin | kashyap: I copied the "show me the trait" snippet from that for the secure boot guide and both lyarwood and melwitt suggested changes to it, so it made sense to fix the original source also | |
| 17:51:41 | stephenfin | *suggested changes to the secure boot variant of the snippet | |
| 17:52:05 | kashyap | stephenfin: Ah, right; just now caught up w/ Mel's comments | |
| 17:52:28 | kashyap | stephenfin: Fair enough. I just rubbing my eyes twice to see if I something else | |
| 17:52:36 | kashyap | s/just/was/ | |
| 18:20:19 | openstackgerrit | Merged openstack/nova master: apidb: Compact Stein database migrations https://review.opendev.org/c/openstack/nova/+/759406 | |
| 18:21:41 | openstackgerrit | Merged openstack/nova master: pci: implement the 'socket' NUMA affinity policy https://review.opendev.org/c/openstack/nova/+/772779 | |
| 18:22:31 | artom | \o/ | |
| 18:38:42 | sean-k-mooney | oh finally | |
| 18:41:14 | sean-k-mooney | stephenfin: why are you creating device of dev_type='VF' | |
| 18:41:16 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/780112/4/nova/tests/functional/libvirt/test_pci_sriov_servers.py#738 | |
| 18:42:08 | stephenfin | cos each vDPA device is associated with a VF? | |
| 18:42:13 | sean-k-mooney | for a start that should be dev_type='type-VF' no if you wanted VFs but in this case it should be dev_type='VDPA' | |
| 18:42:41 | stephenfin | no, that's not creating a PciDevice object | |
| 18:42:46 | stephenfin | that's creating fake libvirt xml | |
| 18:42:47 | sean-k-mooney | right | |
| 18:42:58 | sean-k-mooney | we dont have VF object for the VDPA devices | |
| 18:43:41 | stephenfin | each vdpa device has a '<parent>' element | |
| 18:43:48 | sean-k-mooney | in the DB we will have rows of dev_type='type-PF' and dev_type='vdpa' | |
| 18:44:06 | sean-k-mooney | stephenfin: yep but we dont have VFs in the db | |
| 18:45:08 | stephenfin | Yeah, I know. Again, I'm not creating PciDevice objects here | |
| 18:45:21 | stephenfin | that's just generating libvirt XML for a number of VFs | |
| 18:45:23 | stephenfin | which we do have | |
| 18:45:32 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/770533/12/nova/virt/libvirt/host.py#1236 | |
| 18:45:41 | stephenfin | e.g. 1 PF, 4 VFS, and 1 vDPA dev per VF | |
| 18:45:59 | sean-k-mooney | is this generating the nodedev xmls | |
| 18:46:04 | stephenfin | yes | |
| 18:46:22 | stephenfin | and then we let the libvirt driver generate the PciDevice objects | |
| 18:46:32 | sean-k-mooney | ok so not the data rutrined by _get_pcidev_info or the PciDevice objects | |
| 18:46:36 | stephenfin | no | |
| 18:47:05 | stephenfin | so I'd expect to see see five devices in the database | |
| 18:47:14 | stephenfin | one with type-PF, because I'm whitelisting that | |
| 18:47:18 | stephenfin | and four with 'vdpa' | |
| 18:47:21 | sean-k-mooney | yes | |
| 18:47:38 | stephenfin | we can expand the test to verify that if you like. That would be a good addition | |
| 18:47:55 | sean-k-mooney | no i just want to fiture out how to reuse | |
| 18:47:57 | sean-k-mooney | this | |
| 18:48:05 | sean-k-mooney | i want to create 1 PF and 1 vdpa device | |
| 18:48:29 | sean-k-mooney | can you tell me how to do that | |
| 18:48:33 | stephenfin | you'll want to copy lines 722 - 751 | |
| 18:48:43 | stephenfin | drop the for loop | |
| 18:48:47 | stephenfin | and replace idx with '1' | |
| 18:48:53 | stephenfin | sorry, 0 | |
| 18:49:28 | stephenfin | so what that'll do is generate 1 PCI device with PF caps, 1 PCI device with VF caps, and 1 vDPA device | |
| 18:49:28 | sean-k-mooney | what lines? | |
| 18:49:38 | stephenfin | https://review.opendev.org/c/openstack/nova/+/780112/4/nova/tests/functional/libvirt/test_pci_sriov_servers.py#722 | |
| 18:49:47 | stephenfin | from 'pci_info.add_device(' | |
| 18:50:00 | sean-k-mooney | oh your looking at a differnt version then i was | |
| 18:50:24 | stephenfin | ah yeah, I fixed the broken tests and gibi's comments | |
| 18:50:32 | stephenfin | so that is complete now | |
| 18:50:35 | stephenfin | I think | |
| 18:50:51 | sean-k-mooney | so dev_type PF is not a thing right it should be type-PF that is the value of the constant | |
| 18:50:58 | sean-k-mooney | PF is not a valid PF | |
| 18:51:06 | sean-k-mooney | *dev_type value | |
| 18:51:51 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/objects/fields.py#L753-L761 | |
| 18:52:27 | sean-k-mooney | which is what the pcidevice object uses https://github.com/openstack/nova/blob/63bba50f4336f4b8bf0609b0cbe4717e5d0591d7/nova/objects/pci_device.py#L112 | |
| 18:52:53 | stephenfin | we're not generating PCI devices here | |
| 18:53:07 | stephenfin | https://review.opendev.org/c/openstack/nova/+/780112/4/nova/tests/unit/virt/libvirt/fakelibvirt.py#340 | |
| 18:53:07 | stephenfin | that's just a marker we use to tell the fakelibvirt fixture what type of fake XML to generate | |
| 18:53:29 | stephenfin | we could also pass 'MDEV_TYPES', in which case it would generate XML for a PCI device with mdev capabilities | |
| 18:53:33 | sean-k-mooney | ugh ya | |
| 18:53:46 | sean-k-mooney | we really should not use those contansts though | |
| 18:53:54 | sean-k-mooney | can we make them the same as the real ones | |
| 18:54:19 | sean-k-mooney | i guess we have been using these like this for a while | |
| 18:54:32 | stephenfin | Not really, because we'd need a way to indicate mdevs | |
| 18:54:36 | stephenfin | for XML generation purposes | |
| 18:54:45 | sean-k-mooney | i ocationllay modify this but its really annoying that we have PF and type- | |
| 18:54:50 | sean-k-mooney | type-PF | |
| 18:55:14 | sean-k-mooney | ya im going to sotp using the pci_info as the name | |
| 18:55:31 | stephenfin | I'd rather we moved away from passing the type and instead had e.g. 'add_pci_device', 'add_pci_device_with_mdev', 'add_pci_device_with_vf' etc. helpers | |
| 18:55:38 | stephenfin | that would be clearer IMO | |
| 18:55:52 | sean-k-mooney | ya | |
| 18:56:17 | stephenfin | That would be a good follow-up, but I think my changes there are already too invasive without tacking on even more | |
| 18:56:41 | sean-k-mooney | althoguh i still want fakelibvirt.HostPCIDevicesInfo( num_pci=0, num_pfs=1, num_vfs=0, num_vdpa=2) | |
| 18:57:00 | stephenfin | by all means, go for it. It should be relatively easy to add | |
| 18:57:17 | stephenfin | I only had one caller so I stuck with explicit | |
| 18:57:24 | stephenfin | if we've got more, we can make it more generic | |
| 18:58:28 | sean-k-mooney | im finding it quite hard to follow what that test is doing if im honest | |
| 18:59:05 | sean-k-mooney | espcailly the self.stub_out( | |
| 18:59:07 | sean-k-mooney | 'nova.virt.libvirt.guest.Guest.create', | |
| 18:59:39 | stephenfin | that's a pattern I copied from the SR-IOV test. I want to inspect what the XML being used to create the guest is | |
| 18:59:41 | sean-k-mooney | is vdpa_info.add_device(f'vdpa_vdpa{idx}', idx, vf) | |
| 18:59:42 | stephenfin | so I'm spying on it | |
| 18:59:46 | sean-k-mooney | what makes it a vdpa device | |
| 19:00:11 | sean-k-mooney | right but i dont think we should be doing that in this test | |
| 19:00:22 | sean-k-mooney | or at least i dont think my test should do that | |
| 19:00:35 | stephenfin | no, yours shouldn't. We only need it once | |
| 19:00:44 | stephenfin | it's just to make sure everything is wired up correctly | |
| 19:00:47 | sean-k-mooney | right i would not have put that in the basic create | |
| 19:00:58 | sean-k-mooney | i would have put that explicy in its own test | |