| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-10-15 | |||
| 16:43:25 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Ignore PCI devices with 32bit domain https://review.opendev.org/756697 | |
| 16:45:37 | sean-k-mooney | gibi: i have seen devices that use teh 32bit domian that can be assigned to qemu | |
| 16:45:56 | sean-k-mooney | althoguh i was using a patched qemu | |
| 16:46:31 | gibi | sean-k-mooney: interesting. Could you check if the patch included support for 32 bit domain? there is a link in the commit message for the qemu code | |
| 16:46:54 | sean-k-mooney | it was an internal intel build for unrelated hardware | |
| 16:46:59 | gibi | melwitt: improved https://review.opendev.org/756697 | |
| 16:46:59 | sean-k-mooney | it was never upstreamed :) | |
| 16:47:23 | sean-k-mooney | gibi: looking at your patch i think its fine but it woudl be nice to add some docs for this limitation somewhere | |
| 16:47:40 | sean-k-mooney | maybe in the pci whitelist docs somewhere | |
| 16:47:53 | gibi | sean-k-mooney: we already limit the whitelist to 16 bit | |
| 16:47:57 | melwitt | gibi: thanks, will look | |
| 16:48:12 | sean-k-mooney | gibi: but do we document that limitation or just do it in code | |
| 16:49:08 | gibi | sean-k-mooney: good point, we actually not https://docs.openstack.org/nova/latest/configuration/config.html#pci.passthrough_whitelist | |
| 16:49:20 | gibi | I will add extra information to the config description | |
| 16:49:52 | sean-k-mooney | ya we jsut say "PCI address of the device. Both traditional glob style and regular expression syntax is supported." | |
| 16:50:07 | sean-k-mooney | if we can add a note that we only supprot 16 bit domains ill be happy | |
| 16:50:40 | gibi | sean-k-mooney: I will do | |
| 16:51:03 | gibi | but not today. As it is beer time for me now | |
| 16:51:11 | gibi | see you all tomorrow | |
| 16:51:14 | gibi | o/ | |
| 16:51:23 | sean-k-mooney | gibi: redhatters will be off tomorrow | |
| 16:51:29 | sean-k-mooney | its a company day | |
| 16:51:34 | sean-k-mooney | just an fyi | |
| 16:52:32 | gibi | sean-k-mooney: thanks for the heads up, enjoy your free day | |
| 16:53:11 | sean-k-mooney | ill try to not spend it working on ci :) | |
| 16:55:28 | melwitt | sean-k-mooney: am I missing something or does that make all invalid configs ignored? maybe I just have a wrong understanding | |
| 16:56:16 | sean-k-mooney | i have not done a full review but ill take a look now | |
| 16:57:26 | sean-k-mooney | your wondering about https://review.opendev.org/#/c/756697/4/nova/pci/devspec.py | |
| 16:57:48 | sean-k-mooney | the transfromataion of PciConfigInvalidWhitelist to PciInvalidDevice | |
| 16:58:55 | sean-k-mooney | this change actully does not take into account the glob "*:whatere" adresses ro the regex support | |
| 16:59:31 | sean-k-mooney | self.domain = pci_addr.get('domain', REGEX_ANY) also does not truncate to just 16 bits today | |
| 16:59:32 | melwitt | I don't know this parsing code really, but I was just wondering would this mask all/most actual invalid configs | |
| 17:01:28 | sean-k-mooney | so its cactaching PciConfigInvalidWhitelist | |
| 17:01:34 | sean-k-mooney | which is only raised here https://github.com/openstack/nova/blob/master/nova/pci/devspec.py#L51-L66 | |
| 17:01:51 | sean-k-mooney | but it would catch any case where the value exceed the max | |
| 17:01:58 | sean-k-mooney | or where it was invalid | |
| 17:02:20 | melwitt | yeah, that's where I'm wondering is there a way to tell when it's a 32bit domain parse fail? so that we could raise something different? | |
| 17:02:48 | sean-k-mooney | we can look at teh data we parsed | |
| 17:03:05 | melwitt | I see | |
| 17:03:10 | sean-k-mooney | we could also split the excptions | |
| 17:03:15 | melwitt | kinda like the "if v > maxval" | |
| 17:03:34 | sean-k-mooney | ya so https://github.com/openstack/nova/blob/master/nova/pci/devspec.py#L57-L66 | |
| 17:03:44 | sean-k-mooney | this is the two cases where its currently raised | |
| 17:04:03 | sean-k-mooney | but we can have the maxval branc raise a subclass of PciConfigInvalidWhitelist | |
| 17:04:14 | sean-k-mooney | and then catch that | |
| 17:04:24 | melwitt | yeah, so we are currently hitting the "if v > maxval" condition | |
| 17:04:26 | sean-k-mooney | e.g. PciConfigInvalidWhitelistMaxExceeded | |
| 17:05:05 | sean-k-mooney | yes so we want to ignore those devices | |
| 17:05:28 | sean-k-mooney | the other case is shwere we get non hex adresses | |
| 17:05:32 | melwitt | so we want to ignore all that are > maxval | |
| 17:05:34 | sean-k-mooney | i guess we coudl ignore those too | |
| 17:05:42 | sean-k-mooney | these are comming from libvirt | |
| 17:05:56 | sean-k-mooney | well actully no theey are not | |
| 17:06:13 | sean-k-mooney | its the whitelist at this point althogh some of this code is common | |
| 17:07:15 | melwitt | ok, that's where I was confused was, I was assuming we only wanted to ignore the ones > maxval. do we want to ignore non hex addresses? just saying that this patch seems to just make the validations all ignored. is there are reason to keep the validations? | |
| 17:11:11 | sean-k-mooney | am well we should not allow you to whitelist device that are invliad | |
| 17:11:45 | sean-k-mooney | i need to look at the bug | |
| 17:13:37 | melwitt | ok. I'll try to write more comments | |
| 17:14:14 | sean-k-mooney | so i think the issue is that we sue thise function to filter the pci deivce returned by libvirt | |
| 17:14:44 | sean-k-mooney | so even if you dont have whitelist value that is 32bits it will explode when we try to filter the value | |
| 17:15:05 | sean-k-mooney | we do nova.pci.devspec.WhitelistPciAddress.match() | |
| 17:16:37 | sean-k-mooney | we proably dont want to catch the exption where gibi is | |
| 17:16:56 | sean-k-mooney | instead we proably want to only catch it and skip the device wehre we do the match | |
| 17:18:01 | melwitt | ok, so this issue is NOT to do with the whitelist at all. I see | |
| 17:18:29 | melwitt | so the whitelist parsing code is reused to parse the devices returned by libvirt | |
| 17:18:59 | melwitt | ok, I was missing that | |
| 17:19:17 | sean-k-mooney | so here https://github.com/openstack/nova/blob/master/nova/pci/manager.py#L118-L122 | |
| 17:20:03 | sean-k-mooney | yep we reuse https://github.com/openstack/nova/blob/2c3ff1004e8812da0bc4e5502762e6b9a7650846/nova/pci/whitelist.py#L80-L88 | |
| 17:20:05 | melwitt | ok, this way we're sure we're not parsing a whitelist | |
| 17:20:54 | sean-k-mooney | so what we do is we build a device spec object for every entry in the whitelist | |
| 17:21:16 | sean-k-mooney | then for every device returned by the vert driver we constuct a second one and see if tey match | |
| 17:21:28 | sean-k-mooney | its the libvirt address that is 32bit | |
| 17:22:16 | melwitt | but the exception guessed raised in __init__ not in the match() call? | |
| 17:22:30 | melwitt | *is | |
| 17:23:07 | sean-k-mooney | from here https://github.com/openstack/nova/blob/2c3ff1004e8812da0bc4e5502762e6b9a7650846/nova/pci/devspec.py#L217-L238 | |
| 17:23:28 | melwitt | ohhhh | |
| 17:23:30 | melwitt | guh | |
| 17:23:43 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: Fix the vGPU dynamic options race https://review.opendev.org/758470 | |
| 17:24:44 | melwitt | so devices_json is a list of whitelist pci objects | |
| 17:26:01 | melwitt | maybe not... I'll look. this is so confusing to me | |
| 17:26:34 | sean-k-mooney | its a json blob we construct for the data we get from libvirt | |
| 17:26:43 | sean-k-mooney | the nodedev xmls | |
| 17:28:02 | sean-k-mooney | it looks like a list of these https://github.com/openstack/nova/blob/131f7606c179bd08bde3b7ae0e6bb0b59acf1545/nova/tests/unit/pci/test_manager.py#L32-L41 | |
| 17:28:07 | melwitt | yeah, just that device_assignable is from the Whitelist class | |
| 17:29:40 | sean-k-mooney | i have to look this up everythim but it generated here https://opendev.org/openstack/nova/src/branch/master/nova/virt/libvirt/driver.py#L7127-L7150 | |
| 17:30:42 | sean-k-mooney | i rewrote this recently | |
| 17:31:02 | sean-k-mooney | but we get all the nodedev form libvirt that are pci device ro nics | |
| 17:31:08 | melwitt | thanks | |
| 17:31:55 | melwitt | oh it's the dev_filter that's the whitelist, facepalm. sorry | |
| 17:32:20 | melwitt | I have gone through this before too, but still mess it up | |
| 17:33:07 | sean-k-mooney | dev_filter is an instance of the Whitelist class yes | |
| 17:33:47 | sean-k-mooney | whic has one filed specs which holds all the whitelist entries | |
| 17:34:02 | sean-k-mooney | then we just do | |
| 17:34:04 | sean-k-mooney | for spec in self.specs: | |
| 17:34:06 | sean-k-mooney | if spec.match(dev): | |
| 17:34:08 | sean-k-mooney | return True | |
| 17:34:10 | sean-k-mooney | return False | |
| 17:34:33 | sean-k-mooney | so if any of the entries match the json seriasition of the pci device form the virt driver w return true | |
| 17:35:01 | melwitt | ok | |
| 17:35:39 | sean-k-mooney | that mach is the on defiend in WhitelistPciAddress | |