Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-17
20:59:39 openstackgerrit melanie witt proposed openstack/nova master: Add periodic task to clean expired console tokens https://review.openstack.org/325381
20:59:40 openstackgerrit melanie witt proposed openstack/nova master: Use ConsoleAuthToken object to generate authorizations https://review.openstack.org/325414
20:59:41 openstackgerrit melanie witt proposed openstack/nova master: Convert websocketproxy to use db for token validation https://review.openstack.org/333990
20:59:42 openstackgerrit melanie witt proposed openstack/nova master: Update layout docs for running console proxies https://review.openstack.org/557489
20:59:44 cfriesen I wonder if you just replaced self.api.get_servers() at the bottom with self.admin_api.get_servers()...
20:59:45 mriedem cfriesen: no, not admin
21:00:09 mriedem i can try that
21:00:50 efried jaypipes: Quick +A on previously +2'd https://review.openstack.org/#/c/558089/ ?
21:04:26 mriedem there will be an obvious error in the n-cpu logs when placement isn't configured without that?
21:04:37 mriedem or will we just get the safe_connect warnings?
21:05:10 mriedem cfriesen: that doesn't change anything, listing with admin
21:05:47 efried mriedem: We should see MissingAuthPlugin
21:05:53 efried mriedem: But IMO safe_connect is still broken.
21:06:15 mriedem so we'll get https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L77
21:06:21 mriedem and nova-compute will be running and not reporting into placement
21:06:57 dansmith we shouldn't warn_limit that I think
21:07:04 efried mriedem: Yup, I believe that's right. You'll get that nag the first ten times (or whatever) and then it'll quit.
21:07:28 efried It's more broken than warn_limit can fix.
21:07:51 efried What's broken about it is that we don't raise an exception when it happens. We just let the decorated code continue.
21:08:08 dansmith yeah we should explode I think
21:08:14 mriedem well, for nova-compute sure
21:08:20 mriedem but for nova-api in rocky no
21:08:38 efried If safe_connect was used extremely carefully - only in the outermost scope of whatever is calling into placement - then it would be less broken.
21:08:55 mriedem i.e. https://review.openstack.org/#/c/560706/2/releasenotes/notes/bug-1679750-local-delete-allocations-cb7bfbcb6c36b6a2.yaml
21:09:09 efried But as it stands, it is theoretically possible for it to reconnect and then proceed, having skipped the first step of whatever that outer process was.
21:09:34 efried For this patch, it makes no difference - *nothing* will get done to placement. But for random midstream disconnects, it could potentially be disastrous.
21:11:07 efried I guess ^ only applies for the EndpointNotFound branch.
21:11:17 efried cause that's the only place we try to reconnect.
21:12:28 dansmith safe_connect was to make moving from no-placement to placement very smooth
21:12:36 dansmith it probably needs to change quite a bit in today's world
21:12:46 dansmith logging every single time we fail to talk to placement at least,
21:13:03 dansmith and probably raising a lot more so we don't fail to make changes we think we're making
21:14:31 mriedem so before rocky we never said that nova-api required config to talk to placement, are we now saying it does and if you don't have that config in place things are going to blow up?
21:14:51 mriedem specifically thinking about https://review.openstack.org/#/q/topic:bug/1756179+(status:open+OR+status:merged) and the mirror aggregates stuff
21:15:03 dansmith personally that doesn't bother me, but api-initiated code can be graceful about it
21:15:05 mriedem the former i care about more b/c i thought we would backport those
21:15:09 dansmith without needing safe_connect hiding everything from us
21:16:05 dansmith IMHO, ultimately the api aggregate mirroring stuff should be strict about not making local changes if it can't tell placement, so it needs good exception handling anyway
21:16:22 dansmith making it graceful now and strict later is easy if it's all up there
21:16:34 dansmith well, not all, but .. the handling of this not-configured case at least
21:16:34 mriedem so basically move safe_connect to api
21:16:43 openstackgerrit Merged openstack/nova master: libvirt: Report the allocated size of preallocated file based disks https://review.openstack.org/561704
21:16:45 mriedem and drop it in stein
21:16:53 dansmith I dunno that all of safe_connect needs to go away, but I'd have to go look at it
21:17:14 dansmith but yeah, the "tread lightly" function of it makes no sense for compute anymore, but api has the same need compute did
21:19:35 efried I like the idea of nixing safe_connect, at least in its current form. If we get rid of it, then placement connect errors will raise exceptions that will bubble all the way up the stack. Usually (i.e. in _update-land) that'll mean we hit whatever operation again on the next periodic task.
21:20:33 efried If we keep safe_connect, it should have some functionality that... connects... safely. E.g. the current exception clauses should reconnect and then retry the original call (once).
21:20:53 efried That ^ is what I was expecting it to do when I first saw it.
21:21:00 efried ...before I actually read the code.
21:21:14 openstackgerrit Doug Hellmann proposed openstack/os-vif master: Add lower-constraints job https://review.openstack.org/556056
21:21:59 efried At this point, perhaps certain of those exceptions (like MissingAuthPlugin) should actually disable the compute service.
21:22:00 mriedem i could see adding a kwarg to safe_connect telling it how to behave, so that by default it can now explode on connection failures (nova-compute, scheduler, conductor would want that), but nova-api could tell it to not fail due to those
21:22:29 mriedem efried: well, that was the point of the check on startup region_name
21:22:37 mriedem *using region_name
21:22:54 efried mriedem: Right; so at this point are we ever calling report client from code paths that *don't* require placement to be online?
21:23:04 dansmith he is asserting api
21:23:16 mriedem we have patches up for review which add new requirements to nova-api for placement
21:23:17 dansmith but I personally don't think it's unreasonable to start calling it
21:23:19 efried I'm sayin, are we calling it from API even if we don't require it?
21:23:22 mriedem and i'm trying to make those graceful for upgrades
21:23:36 dansmith I feel like we'd start or stop talking to cinder or neutron from api without thinking about it
21:23:56 dansmith but also,
21:24:08 dansmith I don't think we have to have safe_connect at the lowest layer just because api wants it
21:24:30 mriedem i agree with the last thing you said
21:24:31 dansmith api can do the placement mirroring stuff and except Exception
21:24:52 dansmith or we can move safe_connect to a context manager and you can do:
21:25:00 dansmith with safe_placement_stuff: do_things;
21:25:02 mriedem ^ is what i'm thinking
21:25:16 dansmith and it will catch and ignore or log or whatever is appropriate for up there
21:26:19 efried dansmith: Would you use that context manager from RT or from methods within report client?
21:26:40 dansmith you'd use it in the api methods getting ready to talk to placement
21:26:53 dansmith or anywhere else that still wants safe_connect behavior
21:27:17 dansmith I would expect report client to catch things it needs, bubble unexpected things (like this auth thing) up to the caller
21:27:33 dansmith or translate them into something like PlacementNoWorky or whatever
21:27:53 efried okay. Cause I think the major hole with what we're doing now is that we're calling it from within report client, often in ways that may cause us to be ignoring an exception on step 3 of a 10-step process and then proceeding with the rest.
21:28:23 dansmith right
21:28:24 dansmith which is why we shouldn't have it underneath report client
21:28:28 efried ++
21:28:42 efried unless it's doing a reconnect-and-retry thing.
21:28:47 dansmith sure
21:28:56 cfriesen mriedem: I'm confused why it's not failing...it was trivially easy to reproduce in devstack by adding a single row to the services table.
21:29:09 efried In which case it should probably decorate the get/put/post/delete primitives and nothing else.
21:33:36 lyarwood Does anyone have a simple way to enable debug logging within functional tests? self.flags doesn't appear to work.
21:34:53 mriedem OS_DEBUG=True
21:35:14 mriedem https://github.com/openstack/nova/blob/ad249d8994067b1c255823cb7c1ff4c32593e5ef/tox.ini#L24
21:35:36 lyarwood thanks! :)
21:43:39 cfriesen mriedem: when I run your testcase locally it's failing in _migrate_server()
21:44:57 cfriesen http://paste.openstack.org/show/719417/
21:45:16 mriedem cfriesen: in pike?
21:45:53 mriedem because pike doesn't have 204 in this list https://github.com/openstack/nova/blob/87ea686f9f2cc706205d188922bb14272625e7be/nova/tests/functional/api/client.py#L223
21:46:04 openstackgerrit Eric Fried proposed openstack/nova master: WIP: Make safe_connect retry or raise https://review.openstack.org/562060
21:46:09 efried dansmith, mriedem: ^
21:46:48 efried dansmith, mriedem: There would be a separate, unrelated thingy for the n-api side of the world.
21:46:55 cfriesen mriedem: yes, in pike
21:48:13 cfriesen retrying with that
21:49:59 cfriesen is there a way to stick a pdb breakpoint in there right before the last line?
21:52:35 efried cfriesen: Are you asking how to pdb via stestr?
21:52:56 efried cause I have that answer
21:53:22 cfriesen efried: I want to pdb in a functional test
21:53:41 efried cfriesen: Same as in a unit test.

Earlier   Later