| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-19 | |||
| 21:02:10 | mriedem | sean-k-mooney2: we could, but then anyone using ODL is going to wait up to 5 minutes for an event that's not going to come | |
| 21:02:50 | mriedem | _ix: you could try asking in #openstack-rpm-packaging | |
| 21:03:03 | sean-k-mooney2 | mriedem: ill double check the odl code. ill be back in the office tomorrow so i can try and set up an odl environment | |
| 21:03:13 | mriedem | _ix: or maybe #rdo | |
| 21:03:34 | mriedem | sean-k-mooney2: ok for now i'm just going to neuter this lifecycle event callback code that calls migrate_instance_finish | |
| 21:05:02 | sean-k-mooney2 | well we can still activate the dest binding when we get the hyperviors event that it just paused the source and is about to unpause the dest right | |
| 21:05:20 | sean-k-mooney2 | we just dont wait for the vif plugged event | |
| 21:05:52 | mriedem | two separate issues, | |
| 21:06:11 | mriedem | i'm going to neuter the first right now because it's causing this vif unbound explosion in _post_live_migrate | |
| 21:06:32 | mriedem | the latter issue is not something we're hitting right now, but was in the plan as part of this spec | |
| 21:06:52 | sean-k-mooney2 | ah ok | |
| 21:17:48 | openstackgerrit | Merged openstack/nova master: Move placement exceptions into the placement package https://review.openstack.org/549862 | |
| 21:17:51 | _ix | mriedem: Thanks for the tips. | |
| 21:27:28 | jaypipes | phew, my brain is melting today... | |
| 21:38:28 | cdent | was it the videophone jaypipes ? | |
| 21:40:14 | edleafe | efried: ugh - I had all those changes, and then rebased poorly after the original member_of patch merged. Fixing... | |
| 21:41:01 | efried | edleafe: Here to keep you honest. I feel your rebase pain, homey. | |
| 21:42:02 | edleafe | efried: heh, trying to keep things straight while half-paying attention to meetings | |
| 21:45:58 | cdent | efried: "I have looked at this." beyond that and the typos, do you think I'm on the right track? | |
| 21:46:30 | efried | cdent: The unedited version was something like, "I have looked at this. I have no idea what I'm looking at, so I am abstaining from voting." | |
| 21:46:48 | cdent | ah, that makes a bit more sense | |
| 21:47:19 | efried | cdent: "...and the time it would take me to figure out what I'm looking at doesn't fit in my current budget (still ploughing through vacation backlog)." | |
| 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 | |