Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-17
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.
21:53:54 efried cfriesen: You have to run stestr directly, and with the -n option and the full name of the test case.
21:54:13 efried or you can use remote_pdb
21:54:44 openstackgerrit Jackie Truong proposed openstack/python-novaclient master: Microversion 2.62 - Add trusted_image_certificates https://review.openstack.org/500396
21:55:54 efried cfriesen: e.g.
21:55:54 efried source .tox/func/bin/activate
21:55:54 efried stestr run -n nova.tests.functional.regressions.test_bug_1746509.InstanceListWithServicesTestCase.test_instance_list_deleted_service_with_no_uuid
21:56:11 efried s/func/functional/ I guess - whatever the venv name is for functional.
21:59:59 cfriesen efried: thanks, running ostestr directly worked
22:00:20 efried cfriesen: ostestr? Are you not in nova? But cool.
22:00:43 cfriesen efried: nova, but stable/pike. it's what tox was using
22:00:46 efried ahh
22:01:08 cfriesen mriedem: running db.service_get_all_by_binary() righte before the call to self.admin_api.get_servers() shows that both services have uuid entries
22:01:27 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix docs for confirmResize action https://review.openstack.org/562062
22:01:59 mriedem cfriesen: yes, which they both will for the non-deleted services
22:02:13 mriedem because host1.start() creates a new service record with a uuid
22:02:25 mriedem simulating that you've upgraded to pike and started that compute host with pike code
22:02:38 mriedem after deleting the "newton" compute service / node which didn't have the uuid set
22:03:53 cfriesen mriedem: right, I need the mutated context to read the deleted one.
22:04:20 openstackgerrit Jackie Truong proposed openstack/python-novaclient master: Microversion 2.62 - Add trusted_image_certificates https://review.openstack.org/500396
22:06:26 mriedem cfriesen: only other thing i could think of is like melwitt was saying yesterday and the backref joinedload on the services table is getting lucky and pulling the non-deleted service when listing instances
22:06:49 mriedem this https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/models.py#L103
22:07:03 cfriesen mriedem: the deleted service in the DB has a uuid
22:07:30 melwitt mriedem, cfriesen: I was wrong about that. the instance.services is all services whose host match the instance, and it will try to add a UUID to both of them, so should reliably fail I think
22:08:46 mriedem cfriesen: ok i don't know how that is per https://review.openstack.org/#/c/562041/2/nova/tests/functional/regressions/test_bug_1764556.py@149

Earlier   Later