| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-04-22 | |||
| 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 | efried | Use prints... and hope the stdout fixture isn't set up | |
| 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: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 | efried | like, if you specify a bogus region name or whatever. | |
| 16:22:21 | eandersson | But regions don't actually work | |
| 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 | 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:21 | efried | 2) New patch to pass region_name through to irocicclient setup | |
| 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. | |
| 16:39:20 | efried | I suspect I would be able to infer more if you addressed mriedem's comments | |
| 16:40:08 | efried | Do you have a sec to give me a brief rundown on what actual operations or behaviors are affected by these *HypervisorCPU() calls? | |
| 16:41:52 | openstackgerrit | Eric Fried proposed openstack/nova master: Fix {min|max}_version in ironic Adapter setup https://review.opendev.org/654457 | |
| 17:47:29 | openstackgerrit | Nikolay Fedotov proposed openstack/nova master: Fix disappearing ironic hypervisors after rebuilding hashring https://review.opendev.org/654584 | |
| 17:54:25 | sean-k-mooney | efried: kashyap spec is mainly techdebt removal through the use of newer libvirt apis that would allow nova to validate that the configuration specified in the nova.conf is valid | |
| 17:55:09 | efried | sean-k-mooney: Resulting in... earlier failures in spawn()? Refusal to start nova-compute? | |
| 17:55:54 | sean-k-mooney | i was suggesting failing to start the agent if the cpu flags were not compatible with the host/model specifed | |
| 17:56:07 | sean-k-mooney | but the spec does not really make it clear | |
| 17:57:32 | sean-k-mooney | it will in theroy also make the traits we report more acurate as it would take into account if kvm or qemu/which version of qemu is used too. | |
| 18:00:40 | sean-k-mooney | with all that said i agree with mriedem commets. the spec does not really spell out how useing the new apis deliver a new feature or capablity to operators/users form a nova perspecitve | |