| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-01-24 | |||
| 13:09:29 | efried | rgerganov As written, that change will *not* refresh the tree, though | |
| 13:10:17 | efried | If you want a simple try-out, change to force=True | |
| 13:11:10 | rgerganov | efried, this loop doesn't run at all: for u in self._provider_tree.get_provider_uuids(uuid): | |
| 13:11:25 | rgerganov | because there are no children in the local tree | |
| 13:11:39 | efried | rgerganov That call should at least return `uuid` itself. | |
| 13:11:48 | rgerganov | ah, correct | |
| 13:11:53 | rgerganov | my bad | |
| 13:12:00 | efried | And then the _refresh_associations call should re-grab all the tree-associated providers | |
| 13:12:23 | efried | But possibly only if the cache timeout has occurred. | |
| 13:12:32 | efried | Which may actually be the issue. | |
| 13:12:54 | efried | So we'll refresh the root there, but then on the recursive part of the call, we'll check the cache timeout map | |
| 13:13:21 | efried | ...which will still have stale entries because we only removed the nodes from the ProviderTree; we didn't remove the cache timeout entry for the descendants. | |
| 13:14:02 | efried | So force=True as mentioned above would work; but the more correct solution would be to remove all those other association refresh times when we invalidate that root from the cache. | |
| 13:14:06 | rgerganov | efried, do you want me to try it? | |
| 13:14:22 | efried | rgerganov If you've got a setup all ready to go, please do. It would take me some time to get to that point. | |
| 13:14:32 | efried | rgerganov Meanwhile, let me come up with the other solution real quick... | |
| 13:14:33 | rgerganov | ok | |
| 13:17:53 | rgerganov | efried, with force=True it refreshed the associations for the root provider but it didn't add the nested RP in the tree | |
| 13:20:06 | efried | rgerganov At a glance, I don't see (and I don't offhand remember) how tree-associated providers get refreshed. While I poke at that, would you please try this: https://review.openstack.org/#/c/533821/7/nova/scheduler/client/report.py@1381 | |
| 13:21:19 | rgerganov | efried, ok, trying | |
| 13:21:20 | efried | Okay, that's not gonna work either. | |
| 13:21:28 | efried | But try it anyway, for grins. | |
| 13:21:33 | openstackgerrit | Takashi NATSUME proposed openstack/nova master: Transform instance.resize_prep notification https://review.openstack.org/465081 | |
| 13:24:47 | efried | rgerganov https://review.openstack.org/#/c/526541/19/nova/scheduler/client/report.py@631 and @639 are the things that *should* be repopulating the _provider_tree from placement. | |
| 13:25:00 | rgerganov | efried, AttributeError: 'SchedulerReportClient' object has no attribute 'association_refresh_time' | |
| 13:25:17 | efried | rgerganov Oh, are you sitting on the very top of the series? | |
| 13:25:29 | efried | Or did I forget the underscore :) | |
| 13:27:33 | openstackgerrit | Gao Fei proposed openstack/nova master: Update VMWare vSphere link address https://review.openstack.org/535244 | |
| 13:27:58 | gibi | is it only me or gerrit is slow like hell today | |
| 13:27:58 | rgerganov | efried, my patches are based on top of "Move refresh time from report client to prov tree" | |
| 13:28:01 | gibi | ? | |
| 13:28:11 | rgerganov | gibi, yes, it's really slow | |
| 13:28:13 | efried | gibi Not just you. Slow as hell. Started grinding yesterday. | |
| 13:28:21 | efried | Maybe ask -infra to bounce it. | |
| 13:28:26 | gibi | thanks for the info | |
| 13:28:53 | efried | rgerganov Okay, that patch got rid of _association_refresh_time in the report client, and would have made my suggested fix moot anyway. | |
| 13:29:51 | efried | rgerganov So what I can't figure out is why, in _ensure_resource_provider, the _get_providers_in_tree => populate_from_iterable isn't restoring the descendants to the cache. | |
| 13:30:03 | efried | Are you pdb'ing? | |
| 13:30:07 | rgerganov | efried, yes | |
| 13:30:58 | efried | Can you break at https://review.openstack.org/#/c/536902/1/nova/scheduler/client/report.py@626 at make sure rps_to_refresh contains all the descendants? | |
| 13:31:24 | rgerganov | with or without my patch? | |
| 13:31:35 | efried | Shouldn't matter. | |
| 13:31:52 | rgerganov | ok, tracing | |
| 13:32:13 | efried | Because as it turns out, _refresh_associations isn't where we pick up tree-associated providers. It's in fact in this chunk of _ensure_resource_provider. | |
| 13:33:43 | rgerganov | efried, it does matter because without my patch we don't get there | |
| 13:34:03 | efried | Oh, because 'return uuid'... | |
| 13:34:07 | rgerganov | correct | |
| 13:34:24 | efried | ...but that .exists(uuid) should be False! | |
| 13:34:28 | efried | Because we removed that guy. | |
| 13:34:40 | rgerganov | no, we pass the parent uuid here | |
| 13:34:45 | rgerganov | and it is there | |
| 13:35:19 | artom | Am I doing this right? http://logstash.openstack.org/#dashboard/file/logstash.json?query=libvirtError%3A%20Cannot%20recv%20data | |
| 13:35:30 | rgerganov | efried, _ensure_resource_provider is called with the uuid of the root RP | |
| 13:35:45 | artom | It says 0 hits, but we clearly got some here: http://logs.openstack.org/97/536897/2/check/legacy-tempest-dsvm-cells/fc9986a/logs/screen-n-cpu.txt.gz?level=ERROR#_Jan_23_23_16_41_428802 | |
| 13:35:53 | efried | rgerganov I see, and you were trying to add the bogus trait to the child? | |
| 13:36:02 | rgerganov | efried, correct | |
| 13:36:05 | efried | So we invalidated the child, but not the root. | |
| 13:36:11 | rgerganov | yup | |
| 13:36:13 | efried | Okay, it's coming together. | |
| 13:36:44 | rgerganov | could you please tell why my patch won't refresh the tree? | |
| 13:37:52 | efried | rgerganov No I can't. It should. | |
| 13:38:04 | efried | oh | |
| 13:42:17 | efried | rgerganov So with your patch, is rps_to_refresh populated at that breakpoint? | |
| 13:43:26 | rgerganov | efried, yes | |
| 13:43:46 | efried | rgerganov And after populate_from_iterable, is the _provider_tree populated properly? | |
| 13:43:47 | rgerganov | it contains both parent and child | |
| 13:44:05 | rgerganov | as far as I can see, yes | |
| 13:44:29 | mvenesio | Hi guys i'm trying to set nova to use SSL for the database connection, i set the mysql connection as well as i set it for the rest of the projects like cinder and glance, but for nova it does not work and i got an OperationalError. Any idea about how to set it right ? | |
| 13:45:49 | efried | mvenesio I think that's a better question for #openstack (see channel topic). | |
| 13:46:10 | efried | rgerganov So if the child is in the cache... where do we run into a problem? | |
| 13:46:17 | openstackgerrit | Lee Yarwood proposed openstack/nova master: rbd: flatten images when creating/unshelving an instance https://review.openstack.org/457886 | |
| 13:46:36 | lyarwood | melwitt: https://review.openstack.org/#/c/457886/ ^ I wonder if this is something we could land by the rc | |
| 13:47:26 | mvenesio | efried: ok i'll do it, thanks | |
| 13:47:27 | rgerganov | efried, the child gets in the cache with my patch and then we don't have a problem (at least not now :) ) | |
| 13:48:20 | efried | rgerganov I must have misunderstood "could you please tell why my patch won't refresh the tree?" | |
| 13:49:38 | rgerganov | efried, I asked that because you said "As written, that change will *not* refresh the tree, though" | |
| 13:49:40 | rgerganov | nevermind | |
| 13:50:08 | efried | rgerganov Okay, yeah, I was wrong. I had missed the 'return uuid' part and was misunderstanding my own code :( | |
| 13:51:06 | efried | rgerganov So your solution will work, but it's skipping a nontrivial optimization that I would like to keep if possible. | |
| 13:52:13 | efried | rgerganov One possible alternative would be for that cache invalidation to in fact kill the whole tree. But the implications of that are probably too far-reaching to be practical. | |
| 13:52:58 | efried | rgerganov Other than that patch, do you have any local code on the series? | |
| 13:53:19 | efried | rgerganov In particular, we should definitely commit whatever test case you're running to hit this problem. | |
| 13:53:30 | openstackgerrit | Merged openstack/nova master: Transform instance.resize_confirm notification https://review.openstack.org/482557 | |
| 13:53:45 | efried | rgerganov Though I would like to do it as part of the update_from_provider_tree patch. | |
| 13:54:13 | rgerganov | efried, so basically the test case should be virt driver adding an incorrect trait | |
| 13:54:33 | efried | rgerganov ...to a child | |
| 13:54:36 | efried | right? | |
| 13:54:39 | rgerganov | yes | |
| 13:55:03 | efried | rgerganov How close to your EOD are you? | |
| 13:55:16 | rgerganov | efried, I will head out in 2 hours | |
| 13:57:15 | efried | rgerganov So I've *almost* got this code path in my test_report_client work in progress. | |
| 13:57:56 | efried | rgerganov I've got one piece setting a bogus trait on the root. And another setting inventory in a bogus *resource class* on a descendant. The latter of which should *probably* have the same effect. | |
| 13:58:21 | efried | rgerganov But since we've identified this exact issue, it wouldn't go amiss to have a small, isolated test case that's separate from that big one. | |
| 13:58:46 | efried | rgerganov Do you have the time/inclination/know-how to write that test case before you leave? | |
| 13:58:57 | rgerganov | efried, I will give it a try | |
| 13:59:05 | efried | rgerganov Thanks! | |
| 13:59:22 | rgerganov | efried, do you want me to update an existing patch or start a new patch? | |
| 14:00:49 | mriedem | alex_xu: i'm fine with checking the version in the Selection object or just checking if the allocation request is the list or dict format and adjusting properly, up to you, but i think we have to handle both cases in queens, we can then probably remove that check later in rocky | |
| 14:02:24 | artom | mriedem, 'morning - did we figure out if the libvirt connection reset errors were a real bug or not? | |
| 14:02:29 | efried | rgerganov New one. We can always slot it into the series, or squash it into the existing commit. | |