| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-04-22 | |||
| 13:51:54 | jaypipes | lpetrut: there are more hypervisors than libvirt. :) | |
| 13:52:20 | jaypipes | lpetrut: and the content of the libvirt XML file changes over time of course. | |
| 13:52:57 | jaypipes | lpetrut: we've added things to os-traits as needed. if there's a particular instruction set extension you want/need, don't hesitate to throw up a patch adding it. :) | |
| 13:53:05 | lpetrut | jaypapipes: indeed, in fact I was looking into implementing 'update_provider_tree' for the hyper-v driver :) but those should probably be standardized at the openstack level | |
| 13:53:16 | jaypipes | ++ | |
| 13:53:49 | jaypipes | lpetrut: to answer your question, I don't remember there being any rhyme or reason to the ones I added. probably I just scraped things from unit tests or boxes I had lying around ;) | |
| 13:54:26 | lpetrut | got it, so basically each feature/trait was defined as needed (as opposed to having an extensive list) | |
| 13:55:14 | mriedem | lpetrut: do you guys plan on upstreaming feature parity stuff like this at some point? https://opendev.org/x/compute-hyperv/commit/d4f1fa457a0fcbd1ac84c816157175c090958d6f | |
| 13:55:30 | mriedem | https://opendev.org/x/compute-hyperv/commit/f615b509a2022025f2465341922a2e11427f8822 | |
| 13:55:47 | mriedem | https://opendev.org/x/compute-hyperv/commit/8326d2c43c3e464f0c2e3b58bf57b4ffb9a344e7 | |
| 13:56:18 | lpetrut | those patches look straight forward, should be easy to review/merge | |
| 13:56:51 | lpetrut | well, maybe not the last one :) | |
| 13:56:58 | mriedem | they are at least a specless blueprint just since they are feature additions, but it seems all the feature work on the driver has been out of tree for the last couple of years | |
| 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. | |