| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-10-15 | |||
| 15:42:53 | lyarwood | stephenfin: did you find any issues with _sync_instance_power_state running *after* an instance had already paused? | |
| 15:49:46 | artom | lyarwood, I think he's on PTO this EU afernoon - he cancelled a meeting we had with Brian and Erwan | |
| 15:49:56 | lyarwood | ah forgot my bad | |
| 15:49:59 | lyarwood | thanks artom | |
| 15:51:07 | gibi | nova meeting starts in 10 minutes on #openstack-meeting-3 | |
| 16:13:48 | dansmith | melwitt: ahh, maybe the separate joins are only happening when we grab multiple instances | |
| 16:14:12 | dansmith | if that's the case, then just making the get-by-uuid path work the same would be easy | |
| 16:15:23 | melwitt | is there a specific part of the code you're looking at that makes you think that? | |
| 16:15:37 | dansmith | I shouldn't say "separate joins"... they're called "manual joins" in the db api, because they're separate queries | |
| 16:15:56 | dansmith | melwitt: look at this: https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L1278 | |
| 16:16:05 | dansmith | melwitt: and this: https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L1328 | |
| 16:16:25 | melwitt | ahh | |
| 16:17:13 | dansmith | when we moved flavor info into sysmeta, we caused a giant explosion on every instance list, right around icehouse, | |
| 16:17:41 | dansmith | so we moved to this manual join method where we query the metadata tables separately, but in bulk, to avoid the n:m join | |
| 16:18:11 | dansmith | so the single case is similar, just where n=1, but if m is large enough, still unideal | |
| 16:18:16 | sean-k-mooney | we basicaly need to use a subqurry for the userdata | |
| 16:18:29 | sean-k-mooney | and get the rest of the data normally | |
| 16:19:02 | dansmith | subquery or separate query? not sure how the former will help | |
| 16:19:23 | sean-k-mooney | seperate query | |
| 16:19:36 | dansmith | yeah, we do that on list, just don't on get, that's my point | |
| 16:19:48 | sean-k-mooney | yeah | |
| 16:20:12 | dansmith | all the machinery is there from list, so get can just call it but with the single instance we care about | |
| 16:20:18 | sean-k-mooney | i didnt look a the code specificaly but this sounded familar as i think we already optimised this once before for that list case | |
| 16:21:01 | sean-k-mooney | its just one of those things that works fine for small number and then explode because its n*m complexity | |
| 16:21:10 | sean-k-mooney | in size | |
| 16:21:11 | dansmith | for list, yeah | |
| 16:21:31 | sean-k-mooney | well in this case its joing userdata with each metadata propertiy | |
| 16:21:37 | dansmith | for single get n=1, but m is large | |
| 16:21:54 | dansmith | right | |
| 16:22:04 | sean-k-mooney | ya so its still liniar instad of O(1) | |
| 16:22:16 | sean-k-mooney | not as bad but still kind of sucks | |
| 16:22:24 | dansmith | linearly big is still big :) | |
| 16:30:32 | openstackgerrit | melanie witt proposed openstack/nova stable/ussuri: Follow up for cherry-pick check for merge patch https://review.opendev.org/758463 | |
| 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 | |