| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-02-08 | |||
| 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:22:52 | dmitriis | comparison of its vendor_id/product_id against both ANY and the device dict passed in. | |
| 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: tox: Enable SQLAlchemy 2.0 warnings https://review.opendev.org/c/openstack/placement/+/801108 | |
| 13:08:09 | opendevreview | Stephen Finucane proposed openstack/placement master: db: Use Row, not LegacyRow https://review.opendev.org/c/openstack/placement/+/828305 | |
| 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:13 | dmitriis | raising exceptions in _init_dev_details | |
| 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 | |
| 13:18:22 | sean-k-mooney | dmitriis: your current code avoids needeign to pass allow remote managed allover the place which was the main code smell | |
| 13:18:36 | sean-k-mooney | we have other checks like | |
| 13:18:38 | sean-k-mooney | if address and self.dev_name: | |
| 13:18:41 | sean-k-mooney | raise exception.PciDeviceInvalidDeviceName() | |
| 13:18:46 | sean-k-mooney | in _init_dev_details | |
| 13:18:52 | sean-k-mooney | so it kind of fits there | |
| 13:19:55 | dmitriis | sean-k-mooney: well, the other thing still needs addressing I think: https://review.opendev.org/c/openstack/nova/+/827839/2/nova/pci/devspec.py#318. Do we want to check for the use of `remote_managed: "true"` in case a driver doesn't support it? | |
| 13:20:22 | dmitriis | perhaps not in the devspec.py code but I could try to expose a property on the whitelist and check for this elsewhere | |
| 13:20:24 | sean-k-mooney | no i dont think so | |
| 13:20:34 | sean-k-mooney | we could | |
| 13:20:38 | dmitriis | sean-k-mooney: ok, so I can then just drop this altogether | |
| 13:20:42 | sean-k-mooney | but the request filter woudl block it | |
| 13:20:50 | sean-k-mooney | with the capablty traits | |
| 13:21:35 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/812111/22/nova/scheduler/request_filter.py | |
| 13:21:40 | sean-k-mooney | that is unconditionally enabled | |
| 13:21:57 | sean-k-mooney | so as long as the driver does not incorrectly report the trait | |
| 13:22:03 | dmitriis | so, we are essentially allowing `remote_managed: "true"` to be specified in the config even if a particular driver doesn't support it. It will not be surfaced to an operator early but other parts of the system will work as they should | |
| 13:22:06 | sean-k-mooney | it the host will not be included in the set form placemnt | |
| 13:22:48 | sean-k-mooney | i guess we could see how gibi feels but i think documenation fo "this only work with libvirt" + the prefileter is enough | |
| 13:24:28 | dmitriis | Works for me. I'll just drop the extra check in the other patch for now and we can conclude there if we are ok with the result | |
| 13:24:53 | dmitriis | since we document it for deployers/operators it should be ok in my view | |
| 13:25:16 | sean-k-mooney | cool | |
| 13:25:54 | sean-k-mooney | so assuming the neutorn lib stuff gets sorted we hopefully can get this all merged in the next week or two | |
| 13:26:03 | sean-k-mooney | dmitriis: what is the state of the neutron code | |
| 13:27:25 | dmitriis | sean-k-mooney: just got updated to include the new VNIC_TYPE_REMOTE_MANAGED https://review.opendev.org/c/openstack/neutron/+/808961/. Otherwise fnordahl promoted it from the WIP status and is waiting for reviews | |
| 13:28:10 | dmitriis | there's also a second patch around extra validation https://review.opendev.org/c/openstack/neutron/+/818420 | |
| 13:28:56 | dmitriis | the test failure on the first is about VNIC_TYPE_REMOTE_MANAGED so we're just waiting for it to land | |
| 13:30:14 | sean-k-mooney | ok we likely need a depends on link agaisnt the neutorn series at some point in the nova one | |
| 13:31:17 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/812111/22 should also be before https://review.opendev.org/c/openstack/nova/+/824835/13 i think | |
| 13:32:31 | sean-k-mooney | dmitriis: when you respin those for the vnic type change can you put the filter before the final patch that enables the feature | |
| 13:33:03 | sean-k-mooney | if that is a lot of work we can proably keep the order as is and just merge tehm together | |
| 13:33:29 | sean-k-mooney | /together/at the same time/ | |
| 14:07:35 | dmitriis | sean-k-mooney: ack, the filter one needs `is_smartnic_port` from the previous patch which I am going to replace to is_remote_managed_port | |
| 14:08:18 | dmitriis | so I guess I could add the is_remote_managed_port in the filter patch instead and reorder | |
| 14:08:54 | sean-k-mooney | if that is not a lot of touble i think it woudl better yes but ill leave that to you | |
| 14:09:40 | sean-k-mooney | it just chagnes if we merge the patch all at one or together. | |
| 14:14:20 | dmitriis | sean-k-mooney: ack, started looking into it now (had to attend a call) | |
| 14:14:33 | opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | |
| 14:43:11 | sean-k-mooney | stephenfin: if you rebase https://review.opendev.org/c/openstack/nova/+/705792/12 it might help | |
| 14:43:22 | sean-k-mooney | stephenfin: we are now skiping the failing test https://review.opendev.org/c/openstack/nova/+/705792/12 | |
| 14:43:31 | stephenfin | zuul should do that for me | |
| 14:43:38 | stephenfin | so long as there are no conflicts | |
| 14:43:48 | sean-k-mooney | it should but i think the last run did not have it | |
| 14:43:51 | sean-k-mooney | perhaps | |
| 14:43:53 | sean-k-mooney | it was before that merged | |
| 14:44:25 | sean-k-mooney | oh i ment https://review.opendev.org/c/openstack/nova/+/827851 | |
| 14:44:35 | sean-k-mooney | ah it was | |
| 14:44:41 | sean-k-mooney | the last run was on the 3rd | |
| 14:44:50 | sean-k-mooney | the skip merged on the 5th | |
| 14:45:57 | sean-k-mooney | stephenfin: after those two patches merge care to respin your mock replacement patch? | |
| 14:46:56 | sean-k-mooney | oh looks like there is a func tst failure... | |
| 14:48:40 | opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP db query to support any-traits https://review.opendev.org/c/openstack/placement/+/825848 | |
| 14:48:41 | opendevreview | Balazs Gibizer proposed openstack/placement master: DB layer should only depend on trait id not names https://review.opendev.org/c/openstack/placement/+/826490 | |
| 14:48:51 | opendevreview | Balazs Gibizer proposed openstack/placement master: Enhance doc of _get_trees_with_traits https://review.opendev.org/c/openstack/placement/+/825780 | |