| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-12 | |||
| 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 | sean-k-mooney | what lines? | |
| 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: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 | that's just a marker we use to tell the fakelibvirt fixture what type of fake XML to generate | |
| 18:53:07 | stephenfin | https://review.opendev.org/c/openstack/nova/+/780112/4/nova/tests/unit/virt/libvirt/fakelibvirt.py#340 | |
| 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 | |
| 19:01:24 | stephenfin | I didn't want two tests that did the exact same thing and only changed what they were looking at | |
| 19:01:59 | stephenfin | If I had another test for e.g. resizing, I wouldn't bother with this | |
| 19:02:18 | stephenfin | I just want to make sure what we're handing off to neutron and libvirt is correct | |
| 19:02:51 | stephenfin | i.e. black box testing, looking at only at the inputs and outputs (from the nova service to other services) | |
| 19:03:07 | sean-k-mooney | yep i know anyway i think i have figured out what is needed an not thanks | |
| 19:03:31 | sean-k-mooney | stephenfin: it just a case of you are asserting two thing in one test and i was trying to fiture out what was the minium i needed | |
| 19:03:43 | stephenfin | gotcha | |
| 19:03:53 | stephenfin | yeah, to recap you don't need the libvirt spy | |
| 19:04:02 | stephenfin | nor do you really need to look at the neutron requests | |
| 19:04:11 | stephenfin | or PCI device counts in the DB | |
| 19:04:37 | stephenfin | just create a port, create a server, and then try your various and see that they're correctly rejects, I guess? | |
| 19:04:44 | sean-k-mooney | and i should not need to pass the libvirt and qemu version explictly either | |
| 19:05:01 | sean-k-mooney | since you are passing the default | |
| 19:05:20 | stephenfin | you will | |
| 19:05:32 | stephenfin | the FakeLibvirtFixture defaults to the minimums | |
| 19:05:34 | stephenfin | which are too low | |