| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-sdks - 2019-08-13 | |||
| 10:55:22 | openstackgerrit | Dmitry Tantsur proposed openstack/openstacksdk master: Add support for fields in baremetal get_* resources https://review.opendev.org/676154 | |
| 10:55:24 | dtantsur | mordred: no. I've uncovered two actual bugs, I think ^^^ | |
| 10:55:48 | mordred | ah - neat | |
| 10:55:53 | dtantsur | reminder for us: never ignore arguments, somebody will try to provide them :) | |
| 10:56:03 | mordred | ++ | |
| 12:10:53 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Fix discovery cache sharing https://review.opendev.org/675121 | |
| 12:10:53 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Rationalize endpoint_for and get_endpoint_from_catalog https://review.opendev.org/675130 | |
| 12:10:54 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Add support for global_request_id https://review.opendev.org/674807 | |
| 12:10:54 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Validate that connect_as connects as the project https://review.opendev.org/675178 | |
| 12:10:55 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Replace catalog-v3.json with keystoneauth fixture https://review.opendev.org/675187 | |
| 12:10:55 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Allow services to override proxy creation https://review.opendev.org/674110 | |
| 12:11:49 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Rationalize endpoint_for and get_endpoint_from_catalog https://review.opendev.org/675130 | |
| 12:14:13 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Add support for global_request_id https://review.opendev.org/674807 | |
| 12:14:13 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Validate that connect_as connects as the project https://review.opendev.org/675178 | |
| 12:16:15 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Rationalize endpoint_for and get_endpoint_from_catalog https://review.opendev.org/675130 | |
| 12:16:15 | openstackgerrit | Monty Taylor proposed openstack/openstacksdk master: Replace catalog-v3.json with keystoneauth fixture https://review.opendev.org/675187 | |
| 12:16:34 | mordred | sorry for the churn - I thought I could pull one of those patches out to be independent - but turns out I couldn't | |
| 12:40:52 | openstackgerrit | Maxim Babushkin proposed openstack/openstacksdk master: Add 'tag' support to compute with supported microversion https://review.opendev.org/675306 | |
| 13:03:15 | openstackgerrit | Dmitry Tantsur proposed openstack/openstacksdk master: Add support for fields in baremetal get_* resources https://review.opendev.org/676154 | |
| 13:49:07 | efried | dtantsur: I was going to hit you up about that | |
| 13:49:23 | efried | I was looking into it yesterday | |
| 13:49:31 | dtantsur | I hope I didn't step on your shoes | |
| 13:49:46 | efried | just got to my desk, so haven't caught up, except to see that you commented on the nova patch and seem to have identified the/a problem | |
| 13:49:57 | dtantsur | yeah | |
| 13:50:01 | efried | from what I could tell, it was happening during construction of the proxy | |
| 13:50:16 | efried | is that the case? | |
| 13:50:57 | dtantsur | efried: ignore the version blah-blah logging. the actual problem seems to be get_node(<>, fields=..). we never implemented the "fields" bit. | |
| 13:51:06 | dtantsur | my patch above hopefully adds its support | |
| 13:51:27 | efried | dtantsur: Okay, the thing that concers me is... | |
| 13:52:56 | efried | dtantsur: Okay, here | |
| 13:52:59 | efried | https://logs.opendev.org/99/642899/38/check/ironic-tempest-ipa-wholedisk-bios-agent_ipmitool-tinyipa/665016d/controller/logs/screen-n-cpu.txt.gz?level=ERROR | |
| 13:53:35 | dtantsur | "StrictVersion instance has no attribute 'version'" is an annoying exception that *likely* hides the fact that ironic is not up yet | |
| 13:53:47 | efried | Even when this job succeeds, the "Error: StrictVersion instance has no attribute 'version'" always shows up, as does the "VirtDriverNotReady: Virt driver is not ready." | |
| 13:53:52 | dtantsur | the actual problem that fails you https://logs.opendev.org/99/642899/38/check/ironic-tempest-ipa-wholedisk-bios-agent_ipmitool-tinyipa/665016d/controller/logs/screen-n-cpu.txt.gz?level=ERROR#_Aug_13_05_19_11_540713 | |
| 13:54:02 | efried | And we always see at least one "No compute node record for host" | |
| 13:54:07 | dtantsur | efried: right, it's a bug in ironicclient that hides the ConnectionRefused or whatever | |
| 13:54:18 | dtantsur | because ironic starts later than nova | |
| 13:54:25 | efried | right, so | |
| 13:54:26 | dtantsur | I may be confusing some details, but the overall picture is like this | |
| 13:54:40 | efried | this error didn't start appearing in this patch until the latest PS | |
| 13:54:46 | efried | let me show you a log from before... | |
| 13:55:41 | dtantsur | I've seen it many times already. Not sure if it's permanent or depends on some ordering in devstack. | |
| 13:57:07 | efried | Yeah, I'm just using it as a baseline. | |
| 13:57:07 | efried | dtantsur: So this is what I consider "good": https://logs.opendev.org/99/642899/36/check/ironic-tempest-ipa-wholedisk-bios-agent_ipmitool-tinyipa/4b528c1/controller/logs/screen-n-cpu.txt.gz?level=ERROR | |
| 13:57:07 | efried | That job succeeded ^ | |
| 13:57:12 | dtantsur | same problem in a recent successful run: https://logs.opendev.org/40/675940/1/check/ironicclient-tempest/f57c0f3/controller/logs/screen-n-cpu.txt.gz?level=WARNING#_Aug_12_16_38_40_790400 | |
| 13:57:27 | dtantsur | right | |
| 13:57:48 | efried | So now comparing against the run in the same time frame on the *subsequent* patch, whose only difference is that it starts using .nodes... | |
| 13:58:18 | efried | https://logs.opendev.org/27/656027/20/check/ironic-tempest-ipa-wholedisk-bios-agent_ipmitool-tinyipa/ed7caf9/controller/logs/screen-n-cpu.txt.gz?level=ERROR | |
| 13:58:34 | efried | Now all of a sudden we're seeing this: Error: The baremetal service for 213.32.77.113:RegionOne exists but does not have any supported versions.: NotSupported: The baremetal service for 213.32.77.113:RegionOne exists but does not have any supported versions. | |
| 13:58:47 | efried | which comes from _make_proxy | |
| 13:58:53 | efried | i.e. when we first bootstrap the connection | |
| 13:59:22 | efried | which implies to me that the *previous* succeeding run wasn't hitting the changed code at all, which would be... bad :( | |
| 14:00:00 | dtantsur | we better ask mordred but I suspect "does not have any supported version" may be the consequence of ironic being down still | |
| 14:00:29 | efried | That never appears in any of the other runs though | |
| 14:00:39 | efried | I looked through a dozen yesterday | |
| 14:00:46 | efried | or | |
| 14:01:52 | efried | I guess it could be timing. If the code path that hits .nodes() happens much earlier than get_node(), and the service is still down, and we try to make the proxy and fail discovery... but we should try again a bit later, shouldn't we?? | |
| 14:02:14 | dtantsur | I guess we do? I'd fix the get_node problem first, then see where we stand | |
| 14:03:21 | efried | dtantsur: Yeah, see, the next patch in the series *should* be hitting that same get_node error, but it's not getting that far: https://logs.opendev.org/27/656027/21/check/ironic-tempest-ipa-wholedisk-bios-agent_ipmitool-tinyipa/60265c4/controller/logs/screen-n-cpu.txt.gz?level=ERROR | |
| 14:04:05 | dtantsur | it cannot connect to keystone, maybe that's the cause? | |
| 14:04:09 | efried | I suspect this just means the nova side needs to be more tolerant of service-not-yet-up. | |
| 14:04:15 | dtantsur | yep | |
| 14:08:51 | openstackgerrit | Merged openstack/openstacksdk master: DRY test_connection https://review.opendev.org/676009 | |
| 14:48:35 | efried | dustinc: ^ | |
| 14:52:16 | dustinc | efried, dtantsur: thanks, will catch up when I get to work in about an hour | |
| 14:57:14 | dtantsur | mordred, efried, https://review.opendev.org/#/c/676154/ passed now, please check | |
| 15:14:40 | openstackgerrit | Merged openstack/openstacksdk master: Fix discovery cache sharing https://review.opendev.org/675121 | |
| 15:35:31 | efried | dtantsur: +A, thanks for the quick fix. | |
| 15:36:28 | dtantsur | yvw | |
| 15:47:55 | efried | dtantsur, mordred: still trying to nail down the difference in the discovery path between ksa and sdk | |
| 15:48:08 | efried | specifically here https://logs.opendev.org/27/656027/21/check/ironic-tempest-ipa-wholedisk-bios-agent_ipmitool-tinyipa/60265c4/controller/logs/screen-n-cpu.txt.gz?level=WARNING#_Aug_13_04_59_18_053245 | |
| 15:48:38 | efried | In this case we want ServiceUnavailable to be an exception, but I *think* it's proceeding and still trying to return a real proxy. | |
| 15:49:04 | efried | I think allow_version_hack=False would get us DiscoveryFailure here | |
| 15:49:41 | efried | ...but I'm not sure whether that would help :( | |
| 15:55:04 | efried | mordred: I... kind of suspect we should be defaulting allow_version_hack=False from the sdk side. AFAICT it's always True today - even for the code paths where we're not explicitly setting it. | |
| 16:23:00 | efried | dustinc: When you're about, we should talk. | |
| 16:29:14 | dustinc | efried: around now | |
| 16:29:55 | efried | dustinc: The nova/sdk series needs to be passing the ironic CI job before it's going to fly | |
| 16:30:25 | efried | before yesterday, with the fields thing, the bottom patch was passing, but the second one (and up) was failing. | |
| 16:30:35 | efried | and I still haven't been able to figure out exactly why. | |
| 16:34:31 | dustinc | Thanks, I have not been giving it a lot of attention recently but am working on it today | |
| 16:35:40 | efried | dustinc: In the CI env, the n-cpu service comes up before ironic | |
| 16:35:47 | efried | so the first few attempts to connect will fail. | |
| 16:35:57 | efried | This is a "normal" run: https://logs.opendev.org/40/675940/1/check/ironicclient-tempest/f57c0f3/controller/logs/screen-n-cpu.txt.gz?level=WARNING#_Aug_12_16_38_40_790400 | |
| 16:37:07 | efried | And that's what things would look like, more or less, on the bottom patch (before the fields thing) | |
| 16:37:24 | dustinc | thanks, checking it out right now | |
| 16:37:36 | efried | But then with the second patch, we would start seeing stuff like this https://logs.opendev.org/27/656027/21/check/ironic-tempest-ipa-wholedisk-bios-agent_ipmitool-tinyipa/60265c4/controller/logs/screen-n-cpu.txt.gz?level=WARNING#_Aug_13_04_59_18_053245 | |
| 16:37:37 | dustinc | (I knew about the fields support thing but totally forgot and thus added it back..) | |
| 16:38:12 | efried | My current suspicion is that it's because we're using different retries/timeout than before. | |
| 16:38:21 | dustinc | Hmm | |
| 16:46:25 | efried | dtantsur|afk, dustinc, mordred: I think now that we have retry opts in ksa config [1][2], we should deprecate nova's equivalents [3] via alias so that they're used for both the old (iroincclient) and new (sdk) code paths, which will assure us parity at least in that regard. | |
| 16:46:25 | efried | [1] https://review.opendev.org/666287 | |
| 16:46:25 | efried | [2] https://review.opendev.org/672930 | |
| 16:46:25 | efried | [3] https://opendev.org/openstack/nova/src/branch/master/nova/conf/ironic.py#L39-L63 | |
| 16:59:53 | openstackgerrit | Merged openstack/openstacksdk master: Add support for fields in baremetal get_* resources https://review.opendev.org/676154 | |
| 17:00:11 | openstackgerrit | Eric Fried proposed openstack/openstacksdk master: Bump keystoneauth1 minimum to 3.16.0 https://review.opendev.org/676247 | |
| 17:00:36 | efried | mordred, Shrews: I'm going to propose a sdk release now, mkay? | |
| 17:00:45 | efried | accumulated a bunch of fixes nova (at least) needs. | |
| 17:01:11 | mordred | efried: ++ | |