| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-02-21 | |||
| 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 | gibi | * compute level | |
| 15:11:39 | sean-k-mooney | well it is | |
| 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 | gibi | but there are other codes that goes to sysfs without the driver | |
| 15:16:40 | sean-k-mooney | gibi: yes | |
| 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: 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: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: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 | 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 | |
| 15:31:57 | gibi | https://review.opendev.org/c/openstack/nova/+/342301/2/nova/compute/manager.py this added the early parsing | |
| 15:32:32 | gibi | and at that time the pre_start_hook already called update_available_resource so then I don't see how that original bug happened | |
| 15:33:50 | sean-k-mooney | i dont think i reviewd that | |
| 15:33:54 | sean-k-mooney | so im not sure either | |
| 15:33:55 | gibi | it is ooold | |
| 15:34:07 | gibi | anyhow | |
| 15:34:11 | gibi | in summary | |
| 15:34:17 | gibi | we have plenty of coupling | |
| 15:34:24 | gibi | I will try to document that in the bug | |
| 15:34:27 | gibi | I opened | |
| 15:34:50 | gibi | but I'm not sure any more that I want to refactor everything before I fix the PF MAC update problem | |
| 15:35:01 | sean-k-mooney | actully i think this was invalid | |
| 15:35:05 | sean-k-mooney | https://bugs.launchpad.net/nova/+bug/1603034 | |
| 15:35:21 | sean-k-mooney | pci_passthrough_whitelist = [ {"devname": "hed1", "physical_network": "physnet1"},{"physical_network": "physnet1", "address": "*:04:00.0"},{"physical_network": "physnet2", "address": "*:04:00.1"}] | |
| 15:35:30 | sean-k-mooney | they were expecting an error because hed1 was not present | |
| 15:35:37 | sean-k-mooney | but the whitelist parsing shoudl not check that | |
| 15:35:49 | sean-k-mooney | so the change they have should not affect that behvioar | |
| 15:36:11 | gibi | still the whitelist.__init__ caught that based on the attached stack trace | |
| 15:36:22 | gibi | but did not kill the compute | |
| 15:36:53 | sean-k-mooney | it looks like it was found because of _init_dev_details | |
| 15:36:57 | gibi | yepp | |
| 15:37:03 | sean-k-mooney | so at that poitn it was doing the sysfs lookup | |
| 15:37:07 | gibi | yes | |
| 15:37:24 | sean-k-mooney | ya so that was actully already a bug | |
| 15:37:39 | sean-k-mooney | since it shoudl not call sysfs as we have been disucssing | |
| 15:38:20 | sean-k-mooney | gibi: ok so you are writign up a bug for this | |
| 15:38:39 | sean-k-mooney | and are going to try an capture this old context | |
| 15:39:04 | gibi | yes | |