Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-02-21
14:57:31 gibi so I think today it would be extremely hard to add support to PCI for a non Linux host
14:57:45 sean-k-mooney it could certenly be factored more but i guess the expecation is you woudl add windows suport to the pci module if that ever came to be a thing
14:58:01 gibi hm
14:58:10 gibi yeah that would be an option too
14:58:14 sean-k-mooney gibi: well you would have to just implenet the supprot in the pci.utils module i
14:58:36 gibi I might have been overzealeus about this
14:58:50 sean-k-mooney its what we do in os-vif for what its worth https://github.com/openstack/os-vif/tree/master/os_vif/internal/ip
14:59:16 sean-k-mooney we have an ip module with a windows and linux impl
14:59:22 gibi yeah maybe that is a better direction
14:59:44 gibi maybe I just wish for a better defined pci.utils module
14:59:51 gibi where the interface is clear
15:00:08 sean-k-mooney ack
15:00:18 sean-k-mooney we kind of have grown it organicly over time
15:00:27 gibi yes, it is very organic :)
15:00:56 gibi like the init_host in the compute manager parses the whitelist config early but by that it already reading sysfs a lot :)
15:01:15 sean-k-mooney at some point i think we will want to unify mdev+pci+vdpa into a more uniform device tracker imple
15:01:16 gibi even if the compute manager itself does not use the whitelsit config at all
15:01:34 gibi sean-k-mooney: yeah that is also an interesting direction
15:01:43 sean-k-mooney gibi: parsing the whitelist does not hit sysfs
15:01:47 gibi it does
15:01:50 sean-k-mooney it should not
15:01:53 gibi yeah :d
15:01:58 gibi :D
15:02:01 sean-k-mooney parsing the whitelist does not look at the devices
15:02:13 sean-k-mooney it just reads the json and constucts the filter object
15:02:31 sean-k-mooney it should not make a single call to sysfs
15:02:50 sean-k-mooney https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/pci/whitelist.py#L56-L83
15:03:26 gibi https://github.com/openstack/nova/blob/0c31561792e0e13a9f8267e71fa484ab79957f04/nova/pci/devspec.py#L287
15:03:34 gibi this is part of the Whitelist.__init__ codepath
15:03:59 gibi via PciDeviceSpec
15:04:32 sean-k-mooney it really should not me
15:04:37 sean-k-mooney https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/pci/whitelist.py#L80
15:04:44 sean-k-mooney is where you say the dep happens
15:05:12 sean-k-mooney that shoudl not be happening there
15:05:59 sean-k-mooney we shoudl only be asseting the valudes in the config are valid here
15:06:09 gibi yeah L80 eventually calls _init_dev_details in the PciDeviceSpec
15:06:18 sean-k-mooney i guess we just added the validation for remote managed here
15:06:29 gibi yes that is remote managed part now that calls out to sysfs
15:06:41 sean-k-mooney gibi: ok well the intent is we shoudl not hit sysfs in this code path
15:07:08 sean-k-mooney i had suggested moving the valdiation logic out of there and into the virt driver for remote-managed
15:07:14 sean-k-mooney perhaps we shoudl do that
15:08:17 sean-k-mooney everthing here https://github.com/openstack/nova/blob/0c31561792e0e13a9f8267e71fa484ab79957f04/nova/pci/devspec.py#L315-L351 shoudl really be in teh virt driver
15:08:54 gibi I think there is two places now that potentiall hit the remote managed validation path. 1) the init_host in the compute manager to early exit the compute service 2) the pci device tracker that cannot do anything with an invalid config as it is called from a periodic
15:09:25 gibi so we potentially need a 3rd place that is in the virt driver and part of the init sequence
15:09:33 sean-k-mooney init_host calls the driver
15:09:42 sean-k-mooney that si where this validation shoudl be done
15:10:27 sean-k-mooney in here https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/virt/libvirt/driver.py#L707
15:11:09 gibi yeah the confusion is that the validation is part of the whitelist config parsing and that config is compute level not libvirt level :/
15:11:31 sean-k-mooney gibi: its not
15:11:33 gibi so it make sense to validate whitelist at the config level
15:11:39 sean-k-mooney well it is
15:11:39 gibi * compute level
15:11:56 sean-k-mooney validatign the whitelist is just ment to validate teh syntax
15:12:20 sean-k-mooney with some minor validation of the value in that product ids shoudl be in a certin range of allowed values
15:12:48 sean-k-mooney then avlidation of the semantics is ment to bve done in teh virt driver
15:14:05 sean-k-mooney i raised this in the reivew but i guess i did not see that code path. i did not think that was going to call those fucntion so that is a regression
15:15:39 dmitriis gibi: there's also the code path which uses interface names that calls out to sysfs
15:16:02 sean-k-mooney yes that is the qos code
15:16:11 gibi dmitriis: yes, there is a bunch of other coupling point. It is not just your patches introduced that
15:16:20 gibi sean-k-mooney: the qos code is going through the virt driver
15:16:22 gibi pci_info
15:16:40 sean-k-mooney gibi: yes
15:16:40 gibi but there are other codes that goes to sysfs without the driver
15:19:10 sean-k-mooney gibi: hum https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/virt/hyperv/hostops.py#L194-L217 so hyperv apparently does support pci passhtough
15:19:38 sean-k-mooney but maybe not sriov
15:19:45 sean-k-mooney at least not with qos
15:19:54 sean-k-mooney well neutron sriov ports i mean
15:20:22 sean-k-mooney but we cant depend on sysfs in the whitelist parseing of it woudl break hyperv potentially
15:20:34 sean-k-mooney right now its guarded by the remote managed tag
15:20:47 sean-k-mooney so it wont actully break anything unless you set that in the whitelist
15:21:06 sean-k-mooney but this is much more fragile then i would like
15:21:50 gibi hyperv does not use the pci device tracker I assume
15:21:58 sean-k-mooney i think it does
15:22:16 sean-k-mooney but the current code pats only run if you set remote_manged=true
15:22:29 sean-k-mooney and we catch error for the interface lookup in the qos path
15:22:51 sean-k-mooney which you also said is called form teh virt driver so it would not trigger an error in hyperv
15:22:55 gibi they do a very different and limited device matching https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/virt/hyperv/vmops.py#L438-L444
15:23:01 gibi just vendor and product id
15:23:06 sean-k-mooney yes
15:23:21 gibi so they are not relying on the whitelist pcidevicespec matching logic
15:23:22 sean-k-mooney hyperv does not allow them to access the pci address
15:23:41 sean-k-mooney the have an indrict handel via a uuid if i rememebr correctly
15:24:04 sean-k-mooney so they cant use the devname or adress fields
15:24:11 dmitriis sean-k-mooney: yeah, with the current code, runtime checks would only be triggered if the tag is set to true
15:24:12 sean-k-mooney just vendor id and prduct id
15:26:37 sean-k-mooney gibi: dmitriis so right now the pci device are first loaded form teh hypervior via https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/compute/manager.py#L1554-L1560
15:27:03 sean-k-mooney the compute mangers pre start hook calls update_available_resource
15:27:33 sean-k-mooney which calls into driver which gets the avaible resouces
15:27:59 sean-k-mooney that is the entrypoint that should raise the exction that will kill the comptue service if the confguration is invlaide
15:28:41 sean-k-mooney https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/compute/resource_tracker.py#L879
15:28:50 sean-k-mooney is where the resouce tracker called the virt driver
15:28:55 opendevreview Jonathan Race proposed openstack/nova master: driver/secheduler/docs for Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053
15:28:55 opendevreview Jonathan Race proposed openstack/nova master: object/notification for Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/828369
15:28:56 opendevreview Jonathan Race proposed openstack/nova master: zuul-job for Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/828372
15:29:16 sean-k-mooney which gets the pci passthough info https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/virt/libvirt/driver.py#L8984
15:29:51 gibi sean-k-mooney: so the comment in https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/compute/manager.py#L1388-L1396 is not valid in the sense that the pci device tracker could kill the compute service not just the periodic task
15:30:10 sean-k-mooney that is used to init the pci tracker here https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/compute/resource_tracker.py#L761
15:30:38 sean-k-mooney gibi: yes it should be able to kill both
15:31:57 gibi https://review.opendev.org/c/openstack/nova/+/342301/2/nova/compute/manager.py this added the early parsing
15:31:57 sean-k-mooney gibi: so yes loadign the whitelisth ehre kills things early if the syntax is wrong but the pci tracker can kill the agent and or the perodic later if it whishes by raising an excption

Earlier   Later