| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-02-07 | |||
| 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:flavor name="r1.small"> | |
| 20:02:47 | chateaulav | <nova:memory>4096</nova:memory> | |
| 20:02:47 | chateaulav | <nova:disk>16</nova:disk> | |
| 20:02:47 | chateaulav | <nova:swap>0</nova:swap> | |
| 20:02:47 | chateaulav | <nova:ephemeral>0</nova:ephemeral> | |
| 20:02:48 | chateaulav | <nova:vcpus>2</nova:vcpus> | |
| 20:02:48 | chateaulav | </nova:flavor> | |
| 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: Join quota exception family trees https://review.opendev.org/c/openstack/nova/+/828185 | |
| 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: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: Introduce remote_managed tag for PCI devs https://review.opendev.org/c/openstack/nova/+/824834 | |
| 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: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 | 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 | |