| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-23 | |||
| 16:23:08 | dansmith | but had a "don't actually call the virt driver=True" if we're ironic | |
| 16:23:28 | dansmith | so the change to delgate "what should I do with this external change?" to the driver was to make that cleaner | |
| 16:23:33 | mriedem | yeah i agree that was gross | |
| 16:23:46 | dansmith | didn't we discuss all this during spec review? | |
| 16:23:50 | dansmith | I thought you were involved with that | |
| 16:23:54 | mriedem | not really | |
| 16:24:07 | mriedem | that's why i asked earlier in here if the spec was going to be updated so i could sanely read it before reviewing the code | |
| 16:24:13 | mriedem | but i just dove in anyway | |
| 16:24:35 | dansmith | okay you did review it at one point, but I see not in the later revs | |
| 16:24:39 | mriedem | anyway, that's fine when you explain it that way | |
| 16:24:45 | dansmith | okay | |
| 16:45:37 | efried | mriedem, dansmith: what's your take on the race condition & lock? Add it to the code, or take it out of the spec? | |
| 16:47:07 | mriedem | lock.... | |
| 16:47:17 | mriedem | the api is validating the vm/task/power state right? | |
| 16:47:21 | mriedem | and the compute manager is making the change | |
| 16:47:28 | efried | mriedem: The concern I raised top-level, which was apparently also discussed in the spec. | |
| 16:47:58 | efried | that we could start the power sync before the event hits | |
| 16:48:51 | mriedem | if the sync task runs it would call api.stop() which would set the task_state and then the event should be ignored, | |
| 16:49:07 | mriedem | but there could be a window where we passed the event from the api to compute before compute calls api.stop | |
| 16:49:29 | mriedem | the task_state is generally our lock for stuff, i.e. the task state ignores instances with a non-None task_state | |
| 16:49:49 | mriedem | i'm not sure how you're going to impose a lock otherwise, unless you lock on the instance uuid in the compute manager or something | |
| 16:50:28 | mriedem | note that stop_instance in compute manager already locks on instance uuid | |
| 16:50:30 | efried | I'm okay accepting the risk, but it should probably be noted in a code comment. | |
| 16:54:38 | openstackgerrit | dzogchen proposed openstack/nova master: Nova: node should be deleted when nova-compute is removed https://review.opendev.org/671731 | |
| 16:56:00 | mriedem | efried: i left a comment in the code, | |
| 16:56:20 | mriedem | i *think* if we lose a race somewhere, chances are one of the threads is going to fail with UnexpectedTaskStateError | |
| 16:56:34 | efried | thanks | |
| 16:56:48 | mriedem | which is maybe ok - worst case is the node gets powered off when it shouldn't be, which is the original bug she's trying to fix right? | |
| 16:57:03 | mriedem | some lab monkey powered on the box when nova thinks it's off | |
| 16:57:11 | mriedem | so nova's task shuts it off | |
| 16:57:50 | mriedem | fwiw i think we also have races with the existing sync task and the libvirt lifecycle event callback stuff which is why we have a workaround option to ignore those | |
| 16:58:10 | mriedem | https://docs.openstack.org/nova/latest/configuration/config.html#workarounds.handle_virt_lifecycle_events | |
| 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 | |