Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-07-23
17:46:38 efried mriedem: Yeah, no doubt it's a useful test, I just wasn't sure if it needed to block artom's patch, which is closer to mergeable.
17:46:38 mriedem i have heard "oh for pete's sakes" from this old woman in a hospital waiting room at least a few times
17:46:44 mriedem and "thank you kindly" from an old man
17:46:51 mriedem the muzak, it's.....killing me
17:47:16 mriedem efried: it's not needed to block his patch, he rebased it though
17:47:19 mriedem i'm completely innocent
17:50:55 jamesdenton hey sean-k-mooney - if you get a chance, can you take a peek at https://bugs.launchpad.net/os-vif/+bug/1837252?
17:50:56 openstack Launchpad bug 1837252 in neutron "IFLA_BR_AGEING_TIME of 0 causes flooding across bridges" [Undecided,Incomplete]
17:52:07 artom efried, yeah, I don't mind being on top, it keeps me honest
17:52:22 efried ight, np
17:52:33 artom efried, that being said, if we can use that as an excuse to marshal attention to mriedem's patch, I'd be cool with that ;)
17:52:57 efried that has more or less happened, viz. this conversation.
17:53:05 artom #winning
17:59:03 sean-k-mooney jamesdenton: yes ill take a look now just finishing a patch
17:59:15 jamesdenton great, thank you!
18:00:45 sean-k-mooney jamesdenton: as far as i am aware the aging behavior is quite old. we recently ported it to use pyroute two to set it instead of brctl but i dont think we change the behaivor in stien
18:01:01 sean-k-mooney that said we can always make this configurable.
18:01:19 sean-k-mooney ill take a look at the code and see why we orginially set this
18:03:30 aspiers hey sean-k-mooney I think found an issue
18:04:06 jamesdenton configurable could be ok. creating a bridge manually with brctl results in a 300 sec aging timer. And looking at some older environments, this appears to be the case as well. thanks for looking
18:05:00 aspiers sean-k-mooney: fakelibvirt returns capabilities for other architectures like mips
18:05:11 aspiers sean-k-mooney: but your patch doesn't provide a default machine type for those
18:05:47 aspiers so we get a few None keys in dict returned by get_domain_capabilities
18:07:27 sean-k-mooney jamesdenton: it looks like it was previously commented out https://github.com/openstack/os-vif/blob/stable/ocata/vif_plug_linux_bridge/linux_net.py#L132
18:07:59 sean-k-mooney aspiers: i know that is not an issue
18:08:08 sean-k-mooney i have mips installed locally
18:08:39 sean-k-mooney i didnt add an example for mip to force it t use the dynamic version which is used for x86_64
18:09:15 sean-k-mooney aspiers: we really should not be provideing default machine types in nova IMO, instead we should leave libvirt choose the defualt
18:10:01 jamesdenton sean-k-mooney even as recently as Rocky, it looks like. undocumented comment. love it.
18:10:46 sean-k-mooney ya so this looks like it was an unitntional change. ill see if there is a commit that warrented it.
18:11:18 sean-k-mooney jamesdenton: i know that shaid wanted to set it to 0 for a live migration edge cases at one point
18:11:35 sean-k-mooney that might be valid for ovs hybrid plug
18:11:43 sean-k-mooney but for linux bridge
18:11:55 sean-k-mooney i think we should rever back to the default
18:12:02 jamesdenton agreed.
18:15:08 jamesdenton sean-k-mooney Using common code now? See OVS https://github.com/openstack/os-vif/blob/stable/rocky/vif_plug_ovs/linux_net.py#L180 vs LXB https://github.com/openstack/os-vif/blob/stable/rocky/vif_plug_linux_bridge/linux_net.py#L148
18:15:08 sean-k-mooney jamesdenton: this was the patch that changed it originally https://github.com/openstack/os-vif/commit/fa4ff64b86e6e1b6399f7250eadbee9775c22d32#diff-f55bc78ffb4c10000bbf81b88bf68673
18:16:23 sean-k-mooney ya we are using common code now and i did ovs first so i guess we ended up with the ovs default
18:16:37 jamesdenton makes sense
18:16:45 sean-k-mooney ok ill go fix this shortly and we can get this backported
18:16:50 jamesdenton thank you
18:17:05 sean-k-mooney thanks for reporting it. ill update the bug
18:17:10 jamesdenton right on
18:17:27 aspiers sean-k-mooney: which bit of libvirt provides defaults? caps or domcaps or something else?
18:18:02 aspiers sean-k-mooney: I guess /domainCapabilities/machine ?
18:18:36 aspiers oh no, that's dependent on the API parameter
18:19:20 aspiers although I now remember that the CLI supported a default and the API didn't which was a bug they fixed after I reported it
18:19:26 aspiers amazing how much I forgot in a month :-/
18:22:13 sean-k-mooney aspiers: ya so how i have re written it we now pass none to the libvirt python bindings and libvirt interally select a machine type and we parse it from the result
18:22:28 sean-k-mooney aspiers: i can add mips and other example if you like
18:22:51 sean-k-mooney its just another contstant in the fake data file
18:24:06 sean-k-mooney aspiers: i personally think however that we might want to allow passing the arch and or machine type to the funtion have it lookup the value form libvirt if its not already cached
18:24:35 sean-k-mooney but i currently dont have a usecase where that is strictly needed
18:24:50 sean-k-mooney which is why i havent changed it
18:28:41 mriedem efried: i saw your questions on https://review.opendev.org/#/c/665138/
18:28:59 mriedem i put a bunch of notes on it - i think it's correct for a narrow window, but i'd like to see a functional test as well
18:29:12 mriedem it's basically https://review.opendev.org/#/c/641806/ but revert instead of confirm
18:35:58 efried mriedem: did you see the func test I tried to write?
18:36:22 efried https://review.opendev.org/#/c/665253/
18:36:53 efried which I suspect was totally stolen from the one you mention
18:38:32 mriedem no i didn't
18:39:08 mriedem might want to compare your test to what my scenario is in the commit message on the other patch
18:55:39 aspiers sean-k-mooney: I finally found it https://www.mail-archive.com/libvir-list@redhat.com/msg182746.html
18:55:50 aspiers https://github.com/libvirt/libvirt-python/commit/5004b4ad532ffa93ed4b62c8021eaa9210855209
18:58:09 sean-k-mooney aspiers: ah ok so we need to libvirt-python version to be bumped to 5.3
18:58:34 aspiers sean-k-mooney: http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-04-26.log.html#t2019-04-26T11:30:18
18:59:17 sean-k-mooney aspiers: ya that vaguely rings a bell
18:59:25 aspiers sean-k-mooney: IIUC the workaround was to explicitly pass None to the API
18:59:44 aspiers We don't want to introduce a dependency on newer libvirt-python unless we really have to
19:00:14 sean-k-mooney if explctily passing none works then we dont need too
19:00:25 sean-k-mooney that is what we are doing in my current patch
19:00:27 aspiers right
19:00:45 aspiers but still the nested dict gets broken
19:01:42 sean-k-mooney how so?
19:02:02 sean-k-mooney i never allow a key or value to be none in the current code
19:02:08 aspiers 'ppc': {'None': <nova.virt.libvirt.config.LibvirtConfigDomainCaps object at 0x7f243d3ec7b8>}
19:02:19 aspiers I see entries like that in the domcaps dict
19:02:24 aspiers using your patch
19:02:27 sean-k-mooney that should not happen maybe i missed something
19:02:48 aspiers there is no guarantee that machine_type is not None
19:02:55 aspiers since you removed the "or 'q35'"
19:03:07 sean-k-mooney i was depending on qemu not returing none
19:03:33 sean-k-mooney we cant just default to q35 but we can register default for as many acrhs as we care to support
19:03:33 aspiers where is it supposed to be getting the default from?
19:04:03 aspiers it's currently None for mips, mipsel, and ppc
19:04:56 sean-k-mooney https://review.opendev.org/#/c/670189/4/nova/virt/libvirt/host.py@763
19:05:09 sean-k-mooney we use the value we got back from libvirt as the key
19:05:20 aspiers back from which libvirt call?
19:05:34 sean-k-mooney getDomainCapabilities
19:05:35 aspiers getCapabilities or getDomainCapabilities?
19:06:16 aspiers OK I'll try to track it down
19:06:21 sean-k-mooney so if we dont have a defualt for the arch we call getDomainCapabilities with machine_type=None
19:07:28 aspiers right and then look at the machine type which was returned in the XML
19:07:33 sean-k-mooney then i added https://review.opendev.org/#/c/670189/4/nova/virt/libvirt/config.py@132 to parse the machine type and store it in the object
19:08:08 sean-k-mooney ya so i was assuming that there would aways be a machine type returned in the xml
19:08:21 aspiers I think there is
19:08:40 sean-k-mooney Maybe we are getting the default from https://review.opendev.org/#/c/670189/4/nova/virt/libvirt/config.py@121
19:09:24 sean-k-mooney that would cause it to be none if we did not get a machine type in the xml
19:09:40 sean-k-mooney i could just skip storing it if we get none back
19:10:18 aspiers yeah but I'm not yet convinced that's the issue

Earlier   Later