Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-17
20:48:39 mriedem but, that basically means it's already fixed
20:49:11 cfriesen mriedem: does host1.stop() delete the entry in the DB? or do we need host1.kill()?
20:49:24 cfriesen I definitely tested it with current stable/pike in devstack
20:49:33 mriedem .stop() doesn't delete the service
20:50:16 openstackgerrit Merged openstack/nova master: Parse forbidden in extra_specs https://review.openstack.org/561677
20:50:20 cfriesen ah, I see. you're stopping it then deleting it
20:50:23 mriedem besides, yeah
20:50:49 mriedem i stop the service, delete it, and then have to null out the service uuid because as noted, deleting the service via the rest api looks it up and will migrate the uuid if not set
20:50:59 cfriesen let me just double-check my devstack
20:52:13 mriedem cfriesen: did you have https://review.openstack.org/#/c/545988/ in your devstack env?
20:52:30 openstackgerrit Merged openstack/nova stable/pike: libvirt: Block swap volume attempts with encrypted volumes prior to Queens https://review.openstack.org/543569
20:52:32 mriedem oh you said current stable/pike
20:53:42 openstackgerrit iain MacDonnell proposed openstack/nova master: Remove placement config check https://review.openstack.org/558089
20:56:01 mriedem well, if anything, i guess i can still leave this test in place for bug 1745977 so we have coverage of the scenario
20:56:01 openstack bug 1745977 in OpenStack Compute (nova) queens "When source compute service up, will not destroy and clean up those instances which be evacuated then be deleted." [Medium,Fix committed] https://launchpad.net/bugs/1745977 - Assigned to Matt Riedemann (mriedem)
20:57:08 cfriesen mriedem: when you list the servers, are you doing it as an admin user?
20:57:21 cfriesen mriedem: regular user doesn't get the host info
20:57:57 cfriesen mriedem: and so doesn't trigger the bug
20:58:41 cfriesen I just reproduced in devstack after restarting all the nova services, so should be valid
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.

Earlier   Later