| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-02-07 | |||
| 18:53:03 | sean-k-mooney | dmitriis: you shoudl not be doing https://review.opendev.org/c/openstack/nova/+/827839/2/nova/pci/devspec.py#319 | |
| 18:53:23 | sean-k-mooney | that is not the correct place to raise that excption | |
| 18:53:36 | sean-k-mooney | either the device spec of whitelist shoudl raise that | |
| 18:53:46 | dmitriis | sean-k-mooney: ah, sorry, yes we're moving it elsewhere | |
| 18:54:40 | dmitriis | sean-k-mooney: so maybe in the dev tracker I have access to that info, checking | |
| 18:54:58 | sean-k-mooney | i dont think you do | |
| 18:55:09 | sean-k-mooney | we abstrackt the virt driver info away | |
| 18:55:20 | sean-k-mooney | the pci module is ment ot be virt dirver independent | |
| 18:55:32 | sean-k-mooney | you shoudl be doing this check in the libvirt driver.py | |
| 18:56:19 | sean-k-mooney | you coudl perhaps do it in update_devices_from_hypervisor_resources | |
| 18:56:21 | sean-k-mooney | https://github.com/openstack/nova/blob/b6fe7521afa8d42febc68f5f79782f7bcc3b568f/nova/pci/manager.py#L111 | |
| 18:56:36 | sean-k-mooney | but you would have to modify the device_json which im not sure is the right thing | |
| 18:57:40 | sean-k-mooney | am i need to call it a day | |
| 18:57:56 | sean-k-mooney | dmitriis: can you look at moving the check to the libvirt driver | |
| 18:57:56 | dmitriis | sean-k-mooney: ack, I'll think about how to do it | |
| 18:58:09 | dmitriis | sean-k-mooney: sure | |
| 19:32:39 | opendevreview | Dan Smith proposed openstack/nova master: Move keypair quota error message into exception https://review.opendev.org/c/openstack/nova/+/828186 | |
| 19:32:39 | opendevreview | Dan Smith proposed openstack/nova master: Join quota exception family trees https://review.opendev.org/c/openstack/nova/+/828185 | |
| 19:32:40 | dansmith | melwitt: ^ | |
| 19:32:51 | dansmith | I have one other thing I'm going to stack on there, but it'll take me a few more minutes | |
| 19:33:51 | melwitt | dansmith: ack | |
| 19:34:30 | opendevreview | Dan Smith proposed openstack/nova master: Move keypair quota error message into exception https://review.opendev.org/c/openstack/nova/+/828186 | |
| 20:02:47 | chateaulav | <nova:ephemeral>0</nova:ephemeral> | |
| 20:02:47 | chateaulav | <nova:swap>0</nova:swap> | |
| 20:02:47 | chateaulav | <nova:disk>16</nova:disk> | |
| 20:02:47 | chateaulav | <nova:memory>4096</nova:memory> | |
| 20:02:47 | chateaulav | <nova:flavor name="r1.small"> | |
| 20:02:48 | chateaulav | </nova:flavor> | |
| 20:02:48 | chateaulav | <nova:vcpus>2</nova:vcpus> | |
| 20:03:13 | chateaulav | sean-k-mooney: so setting any topology items doesnt work | |
| 20:03:26 | chateaulav | the above and below are what is set in regards to cpu | |
| 20:03:31 | chateaulav | https://www.irccloud.com/pastebin/f9YIhb6z/ | |
| 20:09:44 | opendevreview | Dan Smith proposed openstack/nova master: Move keypair quota error message into exception https://review.opendev.org/c/openstack/nova/+/828186 | |
| 20:09:44 | opendevreview | Dan Smith proposed openstack/nova master: Join quota exception family trees https://review.opendev.org/c/openstack/nova/+/828185 | |
| 20:12:32 | opendevreview | melanie witt proposed openstack/nova master: Raise InstanceNotFound on fkey constraint fail saving info cache https://review.opendev.org/c/openstack/nova/+/826942 | |
| #openstack-nova - 2022-02-08 | |||
| 00:58:47 | opendevreview | Keigo Noha proposed openstack/nova stable/wallaby: libvirt: Always delegate OVS plug to os-vif https://review.opendev.org/c/openstack/nova/+/828148 | |
| 08:07:36 | opendevreview | Ilya Popov proposed openstack/nova master: Fix to implement 'pack' or 'spread' VM's NUMA cells https://review.opendev.org/c/openstack/nova/+/805649 | |
| 10:18:23 | kashyap | sean-k-mooney: Do you know if Swift is requirement for vTPM to work? The docs say no: https://docs.openstack.org/nova/latest/admin/emulated-tpm.html | |
| 10:18:39 | kashyap | But the spec mentions "Swift" | |
| 10:19:12 | sean-k-mooney | no its not | |
| 10:19:26 | sean-k-mooney | the storage to swift was never implemented | |
| 10:20:02 | kashyap | Ah-ha, good to know. | |
| 10:20:03 | kashyap | Thank you | |
| 10:20:39 | sean-k-mooney | if i recall correctly that was propsed for shelve which i belive we did not implement | |
| 10:21:30 | sean-k-mooney | kashyap: barbican is needed however for the key management | |
| 10:21:45 | kashyap | Yeah, figured as much from the doc | |
| 11:33:28 | opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: [yoga] Include pf mac and vf num in port updates https://review.opendev.org/c/openstack/nova/+/824833 | |
| 11:33:29 | opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: Bump os-traits to 2.7.0 https://review.opendev.org/c/openstack/nova/+/826675 | |
| 11:33:29 | opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: Introduce remote_managed tag for PCI devs https://review.opendev.org/c/openstack/nova/+/824834 | |
| 11:38:58 | dmitriis | sean-k-mooney: added handling for the PF PCI address + vf product/device id + remote managed tag case by using some additional info from sysfs: https://review.opendev.org/c/openstack/nova/+/824834/10/nova/pci/devspec.py#322 Also added unit tests to cover various ways to specify this in a whitelist | |
| 11:39:57 | dmitriis | handling of ANY ("*") for vendor/product id along with PF PCI address specified is interesting | |
| 11:45:11 | sean-k-mooney | well if its a PF and you dont specify the vendor and product id of the vf i think we can treat that as an error | |
| 11:45:24 | sean-k-mooney | since pfs are not supported | |
| 11:45:47 | sean-k-mooney | just using the adress of the pf woudl allow the pf not the vfs normally i belive | |
| 12:22:52 | dmitriis | comparison of its vendor_id/product_id against both ANY and the device dict passed in. | |
| 12:22:52 | dmitriis | sean-k-mooney: looks like WhitelistPciAddress.match treats the presence of pci_phys_addr as an indicator that the caller is trying to match a VF https://review.opendev.org/c/openstack/nova/+/824834/10/nova/pci/devspec.py#252 And then PciDeviceSpec.match https://review.opendev.org/c/openstack/nova/+/824834/10/nova/pci/devspec.py#377 does a | |
| 12:30:30 | dmitriis | IIUC, something like this `passthrough_whitelist = {"vendor_id":"*","product_id":"*", "address":"0000:0a:00.0"}` would match both a PF with address 0000:0a:00.0 and any vendor/device ID, and VFs of any PF with a PCI address 0000:0a:00.0 | |
| 12:33:02 | dmitriis | so, in summary: I could block this explicitly if remote_managed: "true" is present because it allows both PFs and VFs to be matched but we want to prohibit PFs. | |
| 12:35:47 | sean-k-mooney | dmitriis: sorry was on a meeting reading back | |
| 12:35:52 | dmitriis | np | |
| 12:36:51 | sean-k-mooney | you mean block "vendor_id":"*","product_id":"*", explcitly if remote managed | |
| 12:37:24 | sean-k-mooney | i think you need to do this outside of the whitelist/devspec parseing | |
| 12:38:17 | sean-k-mooney | when we are constucting the device pools in the pci tracker we could block it as we woudl know if its a pf | |
| 12:46:41 | sean-k-mooney | dmitriis: im just reviewing you latest patches by the way lets continue this conversation after that | |
| 12:47:27 | dmitriis | sean-k-mooney: ack, trying to figure out what to move to PciDevTracker. There's some remove-managed-related logic already in the PciDeviceSpec in the current patches | |
| 12:49:45 | sean-k-mooney | one important thign to rememebr is the nova/pci fould has to be driver independent so we need to ensure that we do not do any checks there that would be driver dependent. those need to live in the virt driver. in this case i think you can do checks pci manager possible in update_devices_from_hypervisor_resources | |
| 12:50:34 | sean-k-mooney | similar to https://github.com/openstack/nova/blob/master/nova/pci/manager.py#L134-L136 | |
| 12:51:11 | sean-k-mooney | that is currently delegating back to the whitelist class https://github.com/openstack/nova/blob/master/nova/pci/whitelist.py#L85-L93 | |
| 12:51:49 | sean-k-mooney | but its really just validting the whitelist data is correct and that the device being tested is allowed | |
| 12:52:50 | sean-k-mooney | dmitriis: i could maybe live with a check here https://github.com/openstack/nova/blob/master/nova/pci/whitelist.py#L92 | |
| 12:53:17 | sean-k-mooney | assert that dev is not a pf if spec has remote managed | |
| 12:55:28 | sean-k-mooney | dmitriis: perhaps it would make sense to change device_assignable to return the spec that allowed it so that you can use it to do other validation | |
| 12:55:37 | dmitriis | sean-k-mooney: ok, I think I see where you are going. While the PF and VF-related checks for the remote-managed tag are not themselves driver-dependent, whether we need to do them or not is driver-depenent. | |
| 12:56:14 | dmitriis | s/depenent/dependent/ | |
| 12:57:02 | sean-k-mooney | well yes and no the data required to do the check is driver depenent since we need to get the list of device form the driver | |
| 12:57:14 | sean-k-mooney | i guess technially you could get that from sysfs. | |
| 12:57:32 | sean-k-mooney | im concerned that might break powervm | |
| 12:57:53 | sean-k-mooney | or hyperv but im not sure it would | |
| 12:58:36 | dmitriis | yeah, I added a couple of utils to do sysfs lookup in the last upload https://review.opendev.org/c/openstack/nova/+/824834/10/nova/pci/devspec.py#324 https://review.opendev.org/c/openstack/nova/+/824834/10/nova/pci/utils.py#217 | |
| 12:59:05 | sean-k-mooney | so https://github.com/openstack/nova/blob/master/nova/pci/utils.py#L132-L144 could be called by the whitelist | |
| 12:59:24 | sean-k-mooney | in device_assignable | |
| 12:59:35 | sean-k-mooney | to check if the dev is a pf | |
| 12:59:52 | sean-k-mooney | and you could see if it has remote managed and return true or false | |
| 13:00:02 | sean-k-mooney | that would filter out the pfs | |
| 13:01:03 | sean-k-mooney | may main concern with tha tis currently device_assinable does not make casles to the underlying system | |
| 13:01:25 | sean-k-mooney | it used the in memory specs list and the data passed in | |
| 13:02:57 | dmitriis | sean-k-mooney: there's one code-path where it may do it https://github.com/openstack/nova/blob/master/nova/pci/devspec.py#L290 | |
| 13:03:16 | dmitriis | get_function_by_ifname uses sysfs for the netdev lookup | |
| 13:03:43 | sean-k-mooney | yes indriectly by the match function | |
| 13:03:47 | sean-k-mooney | with dev name | |
| 13:04:12 | sean-k-mooney | so ya if it works for you i woudl move the check to device_assignable | |
| 13:04:32 | sean-k-mooney | although | |
| 13:04:41 | sean-k-mooney | you curently have it in _init_dev_details | |
| 13:05:45 | sean-k-mooney | dmitriis: im just looking at https://review.opendev.org/c/openstack/nova/+/824834/9..10/nova/pci/devspec.py#287 | |
| 13:06:57 | dmitriis | ack | |
| 13:08:09 | opendevreview | Stephen Finucane proposed openstack/placement master: db: Use Row, not LegacyRow https://review.opendev.org/c/openstack/placement/+/828305 | |
| 13:08:09 | opendevreview | Stephen Finucane proposed openstack/placement master: tox: Enable SQLAlchemy 2.0 warnings https://review.opendev.org/c/openstack/placement/+/801108 | |
| 13:17:13 | dmitriis | raising exceptions in _init_dev_details | |
| 13:17:13 | dmitriis | sean-k-mooney: the goal of moving the checks closer to parsing was to avoid devices from getting into the pools in the first place (to avoid filtering PFs in PciDeviceStats) and for early config error detection. During the earlier reviews I went from (1) filtering in PciDeviceStats -> (2) returning false while matching in PciDeviceSpec -> (3) | |
| 13:17:30 | sean-k-mooney | dmitriis: while i dont nessisaly love doing these remote managed check in _init_dev_details i think im ok with it for now | |