Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-04-22
13:57:33 jaypipes lpetrut: yes, exactly.
13:58:32 mriedem lpetrut: anyway, just a thought, those first 2 look simple. i'm not looking to generate work, but i'd like to see the in-tree driver close some of these feature gaps that are already in the out of tree driver.
13:59:39 lpetrut mriedem: yep, we can start with a few patches to close the gap a bit. thanks for bringing this up
14:01:44 mriedem bauzas: this is another simple functional recreate test for a stein regression: https://review.opendev.org/#/c/653098/
14:05:19 mriedem jaypipes: trying to think of ways to maybe debug that oslo.config getattr thing - we could maybe have a DNM patch to oslo.config that counts the number of times we've recursed into that getattr loop and log a traceback to see what's triggering it, then push a nova change that depends-on that oslo.config debug patch?
14:09:53 ganso mriedem: hey Matt! sorry for interrupting the current convo, but just a heads up of https://review.opendev.org/#/c/652153 ... I tested in my env and it did not work. It could be perhaps because I run stable packages instead of directly from source. Did it work in your env?
14:11:08 mriedem ganso: i saw your comment but i haven't yet tried to recreate in a queens devstack
14:11:23 mriedem do you see errors in the source node compute logs?
14:11:35 mriedem did you try rocky?
14:11:45 ganso mriedem: no I did not see any errors. It just did not remove the allocations
14:11:50 ganso mriedem: no I haven't tried rocky
14:12:33 mriedem hmm, i'm not sure what the problem would be since it's calling the same method
14:13:40 mriedem alternatively i could write a functional test to recreate it
14:14:07 mriedem if you can debug it would obviously speed things up
14:25:21 jaypipes mriedem: yeah, the issue is we don't really know where the original call site is for the getattr that is causing the stack exhaustion. if we did know what is the thing that is the base object or thing that is being called there, we could swap out that for either a mock or an actual collections.Mapping etc and see if the issue is in the ovo code, the oslo.cfg code, or something else.
14:26:20 jaypipes mriedem: I'm wondering if there's a way to limit the Python stack recursion size to a smallish number so that the stacktrace would show the entire traceback instead of just the last hundred or so repeated recursive calls to getattr.
14:26:43 jaypipes https://docs.python.org/3/library/sys.html#sys.setrecursionlimit
14:27:03 jaypipes might be worth throwing that in a test, mriedem? ^
14:27:11 jaypipes just to get the whole traceback.
14:27:21 jaypipes which might give us a better starting point.
14:32:17 mriedem jaypipes: that's what i'm working on - dump the traceback to the logs before we get too far into the recursion
14:34:12 jaypipes mriedem: coo.
14:34:25 mriedem i'll push a nova change which depends-on it and see if we can recreate
14:35:23 ganso mriedem: thanks!
14:38:01 openstackgerrit Matt Riedemann proposed openstack/nova master: DNM: debug config opts infinite recursion https://review.opendev.org/654468
15:13:33 openstackgerrit Merged openstack/nova stable/pike: Fix incomplete instance data returned after build failure https://review.opendev.org/647916
15:15:54 efried mriedem: Where are the logs you inserted?
15:18:46 efried I don't see 'em in the job output or subunit results
15:22:18 efried maybe logger config is set higher for oslo?
15:24:13 mriedem efried: it won't dump the logs unless we hit a failure i don't think...
15:24:16 mriedem maybe i need to use prints
15:24:47 mriedem because what fails (TestRPC) isn't where the stack overflow happens i don't think
15:26:16 mriedem i don't see any default log level set for oslo.config either https://github.com/openstack/oslo.log/blob/22e8a347c8ba914cefa26df42dde632937259a79/oslo_log/_options.py#L19
15:26:16 efried Use prints... and hope the stdout fixture isn't set up
15:26:51 efried so what's the default default if there's no default?
15:27:04 mriedem in unit tests it'd be INFO
15:27:14 efried okay
15:27:19 mriedem which is why we have the OS_DEBUG env var
16:20:28 eandersson Looks good efried
16:20:39 eandersson As for your devstack comment, that was never meant to break
16:20:44 eandersson as you are passing on endpoint = None
16:20:59 eandersson https://review.opendev.org/#/c/653839/
16:21:02 eandersson This was the actual test
16:22:02 efried eandersson: But that test only reports that our endpoint lookup failed
16:22:12 eandersson Exactly
16:22:13 efried eandersson: ...which is still a valid code path
16:22:16 eandersson Sure
16:22:21 eandersson But regions don't actually work
16:22:21 efried like, if you specify a bogus region name or whatever.
16:22:25 efried right
16:22:29 eandersson Not sure what your point is
16:22:30 efried see latest comment in the devstack patch.
16:22:55 eandersson Well that patch was not to prove that multi region was broken
16:22:59 efried eandersson: My point is: I'm happy to have the behavior fixed, but it would be nice to have a regression test in place.
16:23:07 eandersson That patch was to prove that get_endpoint was broken if region is supplied
16:23:21 eandersson Of course if you introduced multi-region it would have been way worse
16:23:29 efried okay, and that proof is the presence of that log message...
16:23:30 eandersson Since that would never have worked
16:23:59 efried so now if we reinstate that patch and rebase it onto the fix, the log message should be absent.
16:24:12 eandersson Exactly
16:24:20 efried In fact, we could just raise an exception and blow up the world if the endpoint is None there.
16:24:29 eandersson I think we should.
16:24:36 efried cool, you or me? :)
16:24:55 eandersson I can follow up after your patch so people can git blame me :p
16:25:05 efried So hold on a sec
16:25:15 efried I'm not completely convinced we should be doing this for real
16:25:20 efried just to prove the fix
16:25:25 eandersson We can also take path nr 2
16:25:32 eandersson just pass on the region_name to the ironic client
16:25:51 efried If ironicclient has a way to recover, and we disable/disallow it, that could piss off some operators :)
16:26:02 efried So yeah, passing the region_name to the ironicclient's recovery dealy would be better I guess.
16:26:04 eandersson The problem is really that we are letting the ironic client handle a failure scenario, but not actually giving it the context it needs
16:26:09 efried Though once again we're throwing good code after bad.
16:26:47 efried okay, to pass the region_name through, we only need changes on the nova side, right? ironicclient is already set up to handle it if it's in kwargs?
16:27:14 eandersson https://github.com/openstack/python-ironicclient/blob/master/ironicclient/client.py#L121
16:27:18 eandersson Looks like it yea
16:28:20 efried Cool. So two things:
16:28:21 efried 2) New patch to pass region_name through to irocicclient setup
16:28:21 efried 1) Test patch that raises an exception if the ksa endpoint lookup fails, based on top of the fix https://review.opendev.org/654457 <== expect it *not* to blow up
16:28:33 eandersson Perfect!
16:28:36 efried I wonder if 2) should actually take two completely separate code paths.
16:28:49 efried One if ksa endpoint lookup succeeds, that passes in no additional kwargs
16:28:51 eandersson Yes - that would be fine
16:28:57 eandersson And safer
16:29:01 efried and a separate one if ksa lookup fails, that passes in basically all the ksa opts :(
16:29:37 efried If you have the time to propose those, I'll happily review. I'm in the midst of summit chart prep hell.
16:30:45 eandersson Sure - I'll try. I am also slammed this week though.
16:31:19 efried looks like interface is already passed through. service_type *should* be unnecessary, but <shrug> guess it wouldn't hurt to pass it through.
16:31:29 efried and those are the only three used at the moment.
16:31:32 eandersson Yea - was thinking the same.
16:32:49 eandersson Unrelated to this discussion. Is it just for me that I can't open links on review.opendev.org in new tabs anymore?
16:34:32 eandersson (right click, Open link in new tab)
16:36:01 efried uh
16:36:17 efried eandersson: wfm
16:36:31 efried what happens when you try it?
16:36:35 eandersson Nothing
16:36:59 eandersson Hmm works on Firefox
16:37:03 eandersson I guess my browser is just acting up.
16:37:41 efried I'm Chrome 73.0.3683.75 on Bionic fwiw
16:38:21 efried kashyap: I'm reading https://review.opendev.org/#/c/645814/4/specs/train/approved/cpu-selection-with-hypervisor-consideration.rst and not understanding it, at I think a more fundamental level than other reviewers.

Earlier   Later