| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-20 | |||
| 19:00:47 | melwitt | but it's from around that time | |
| 19:01:00 | melwitt | yes, it's a RequestContext | |
| 19:01:10 | efried | What's in the RequestContext.service_catalog? | |
| 19:03:01 | melwitt | it's empty ... though I think that's my fault, I created an admin context to call glance.api_servers | |
| 19:05:35 | efried | Well, unless I'm mistaken, the get_endpoint() stuff winds up in whatever context you're using. So that would splain why it's empty in your pdb. | |
| 19:06:31 | melwitt | is the RequestContext.service_catalog supposed to get populated as a result of the get_endpoint() call? or is it supposed to be pre-populated before the get_endpoint call? | |
| 19:06:47 | efried | The latter | |
| 19:07:01 | melwitt | okay, yeah, then I've messed up this attempt to repro the problem | |
| 19:07:02 | efried | get_endpoint gets its information *from* the context. | |
| 19:08:26 | openstackgerrit | Chris Dent proposed openstack/nova master: WIP: Parse placement forbidden traits query string https://review.openstack.org/554665 | |
| 19:08:53 | openstackgerrit | Giridhar Jayavelu proposed openstack/nova-specs master: VMware: place instances on resource pool https://review.openstack.org/549067 | |
| 19:13:21 | openstackgerrit | Jim Rollenhagen proposed openstack/nova master: ironic: stop lying to the RT when ironic is down https://review.openstack.org/545479 | |
| 19:13:23 | jroll | whee, that should be good now | |
| 19:13:27 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Use Queens UCA for nova-multiattach job https://review.openstack.org/554317 | |
| 19:13:28 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Preserve multiattach flag when refreshing connection_info https://review.openstack.org/554667 | |
| 19:16:59 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Use Queens UCA for nova-multiattach job https://review.openstack.org/554317 | |
| 19:16:59 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Preserve multiattach flag when refreshing connection_info https://review.openstack.org/554667 | |
| 19:24:14 | melwitt | okay, repro'd again by actually creating an instance, doing a virsh shutdown, then waiting for nova-compute to stop the instance, same error and again RequestContext.service_catalog = [] (I logged it from nova/image/glance.py) | |
| 19:25:04 | melwitt | so somewhere along the way, the context we pass isn't one with a properly populated service_catalog | |
| 19:31:41 | melwitt | uh oh, I think I know why. this path is being run through a periodic task which has been given an anonymous get_admin_context(), and when we eventually try to do something with a service_catalog in the RequestContext, there isn't going to be one | |
| 19:32:18 | efried | melwitt: That'd do it. Lemme dig up the patch that did the context-y stuff... | |
| 19:32:52 | efried | melwitt: https://review.openstack.org/#/c/490057/ | |
| 19:32:53 | melwitt | I'm not sure what the answer is here, other than gibi looking into whether we can avoid relying on service catalog stuff for sending notifications | |
| 19:33:19 | efried | melwitt: That patch ought to help us isolate which context is giving us grief here. | |
| 19:33:23 | mriedem | nova doesn't have admin creds to glance like we do for cinder and neutron, so you can't rely on that in a periodic either | |
| 19:33:25 | melwitt | because the context we use in periodic tasks is going to be a mostly empty admin one intended to read the database, etc. it's not going to have service catalog info in it | |
| 19:33:43 | melwitt | oh, okay. so we are ok for cinder and neutron then | |
| 19:33:54 | mriedem | if properly configured | |
| 19:34:06 | mriedem | you have to configure nova to talk to neutron with an admin role token for port binding, | |
| 19:34:18 | efried | Mm, yeah, it's coming back to me. I don't remember where else we've seen this, but the answer was: if you want this to work, you have to supply creds in the conf so that we can build a proper admin context. | |
| 19:34:19 | mriedem | the cinder <> admin config was added in queens for a related bug with periodic tasks | |
| 19:34:21 | mriedem | doing things with volumes | |
| 19:34:28 | efried | Yeah, what mriedem said. | |
| 19:34:33 | melwitt | gotcha, okay | |
| 19:34:44 | efried | And... is that okay? | |
| 19:34:51 | efried | or do we need to "fix" it? | |
| 19:35:10 | mriedem | for notifications, we really shouldn't have to hit a REST API every time we send a notification, because that's kind of crazy | |
| 19:35:22 | openstackgerrit | sean mooney proposed openstack/nova master: add mtu to libvirt xml for ethernet and bridge types https://review.openstack.org/553072 | |
| 19:35:39 | efried | Yeah, I'll agree with that. Probably the first thing to look into, then - why we need to talk to glance to send a notification. | |
| 19:35:54 | mriedem | i've mentioned this before, | |
| 19:35:55 | melwitt | yeah, gibi said he's going to investigate that tomorrrow | |
| 19:36:08 | mriedem | but whenever we construct a glance "client" object in-tree, it goes thorugh the 'get endpoint url' stuff | |
| 19:36:35 | mriedem | notifications goes through info_from_instance to build a payload, | |
| 19:36:41 | mriedem | which gets an image ref URL | |
| 19:36:47 | efried | mriedem: Yeah, I remember you mentioning it was doing it like 4000 times in a devstack run, or something. | |
| 19:36:52 | mriedem | efried: yup | |
| 19:36:53 | efried | s/devstack/tempest/ | |
| 19:36:58 | mriedem | we f'ing love to hit glance | |
| 19:37:24 | mriedem | if [glance]/api_servers is set, we just build a static string based on that | |
| 19:37:29 | sean-k-mooney[m] | mriedem: so the api call to glance when sending the notificaiton is from creating the glance client object? | |
| 19:37:33 | mriedem | but if it's not set, we go through the ksa magik | |
| 19:37:45 | efried | Which *should* be using cached values, I thought. | |
| 19:37:55 | mriedem | maybe it is | |
| 19:37:55 | efried | i.e. we're not actually hitting the API 4000 times. | |
| 19:38:02 | mriedem | that would be nice to know | |
| 19:38:46 | efried | mriedem: Yeah, we build the adapter every time, but the session & auth are cached. | |
| 19:39:22 | sean-k-mooney[m] | efried: well if the call to the api is coming from createing the client object perhaps we should be storing that between requests so that we dont have to keep creating it | |
| 19:39:48 | openstackgerrit | Jackie Truong proposed openstack/nova master: Add trusted_certs object https://review.openstack.org/489408 | |
| 19:39:48 | openstackgerrit | Jackie Truong proposed openstack/nova master: Add trusted_certs to instance_extra https://review.openstack.org/537897 | |
| 19:39:50 | openstackgerrit | Jackie Truong proposed openstack/nova master: Implement certificate_utils https://review.openstack.org/479949 | |
| 19:39:52 | openstackgerrit | Jackie Truong proposed openstack/nova master: Add trusted_image_certificates to REST API https://review.openstack.org/486204 | |
| 19:40:12 | mriedem | sean-k-mooney[m]: efried is saying we're not hitting the api every time | |
| 19:40:19 | mriedem | just the first time to get the service catalog entry | |
| 19:40:29 | mriedem | and it's the identity api in this case | |
| 19:40:41 | sean-k-mooney | mriedem: yes but we are still creating the client 4000 and hitting the cache 3999 times right? | |
| 19:40:47 | mriedem | alternatively, don't put an image ref url in the notification payload, just the image uuid | |
| 19:40:52 | mriedem | sean-k-mooney[m]: yeah | |
| 19:41:28 | mriedem | the notification payload attempts to mimic the GET /servers/detail API which returns the image id and bookmark link to the image | |
| 19:41:35 | mriedem | using the same thing we're hitting here | |
| 19:41:35 | sean-k-mooney | so if we create teh client once and resuit we definetlly dont hit the api and we get rid of 3999 calls to the client constructor? | |
| 19:42:29 | mriedem | sure, but that's lower priority atm | |
| 19:42:37 | mriedem | the thing now is how to fix this periodic | |
| 19:42:48 | efried | It'd be interesting to see if I can cache the adapter and stuff will still work. | |
| 19:43:06 | sean-k-mooney | mriedem: image uuid might be better in general then the url | |
| 19:43:21 | mriedem | sean-k-mooney: at least for the notification yeah i agree | |
| 19:43:47 | mriedem | we can change that with versioned notifications, but this also gets shoved into the legacy notifications and changing those is like breaking an api | |
| 19:43:49 | sean-k-mooney | glance can have multiple image urls correct? is there any guarntee the consumer of the url can reach that backend | |
| 19:44:38 | mriedem | well, it's over rpc so if you've configured nova to hit internal glance api endpoints, your notification consumer probably can too | |
| 19:44:57 | mriedem | if that consumer actually needs to get the image details, idk | |
| 19:45:11 | mriedem | or why the consumer can't just take the image id and form it's own image api request, ... | |
| 19:45:29 | sean-k-mooney | mriedem: sure but with the uuid they can query for which ever one they actully need so that is more generally useful i think | |
| 19:45:30 | mriedem | it's not like we return links to volumes and ports in the notification either | |
| 19:45:38 | mriedem | sean-k-mooney: i agree | |
| 19:45:42 | melwitt | yeah, the only other thing I can think of for fixing the periodic task is if we could somehow seed periodic tasks with the service catalog instead of the empty get_admin_context() one, but I don't know what would be involved there | |
| 19:46:10 | melwitt | I guess it anyway won't be able to get any info from glance even if it had the catalog, it would just fail later, right? | |
| 19:46:34 | mriedem | the notification path here isn't actually doing a GET to glance | |
| 19:46:47 | efried | Yeah, I'm not worried about that bit at all. | |
| 19:46:49 | mriedem | it's getting the service catalog from keystone via ksa | |
| 19:46:53 | melwitt | meaning, it won't do that even if it has the catalog? | |
| 19:46:54 | mriedem | but w/o a token | |
| 19:47:05 | melwitt | okay | |
| 19:47:27 | mriedem | so for now, the easiest thing to do probably do is in this notification code, handle EndpointNotFound and just set the image_ref_url to the image id | |
| 19:47:36 | efried | are context hashable? | |
| 19:47:45 | mriedem | they are serializable | |
| 19:47:46 | dansmith | edleafe: hmm, so member_of with multiple aggregates is doing an "or" of all the ones you pass, right? is that what we really want? | |
| 19:47:51 | openstackgerrit | Merged openstack/nova master: [libvirt] Add _get_XXXpin_cpuset() https://review.openstack.org/527631 | |
| 19:47:54 | efried | so probably | |
| 19:48:00 | mriedem | efried: i'd rather not do crazy cache shit with contexts | |
| 19:48:02 | openstackgerrit | Merged openstack/nova master: api-ref: add a note in DELETE /os-services about deleting computes https://review.openstack.org/553598 | |
| 19:48:14 | mriedem | we already have some craziness with contexts and periodics today pulling off the local thread storage | |