| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-31 | |||
| 16:17:17 | cdent | wow, major jinx with melwitt there | |
| 16:17:42 | dansmith | melwitt: it happens in compute, which never uses target_cell, so I wouldn't think that is related | |
| 16:18:02 | cdent | melwitt: rado is pretty sure it is "simply" an import ordering problem | |
| 16:18:22 | cdent | he can fix it by changing things around | |
| 16:18:23 | dansmith | like the comment on that patch says | |
| 16:18:24 | melwitt | oh, yeah okay. that makes more sense | |
| 16:19:03 | melwitt | ah, I missed that comment | |
| 16:19:28 | mriedem | dansmith: melwitt: cdent: yeah says the same as radu's bug | |
| 16:19:39 | mriedem | "In his patch, eventlet.monkey_patch() must before import oslo_log (actually before import oslo_context)." | |
| 16:19:43 | dansmith | yeah | |
| 16:22:28 | melwitt | unrelated to that, I'd think the cell targeting would cause request_id changes in logging in nova-api though. but I'm not sure I've observed that before. I've seen only the ones in compute | |
| 16:23:26 | dansmith | melwitt: but any targeting of a context should result in the same request id right? | |
| 16:23:45 | dansmith | so even if you did overwrite the logging context with the targeted one, it shouldn't matter I would think | |
| 16:24:23 | melwitt | dansmith: yeah, maybe that's why I haven't observed it | |
| 16:24:27 | dansmith | yeah :) | |
| 16:24:46 | openstack | Launchpad bug 1773102 in OpenStack Compute (nova) "Abnormal request id in logs" [Medium,In progress] - Assigned to melanie witt (melwitt) | |
| 16:24:46 | melwitt | mriedem: do we want to dupe one of these to the other bug? https://bugs.launchpad.net/nova/+bug/1773102 | |
| 16:28:49 | mriedem | melwitt: yes done | |
| 16:28:55 | melwitt | thanks | |
| 16:30:03 | mriedem | cdent: i assume radu is pushing a patch? | |
| 16:30:36 | cdent | rado. checking | |
| 16:30:58 | cdent | ealier today he said he was | |
| 16:32:49 | cdent | mriedem: yeah, tomorrow | |
| 16:33:04 | cdent | (it's getting late where he is) | |
| 16:41:33 | melwitt | dansmith: I'm trying to determine if I could salvage this small part of the patch for the scheduler host manager, those could change logged request_id, I would think. do you disagree? https://review.openstack.org/#/c/582332/4/nova/scheduler/host_manager.py | |
| 16:42:29 | dansmith | melwitt: why do you want to use the current request's req-id for that? | |
| 16:42:34 | dansmith | or, meaningm, | |
| 16:42:50 | dansmith | why does it matter that we log the current request as the one that triggered the cache update? | |
| 16:42:55 | melwitt | want to avoid overwriting thread local context bc that's what's used in logging | |
| 16:43:41 | dansmith | that's the async_init_instance_info() part yeah? | |
| 16:44:02 | melwitt | creating the new RequestContext without overwrite=False will replace the thread local context with one that has a freshly generated request_id, so log messages that occur after it was overwritten could have the random request_id | |
| 16:44:35 | melwitt | yeah _async_init_instance_info and refresh_cells_caches | |
| 16:44:50 | dansmith | right, but, why not just create those with overwrite=False | |
| 16:45:06 | melwitt | dansmith: that's what my change does, uses a helper method that does that | |
| 16:45:41 | melwitt | it's the equivalent of get_admin_context except not admin | |
| 16:46:17 | dansmith | oh, okay I see, I thought that was getting it from the TLS for some reason | |
| 16:47:08 | dansmith | why is that not an admin context? | |
| 16:47:29 | melwitt | because the original wasn't? you think it should be admin? | |
| 16:48:06 | dansmith | yeah, I know, it wasn't.. but the reason I didn't match what that get_context() was, is because I normally think of get_admin_context() as "I need an internal context to do a DB thing" | |
| 16:48:15 | dansmith | like, | |
| 16:48:30 | dansmith | it's a pattern I recognize, but don't recognize the get_context() bit even though they're basically the same | |
| 16:48:42 | melwitt | oh, I see. yeah, it could be, but I wasn't thinking to change it from what it was before because I don't know if it was intentional | |
| 16:48:50 | dansmith | anyway, yes probably worth keeping that bit | |
| 16:49:01 | dansmith | yeah, I'm surprised it doesn't need to be admin because it's listing all instances right? | |
| 16:49:16 | dansmith | maybe it works because it has no project_id? | |
| 16:49:20 | dansmith | anyway, seems weird | |
| 16:49:40 | dansmith | those are clearly admin-y internal cache-fill ops, so I would think.. admin. | |
| 16:49:45 | melwitt | yeah, that's a good point. it would miss instances where project_id doesn't match, so maybe it misses instances | |
| 16:50:08 | dansmith | well, it's setting project_id=None, so presumably it's working, but weird | |
| 16:50:19 | melwitt | admin doesn't have to match project_id for instance get_all but non-admin does. I have learned that the hard way from func test writing | |
| 16:50:28 | dansmith | also, would it not make sense to make oslo_context not overwrite context in TLS by default if one is already set? | |
| 16:50:52 | melwitt | I dunno, tbh | |
| 16:50:55 | dansmith | melwitt: right, but surely this isn't so broken | |
| 16:51:03 | dansmith | that it's not collecting any instances right? | |
| 16:52:04 | melwitt | yeah, it would be surprising if it's not working but I admit I don't understand how it's working with a blanked out non-admin context | |
| 16:52:12 | dansmith | yeah | |
| 16:52:30 | dansmith | I think I would admin the eff out of that mah fah | |
| 16:52:38 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Use placement 1.28 in scheduler report client https://review.openstack.org/583667 | |
| 16:52:40 | melwitt | there are places we do the same thing in nova-manage too, and those work too I think. hm. | |
| 16:52:53 | melwitt | yeah, I'll change it to admin | |
| 16:53:30 | melwitt | ah, it's doing context.elevated() | |
| 16:53:35 | dansmith | lol | |
| 16:53:36 | dansmith | hah | |
| 16:53:36 | melwitt | I missed that | |
| 16:53:50 | dansmith | maybe just get_admin_context() then? :) | |
| 16:53:56 | melwitt | right at the instance list call | |
| 16:54:13 | melwitt | I'm not disagreeing with you, I'd rather have it be more obvious when it's created | |
| 16:54:29 | melwitt | i.e. make it admin | |
| 16:54:31 | dansmith | yeah, I'm just making fun of the round-about-ness causing confusing | |
| 16:54:33 | dansmith | *confusion | |
| 16:54:39 | melwitt | I just didn't get how this was working until I finally noticed the elevated() call | |
| 16:54:53 | melwitt | ah, yeah :) | |
| 16:56:47 | gibi | tetsuro: FYI, made a bit of progress in https://review.openstack.org/583667 and pushed it up. I don't have any other modification in progress locally, so you are free to change the series while I asleep | |
| 16:56:49 | melwitt | yay, get to keep part of mah patch | |
| 16:58:41 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: Remove usage of migrateToURI{2} APIs https://review.openstack.org/567258 | |
| 16:58:48 | kashyap | sahid: gibi: Thanks, I've addressed the final nits ^ | |
| 16:59:35 | kashyap | melwitt: In your "copious free time", appreciate if you'll have a gander, as you've reviewed a past iteration | |
| 16:59:56 | melwitt | okay, in nova-manage we're using non-admin RequestContext but we call elevated() if we're going to query for instances | |
| 16:59:57 | kashyap | sahid: s/nit/required missing bit in the test/ (which is not a nit :-)). | |
| 17:00:07 | melwitt | kashyap: ack | |
| 17:00:47 | melwitt | which makes sense, only use admin-ness when you need it | |
| 17:02:27 | melwitt | it just throws me off a lot because everywhere else we use get_admin_context for similar things | |
| 17:08:50 | gibi | kashyap: thanks. I put back my +2 | |
| 17:14:44 | openstackgerrit | melanie witt proposed openstack/nova master: Don't overwrite greenthread-local context in host manager https://review.openstack.org/582332 | |
| 17:21:51 | dansmith | man, this poor guy keeps getting disconnected trying to tell us his story | |
| 17:26:30 | cdent | I'm more interested in the fentanyl | |
| 17:30:33 | cdent | thanks! | |
| 17:34:47 | mriedem | johnthetubaguy: you know this Stats object that we manage in the ResourceTracker per node which gets used in the scheduler? could that ever possibly be correct for ironic? | |
| 17:35:27 | mriedem | because RT.stats is a cumulative set of stats for all instances managed by the compute host, | |
| 17:35:31 | mriedem | and gets copied per node | |
| 17:39:45 | mriedem | although i thought before traits, people would use ComputeCapabilitiesFilter for ironic | |
| 17:39:49 | mriedem | and that relies on stats | |
| 17:39:57 | mriedem | jroll: ^? | |
| 17:40:35 | jroll | hm | |
| 17:41:29 | jroll | I thought stats was per node, not host | |
| 17:41:53 | mriedem | they are, | |
| 17:41:54 | mriedem | but see https://review.openstack.org/#/c/576099/2/nova/compute/resource_tracker.py@1130 | |
| 17:43:13 | jroll | mriedem: # purge old stats and init with anything passed in by the driver | |
| 17:43:28 | jroll | tells me it's handled, without digging too deep | |
| 17:43:53 | mriedem | oh i didn't see that | |