| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-02-21 | |||
| 12:36:52 | gibi | sean-k-mooney: filed the bug for the pci.utils networking.neutron coupling issue https://bugs.launchpad.net/nova/+bug/1961587 | |
| 12:59:47 | sean-k-mooney | ack just triaged it | |
| 13:43:45 | gibi | thanks | |
| 14:22:06 | opendevreview | Alexey Stupnikov proposed openstack/nova master: Add functional tests to reproduce bug #1960412 https://review.opendev.org/c/openstack/nova/+/830010 | |
| 14:25:12 | opendevreview | Alexey Stupnikov proposed openstack/nova master: Add functional tests to reproduce bug #1960412 https://review.opendev.org/c/openstack/nova/+/830010 | |
| 14:38:27 | sean-k-mooney | gibi: have i mentioned how much i hate the fact that gerrit forward ports comments now | |
| 14:38:37 | sean-k-mooney | its such a ux regressions | |
| 14:38:59 | gibi | it is only a problem iff we forget to close the comments when they are solved | |
| 14:39:18 | sean-k-mooney | liek these ones https://review.opendev.org/c/openstack/nova/+/821606/10/nova/virt/libvirt/volume/lightos.py | |
| 14:39:35 | sean-k-mooney | gibi: also this has been retoactivly enabeld for all reviews in the past | |
| 14:39:51 | gibi | yeah, that retroactive thing is a pain I agree | |
| 14:40:03 | sean-k-mooney | so if we lookup a revew becasue we are lookign at git blame we will now get all the unresoved ones where the feature did not exist | |
| 14:41:04 | sean-k-mooney | im going to ack most of the commend on that once i check them but to me this breaks the workflow since if the converation is happenign betwen reviewrs rather then just the revier and authour | |
| 14:44:17 | gibi | yeah | |
| 14:44:44 | gibi | not optimal but in the other hand it helps when people only fix a set of comments and forget the rest | |
| 14:44:49 | gibi | and push a new PS | |
| 14:45:16 | sean-k-mooney | yes and know. it can but over all i think this is a more harmful change then good | |
| 14:45:41 | gibi | It forces me to close comment when they fixed or answered | |
| 14:45:50 | gibi | I think in time we can learn to use it well | |
| 14:45:54 | sean-k-mooney | right but i dont think you should have to do that | |
| 14:46:10 | sean-k-mooney | i am hoping we will get the option to disable it personlly | |
| 14:46:48 | sean-k-mooney | its the frist time that gerrit has made a change that has made me consider if we shoudl continue to use gerrit longterm | |
| 14:47:14 | sean-k-mooney | to me the fact that comments were pined to reveriosn was one of the big benifits for using gerrit | |
| 14:47:17 | sean-k-mooney | this regresses that | |
| 14:48:00 | sean-k-mooney | i can see the beniftis too but to me in its current form the beiftis do not outway the downside | |
| 14:48:21 | gibi | hm, maybe a toggle in the UX to show / hide the old PS comments can be a compromise | |
| 14:48:32 | opendevreview | yuval proposed openstack/nova master: Lightbits LightOS driver https://review.opendev.org/c/openstack/nova/+/821606 | |
| 14:48:38 | sean-k-mooney | if it only forward ported the comment that refered to lines that were modified in the next revision i could proably live with that | |
| 14:49:35 | yuval_ | gibi sean-k-mooney thanks for the fast reply! | |
| 14:49:41 | sean-k-mooney | like its infurating that i have to load back up ps 8 to see why they refer too | |
| 14:50:53 | gibi | yuval_: will check the last version before end of my day | |
| 14:52:16 | yuval_ | thanks, I just updated as sean-k-mooney suggested the req file and lower constraints | |
| 14:52:23 | yuval_ | to see zuul pass | |
| 14:52:47 | yuval_ | I am now re-adding the lightbits ci to work on nova repo | |
| 14:53:00 | sean-k-mooney | yuval_: https://review.opendev.org/c/openstack/nova/+/821606/11/nova/virt/libvirt/volume/lightos.py#31 can you respond to this too | |
| 14:53:40 | yuval_ | yes, no problem | |
| 14:53:45 | dmitriis | sean-k-mooney, gibi: w.r.t. the above ping, I can chime in to help with reviews | |
| 14:54:14 | gibi | dmitriis: thanks | |
| 14:55:08 | gibi | dmitriis, sean-k-mooney: w.r.t. the sysfs pci_utils network.neutron coupling, I started looking into the refactor and I have to say that the whole resource tracker is infected with teh sysfs coupling via the pci device tracker | |
| 14:55:52 | sean-k-mooney | gibi: yes that is not surpriseing to be honest | |
| 14:56:08 | gibi | so this sean-k-mooney but then we are back to that the compute manager depends on sysfs | |
| 14:56:16 | sean-k-mooney | everything pci related is only supproted on linux today | |
| 14:56:47 | gibi | which is an acceptable limitation but the external depenency (sysfs) is not abstracted out | |
| 14:56:58 | gibi | and heavily tangled in | |
| 14:57:10 | sean-k-mooney | yes it is | |
| 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 | |