Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-03-20
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
19:48:22 mriedem which totally effs with request id log tracing
19:48:23 efried mriedem: I was just going to experiment, cool yer jets.
19:48:33 mriedem i will not cool my jets or hold my horses
19:48:51 melwitt slow your roll
19:48:57 mriedem this is the part that blows up https://github.com/openstack/nova/blob/master/nova/notifications/base.py#L397
19:48:58 openstackgerrit Chris Dent proposed openstack/nova master: Use nova.db.api directly https://review.openstack.org/543262
19:49:54 sean-k-mooney mriedem: is instance.image_ref the image id?
19:51:18 melwitt gdi I tried to set the importance at the same time as mriedem and set it differently. will set it back
19:53:34 openstackgerrit Eric Young proposed openstack/nova master: Support extending attached ScaleIO volumes https://review.openstack.org/554679
19:54:17 mriedem sean-k-mooney: yes
19:54:28 melwitt mriedem, efried: are one of you going to propose the patch or shall I?
19:54:38 mriedem i can push a patch
19:54:43 melwitt k
19:54:44 mriedem if LP would not timeout on me

Earlier   Later