Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-19
21:47:42 cdent if/when you surface from that and you want a tour, let me know
21:48:11 cdent thanks for looking in any case, I'm always glad to have your proofing
22:08:13 edleafe efried: let
22:08:17 edleafe oops
22:08:23 openstackgerrit Ed Leafe proposed openstack/nova master: Address issues raised in adding member_of to GET /a-c https://review.openstack.org/554357
22:08:32 edleafe efried: let's try this again ^^
22:08:55 efried ack
22:09:40 jaypipes cdent: nah, trying to fix the nested providers alloc candidates stuff
22:19:08 efried mriedem: 1.19 was for bp placement-aggregate-generation. Gerrit likes to overwrite the topic for all patches when submitting a series, which is how 1.19 ended up in the whiteboard for https://blueprints.launchpad.net/nova/+spec/generation-from-create-provider. FTFY.
22:22:38 efried mriedem: You can mark https://blueprints.launchpad.net/nova/+spec/placement-aggregate-generation done too.
22:22:41 mriedem melwitt: totally random but i was just doing some blame game and came across https://review.openstack.org/#/c/377093/1/nova/rpc.py
22:22:55 mriedem melwitt: ever noticed that RequestContext(overwrite) kwarg is not used at all in that patch?
22:23:11 mriedem https://review.openstack.org/#/c/377093/1/nova/context.py@72
22:24:19 mriedem efried: done
22:24:25 efried thx
22:25:31 melwitt mriedem: yeah, it's used in the base context class from oslo https://github.com/openstack/oslo.context/blob/a8d86df/oslo_context/context.py#L225
22:26:22 mriedem oh i see
22:26:41 melwitt it's kind of unclear though, being lumped into **kwargs like that
22:26:47 mriedem have been trying to figure out wtf periodic tasks are running with a request id which is the same request id i'm tracking for an instance create operation
22:27:41 melwitt ah, okay. I'd like to know how that happens too
22:27:58 mriedem well, when the periodics run, they call get_admin_context
22:28:05 mriedem which doesn't overwrite the thread local context
22:28:16 mriedem so i guess that's why
22:28:20 melwitt oh, so they just get whatever was there. yeah
22:28:38 mriedem https://github.com/openstack/nova/blob/master/nova/service.py#L295
22:28:56 mriedem which are run from this thread group timer https://github.com/openstack/nova/blob/master/nova/service.py#L213
22:29:19 mriedem but it makes tracing a failed operation for a specific instance really frustrating if everything else picks up the thread local version
22:29:23 melwitt right
22:30:14 melwitt I have this old patch up around generating an admin context up front and storing it on the service for use in periodic tasks (to help us get to configuring CellDatabases differently). I'm not sure if it would do any better than the current situation though https://review.openstack.org/#/c/524306
22:30:45 melwitt like, I'm not sure if that initial get_admin_context in the service creation might also get tied to some unhelpful request id
22:31:09 melwitt er, service.start() I mean
22:32:22 openstackgerrit Matt Riedemann proposed openstack/nova master: Add VIFMigrateData object for live migration https://review.openstack.org/515423
22:32:23 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: libvirt: use dest host vif migrate details for live migration https://review.openstack.org/551370
22:32:23 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Port binding based on events during live migration https://review.openstack.org/434870
22:32:24 openstackgerrit Matt Riedemann proposed openstack/nova master: Add "delete_port_binding" network API method https://review.openstack.org/552170
22:32:24 openstackgerrit Matt Riedemann proposed openstack/nova master: compute: use port binding extended API during live migration https://review.openstack.org/551371
22:32:25 openstackgerrit Matt Riedemann proposed openstack/nova master: conductor: use port binding extended API in during live migrate https://review.openstack.org/522537
22:32:25 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Turn on new port binding extended live migrate flow https://review.openstack.org/552173
22:33:36 mriedem it shouldn't if we're not calling get_admin_context() every time the periodic runs
22:34:36 mriedem i was going to try setting the request_id='req-%s' % self.service_ref.uuid so we'd have a predictable request id in periodics, but that would just be ignored since get_admin_context uses overwrite=False
22:35:08 melwitt okay, then that patch might help then. it got stalled on me not providing a reliable poison fixture for making sure nothing in periodics tries to get_admin_context on their own. and gibi proposed something better around that. but I haven't gotten back to it
22:49:14 dansmith mriedem: I'm still not sure how that makes sense,
22:49:25 dansmith the periodics are their own (eventlet) thread, they should have their own TLS
22:49:53 dansmith not sure why they shouldn't be able to set their TLS there
22:50:17 dansmith I'd guess that overwrite=False is actually for the case where some user operation needs an admin context to do something specific, and you don't want to leave them with admin when you're done
22:50:30 dansmith but periodics probably *should* overwrite TLS in that case
22:51:16 dansmith I would also expect TLS to be reset on each spawn/switch of a thread, otherwise you leak things between requests until you set TLS _to_ something
22:51:38 mriedem i assume overwrite=False was added to get_admin_context() for a wholly different reason than why we're using get_admin_context() for periodics
22:52:04 dansmith that's what I'm saing
22:52:16 dansmith for a localized "okay, do this thing for the user as admin"
22:52:23 dansmith looking up a fixedip before assignment, etc
22:52:34 dansmith that's why I'm saying we probably want overwrite=True for periodics
22:53:06 dansmith that kinda goes against the original supposition of melwitt's patch to client RPC though
22:53:11 melwitt here's the original bug that led to using overwrite=False https://bugs.launchpad.net/nova/+bug/1627838
22:53:12 openstack Launchpad bug 1627838 in OpenStack Compute (nova) newton "Context is reset to by ClientRouter in nova/rpc.py" [High,Fix committed] - Assigned to Matt Riedemann (mriedem)
22:54:59 dansmith hmm, I don't get that
23:00:07 melwitt yeah, I guess that was a different thing. normally we run periodics with get_admin_context which does an overwrite=False and when I added a different unrelated periodic I didn't use get_admin_context and inadvertently made it overwrite=True without realizing it
23:01:26 melwitt and the reported problem was that the periodic was blowing away other requests auth token in-flight
23:01:57 dansmith although ClientRouter does schedule periodics quite differently from service it seems
23:03:28 melwitt yeah, maybe there is something wrong with how it's running periodics?
23:03:36 dansmith well,
23:03:59 dansmith I'm wondering if that's why it needed overwrite=False, but our periodics in compute manager need overwrite=True
23:04:11 melwitt oh, I see
23:04:24 dansmith melwitt: that thing needed periodics to age out clients but that code seems gone I think, so can we remove thatl ine anyway?
23:04:40 melwitt yeah, I was thinking that too
23:06:18 dansmith so we're running periodics in our managers via threadgroup timers, which I assume run in threads of their own, but I should check
23:06:52 dansmith mriedem: yeah, so that _remove_stale_clients() periodic was removed later, so we should nuke that line
23:06:56 dansmith er, melwitt ^
23:07:25 melwitt ack. I can propose that
23:10:12 openstackgerrit Matt Riedemann proposed openstack/nova master: Remove RequestContext.instance_lock_checked https://review.openstack.org/554378
23:10:13 mriedem more cleanup while i was poking around ^
23:11:14 dansmith so I'm not exactly seeing how the way periodics are used in ClientRouter would have worked, other than the single go of them run via __init__
23:11:24 dansmith but my guess is that they're not run the way we run them in our service
23:11:50 dansmith melwitt: do we know for sure that those periodics ever ran more than once?
23:11:56 dansmith that code wasn't in the tree for very long, so..
23:12:52 dansmith it sure looks like we just fired them off once during __init_ and never ran them again
23:13:25 dansmith which is also puzzling as to why that would be a problem needing to be reported, but the bug doesn't have much info
23:13:31 melwitt dansmith: maybe not. ClientRouter inherits from oslo PeriodicTasks, so it would do this https://github.com/openstack/oslo.service/blob/master/oslo_service/periodic_task.py#L193
23:13:59 dansmith melwitt: well, it does that once when you run it in __init__
23:14:00 dansmith but I don't see where that gets scheduled any other place
23:14:02 dansmith service.py will do it, but you're not a service there
23:16:37 dansmith and the tests just fire the handler manually
23:16:49 melwitt hm
23:16:50 dansmith (as expected, we don't really have good coverage over periodics)
23:17:43 dansmith https://www.youtube.com/watch?v=C9MG9b4mEU0
23:18:29 melwitt lol, great movie
23:18:53 dansmith the only thing I can think of,
23:19:07 dansmith would be that we instantiate a new compute rpc class for certain things at runtime,
23:19:20 dansmith and maybe we blow away whatever context was current at that time,
23:19:46 dansmith because anything running ComputeRPCAPI.__init() will run ClientRouter.__init__(), which will instantiate a context with overwrite=True before your patch
23:20:13 dansmith er, rpcapi.ComputeAPI() I mean
23:20:56 melwitt yeah :\
23:22:33 dansmith mriedem: so I think maybe chatting with someone like dhellmann might be a good idea, but I kinda think we should be running with overwrite=True on those periodics' context
23:24:09 dansmith I'd also like someone to tell me why TLS isn't cleared when a new thread is spawned and/or would like to confirm that timers as we use them to start periodics in our services are actually running in their own threads and not as if they were signal handlers
23:26:38 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix message for unexpected external event https://review.openstack.org/554380
23:27:18 mriedem ok, but not tonight
23:31:46 openstackgerrit melanie witt proposed openstack/nova master: Remove useless run_periodic_tasks call in ClientRouter https://review.openstack.org/554381
23:34:02 melwitt dansmith, mriedem ^ I left the related overwrite=False regression test case alone thinking we would address that when we get answers about what we should be doing with overwrite + periodics later
23:35:22 dansmith melwitt: hmm, I'm not sure what that test is testing TBH
23:35:35 dansmith I'm pretty fried and well past punkin time, so I'll look tomorrow with a fresh head

Earlier   Later