| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-02-08 | |||
| 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 | |
| 14:48:51 | opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP tree DB query to support any-traits https://review.opendev.org/c/openstack/placement/+/825849 | |
| 14:48:56 | opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for listing resource providers https://review.opendev.org/c/openstack/placement/+/826491 | |
| 14:49:02 | opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for allocation candidates https://review.opendev.org/c/openstack/placement/+/826492 | |
| 14:49:08 | opendevreview | Balazs Gibizer proposed openstack/placement master: Remove unused compatibility code https://review.opendev.org/c/openstack/placement/+/826493 | |
| 14:49:19 | opendevreview | Balazs Gibizer proposed openstack/placement master: Add microversion 1.39 to support any-trait queries https://review.opendev.org/c/openstack/placement/+/826719 | |
| 14:49:37 | opendevreview | Dmitrii Shcherbakov proposed openstack/nova master: Introduce remote_managed tag for PCI devs https://review.opendev.org/c/openstack/nova/+/824834 | |
| 14:52:08 | stephenfin | sean-k-mooney: Yoga is due the end of March, yeah? Assuming we can drop Python 3.6 (and maybe 3.7) then, I think we should just wait. unittest.mock has a solid number of bugs in 3.6 that we're having to work around, including that one in the func test | |
| 14:52:44 | stephenfin | gibi: since you've been following that long-running effort ^ | |
| 14:57:12 | gibi | stephenfin: I'm not sure what are the plans about dropping py36 | |
| 14:57:23 | sean-k-mooney | gibi: we really reallly need too | |
| 14:57:36 | gibi | but overall agree that if we can drop py36 that would help | |
| 14:57:39 | sean-k-mooney | gibi: we shoudl not keep support after yoga | |
| 14:58:08 | sean-k-mooney | i wanted to drop it this cycle but we kept it for centos 8 for one more release | |
| 14:58:25 | sean-k-mooney | we shoudl be droping centos 8 entirly next release and with it 36 | |
| 14:58:28 | gibi | ahh, yes centos 8 was the reason we kept it | |
| 14:58:46 | sean-k-mooney | stephenfin: so yes let hold off so till we drop centos 8 and 36 | |
| 14:58:53 | gibi | OK, lets drop centos 8 and py36 and then we can merge the unittest change a easier | |
| 14:58:57 | gibi | sean-k-mooney: agree | |
| 14:59:23 | stephenfin | Sweet. I'll mark as -W and comment as much. Thanks | |
| 14:59:33 | sean-k-mooney | speaking of cento i shoudl proably finsh my "stack on c9s" and see what breaks taks | |
| 14:59:58 | gibi | melwitt: thank you so much for review on the any-traits series. I've fixed your comments. | |
| 15:01:14 | sean-k-mooney | chateaulav: by the way are you planning to split up https://review.opendev.org/c/openstack/nova/+/822053/34 | |
| 15:02:10 | chateaulav | sean-k-mooney: i can,if that makes things easier? | |
| 15:02:14 | sean-k-mooney | im not sure how tightly coupled that change is but its getting pretty large | |