| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-10-15 | |||
| 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 | |
| 17:36:09 | sean-k-mooney | which does https://github.com/openstack/nova/blob/2c3ff1004e8812da0bc4e5502762e6b9a7650846/nova/pci/devspec.py#L238 | |
| 17:37:07 | sean-k-mooney | whic eventually expodes | |
| 17:37:51 | sean-k-mooney | it actully expoding here https://github.com/openstack/nova/blob/2c3ff1004e8812da0bc4e5502762e6b9a7650846/nova/pci/devspec.py#L237 | |
| 17:38:20 | sean-k-mooney | when we contuct the dev object to compare agaisnt form teh adress we get form libvirt | |
| 17:38:55 | sean-k-mooney | that why its expoliding in init | |
| 17:39:21 | sean-k-mooney | we constuct the object form the string to reuse the parsing that was written for teh whitelist | |
| 17:39:30 | sean-k-mooney | so we dont have to have it twice | |
| 17:40:13 | melwitt | is pci_phys_addr the whitelist entry? | |
| 17:41:24 | sean-k-mooney | no that is the pci adress form libvirt | |
| 17:41:39 | sean-k-mooney | self.pci_address_spec is the object form the whitelist | |
| 17:41:47 | sean-k-mooney | arent the names great | |
| 17:41:57 | sean-k-mooney | so easy to follow and not confusing at all | |
| 17:42:04 | melwitt | /o\ | |
| 17:43:01 | sean-k-mooney | melwitt: its not taking this branch https://github.com/openstack/nova/blob/2c3ff1004e8812da0bc4e5502762e6b9a7650846/nova/pci/devspec.py#L231-L234 | |
| 17:43:10 | sean-k-mooney | but it would explode if it did | |
| 17:43:31 | melwitt | well, what I mean is, IIUC (which I probably don't) one of those construct of PhysicalPciAddress is "correctly" parsing a whitelist entry and the other one is parsing an address from libvirt. is my understanding wrong? | |
| 17:43:34 | sean-k-mooney | that is for pci device that are capable fo sriov | |
| 17:43:46 | melwitt | oh, ok. nevermind my question then | |
| 17:44:09 | sean-k-mooney | for PF if you list a PF we allow all the VF if they are allocated to be used | |
| 17:44:15 | sean-k-mooney | that is what that branch is for | |
| 17:44:38 | sean-k-mooney | so if whitelist a PF its a shortcut to whitelist the VFs | |
| 17:44:55 | melwitt | that makes sense | |
| 17:46:59 | sean-k-mooney | anyway the tl;dr for gibi is we shoudl catch the excpetion here https://github.com/openstack/nova/blob/2c3ff1004e8812da0bc4e5502762e6b9a7650846/nova/pci/whitelist.py#L85-L87 instead | |
| 17:47:18 | sean-k-mooney | to that if should be in a try | |
| 17:48:00 | sean-k-mooney | and we retrun fause if we get the PciConfigInvalidWhitelist excption | |
| 17:48:15 | sean-k-mooney | whcih is what he is doing here https://review.opendev.org/#/c/756697/4/nova/pci/manager.py | |
| 17:48:54 | sean-k-mooney | back to your orginal question | |
| 17:48:57 | sean-k-mooney | https://review.opendev.org/#/c/756697/4/nova/pci/devspec.py | |
| 17:49:18 | sean-k-mooney | is still going to raise and excption if you input an invalid whitelist | |