| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-04-22 | |||
| 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 | |
| 18:03:10 | efried | thanks sean-k-mooney. I'm putting together charts for the project update at the summit and need to understand to *some* extent what each blueprint is about. | |
| 18:04:43 | sean-k-mooney | efried: from kasyaps top comment on the rewview 'Nova will now be able to answer: "Is this combination of IvyBridge + 'pcid' & 'pdpe1gb' supported by KVM, QEMU and libvirt on the host?"' | |
| 18:05:24 | efried | Right, "be able to answer" doesn't help me understand what will actually *happen* | |
| 18:05:58 | efried | so, detect whether A+B+C is copacetic and... what? | |
| 18:06:17 | sean-k-mooney | so really kashyap is trying to turn the a possible runtime failure when you boot the first vm into hopefully a startup failure of the agent with a detailed error message | |
| 18:06:28 | efried | when you say "agent", you mean n-cpu? | |
| 18:06:43 | sean-k-mooney | ya at least that what i hope is the plan | |
| 18:06:43 | efried | ...by blowing up the virt driver init, I guess | |
| 18:06:50 | sean-k-mooney | yep | |
| 18:06:54 | efried | Cool. Definitely needs to be spelled out in the spec. | |