| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-23 | |||
| 16:59:04 | efried | right, so like, fixing that bug is the whole reason this patch exists. So if we're leaving that bug open -- even if we're drastically reducing the probability -- we need to own that somehow. | |
| 17:05:41 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Remove super old unnecessary TODO from API start() method https://review.opendev.org/672330 | |
| 17:32:49 | sean-k-mooney | lyarwood: mriedem: can ye add this depercation warning backport to your review queue https://review.opendev.org/#/c/671532/ | |
| 17:39:48 | mriedem | sean-k-mooney: done | |
| 17:40:31 | mriedem | note that issue is also in cinder | |
| 17:40:34 | mriedem | i'll add cinder to the bug | |
| 17:42:52 | efried | artom, mriedem: Why is https://review.opendev.org/#/c/668631/ (pass migration to finish_revert_migration) on top of https://review.opendev.org/#/c/670393/ (func test for crash of same)? | |
| 17:42:52 | efried | Are we making sure the former doesn't change any of the (broken) behavior demonstrated by the latter? | |
| 17:42:58 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: Rename 'nova.objects.instance_numa_topology' https://review.opendev.org/671789 | |
| 17:42:58 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Remove unnecessary try-catch around 'getCPUMap' https://review.opendev.org/671790 | |
| 17:42:59 | openstackgerrit | Stephen Finucane proposed openstack/nova master: objects: Remove legacy '_from_dict' functions https://review.opendev.org/537414 | |
| 17:43:00 | openstackgerrit | Stephen Finucane proposed openstack/nova master: claims: Remove useless caching https://review.opendev.org/671791 | |
| 17:43:00 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Add '[compute] cpu_dedicated_set' option https://review.opendev.org/671792 | |
| 17:43:01 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Start reporting PCPU inventory to placement https://review.opendev.org/671793 | |
| 17:43:02 | openstackgerrit | Stephen Finucane proposed openstack/nova master: trivial: Rename exception argument https://review.opendev.org/671795 | |
| 17:43:02 | openstackgerrit | Stephen Finucane proposed openstack/nova master: trivial: Remove unused function parameter https://review.opendev.org/671796 | |
| 17:43:03 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'hardware.get_host_numa_usage_from_instance' https://review.opendev.org/671797 | |
| 17:43:04 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'hardware.host_topology_and_format_from_host' https://review.opendev.org/671798 | |
| 17:43:04 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'hardware.instance_topology_from_instance' https://review.opendev.org/671799 | |
| 17:43:05 | openstackgerrit | Stephen Finucane proposed openstack/nova master: WIP: hardware: Differentiate between shared and dedicated CPUs https://review.opendev.org/671800 | |
| 17:43:05 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Add support translating CPU policy extra specs, image meta https://review.opendev.org/671801 | |
| 17:43:06 | openstackgerrit | Stephen Finucane proposed openstack/nova master: tests: Split NUMA object tests https://review.opendev.org/672336 | |
| 17:44:15 | mriedem | efried: they were written independenty and artom had a bug i pointed out in his change which prompted me to write the functional test since the unit tests were just asserting the broken mock call, | |
| 17:44:27 | mriedem | he rebased on top of the functional test to assert that it would hit the issue in his patch, | |
| 17:44:35 | efried | okay | |
| 17:44:50 | mriedem | regardless of his patch, i think it's a useful functional test for that init host restart code since we don't test that stuff in gate runs and unit tests for that kind of code kind of suck.... | |
| 17:45:32 | mriedem | as you see from the test we leak allocations and things in the nova db etc | |
| 17:45:50 | aspiers | kashyap: I guess you're not around? | |
| 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 | |