Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-07-23
16:19:12 dansmith why we're plumbing this to the driver instead of just telling the compute about the change?
16:20:29 mriedem all the virt driver does is set the values on the instance and save them to the db,
16:20:38 mriedem which is what stop/start_instance in the compute manager does for normal stop/start,
16:20:52 mriedem so why not just do the db updates in the compute manager code rather than muck with driver plumbing
16:20:56 mriedem just seems weird to me,
16:21:08 mriedem if it's "we don't want non-ironic drivers to do this" then add a driver capability flag
16:21:12 dansmith mriedem: compute start/stop will call the driver and tell it what to do right?
16:21:35 mriedem to actually power on / off a guest yeah
16:21:37 mriedem this is just db muckery
16:21:45 dansmith the point of this is to decide what needs to happen when some external change happens to the instance, just like how libvirt gets direct events
16:22:00 dansmith the point being, ironic needs to know this should be swallowed and the db updated,
16:22:03 mriedem none of that logic is in the actual driver
16:22:10 dansmith but that's not necessarily what should happen for vmware
16:22:29 openstackgerrit Merged openstack/nova master: Update api-ref location https://review.opendev.org/672077
16:22:39 mriedem ok i guess "it's just a simple abstraction" is the answer then
16:22:57 dansmith remember the first version of this *did* just call start/top,
16:23:00 mriedem i don't expect any other drivers to ever implement this, but i guess it's generic enough that they could
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

Earlier   Later