| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-21 | |||
| 19:08:54 | mriedem | also because takashi wants to do this same thing for cold migrations | |
| 19:09:06 | mriedem | well, takashi and others... | |
| 19:09:34 | dansmith | aye | |
| 19:10:13 | mriedem | so now i'm looking at what conductor does when there isn't a forced destination host https://github.com/openstack/nova/blob/16.0.0.0rc1/nova/conductor/tasks/live_migrate.py#L153 | |
| 19:10:38 | mriedem | the request spec stuff gets pretty confusing in here | |
| 19:10:44 | mriedem | for compat things | |
| 19:11:24 | mriedem | we wouldn't call _find_destination i realize, | |
| 19:11:43 | mriedem | i'm just not sure how much we need to pass in the existing request spec with some "use this host, seriously" field set | |
| 19:13:42 | mriedem | would the scheduler also run the filters on the forced host in this case? | |
| 19:14:49 | mriedem | that might take the 'force-ness' out of it a bit if you fail to force because of a scheduler filter - but would allow us to remove this code https://github.com/openstack/nova/blob/16.0.0.0rc1/nova/conductor/tasks/live_migrate.py#L103 | |
| 19:14:59 | dansmith | mriedem: the scheduler won't know why you're running it | |
| 19:15:16 | dansmith | the point is, | |
| 19:15:32 | dansmith | the select_destinations() call should eliminate the need to manually check for memory space on the destination, | |
| 19:15:49 | mriedem | from conductor | |
| 19:15:51 | mriedem | i agree | |
| 19:15:53 | dansmith | because if it doesn't return the host you asked for then you fail with "sorry this peg doesn't fit in that hole" | |
| 19:15:55 | dansmith | yeah | |
| 19:16:03 | mriedem | yeah which is nice | |
| 19:16:07 | mriedem | removes my issue with that code not using placement | |
| 19:17:21 | mriedem | so it seems all we need to do then is have conductor set force_hosts/nodes on the request spec and call select_destinations - if that raises NoValidHost, we know what to do | |
| 19:17:47 | mriedem | i seriously feel like if i try to monkey with the request spec there is some landmine that will blow off at least 2 fingers | |
| 19:18:39 | dansmith | hah | |
| 19:18:54 | mriedem | this could also go away assuming you have the ComputeFilter enabled https://github.com/openstack/nova/blob/16.0.0.0rc1/nova/conductor/tasks/live_migrate.py#L85 | |
| 19:22:07 | mriedem | i think it's basically this that we need https://github.com/openstack/nova/blob/16.0.0.0rc1/nova/compute/api.py#L3880-L3884 | |
| 19:26:52 | mriedem | god, why doesn't this check if self.requested_destination is set when converting down to a legacy filter_properties dict? https://github.com/openstack/nova/blob/16.0.0.0rc1/nova/objects/request_spec.py#L347 | |
| 19:27:04 | mriedem | requested_destination essentially supersedes force_hosts/nodes doesn't it? | |
| 19:30:32 | dansmith | mriedem: afaik yeah | |
| 19:31:04 | mriedem | weird, ok, because it's not consulted at all, from what i can tell, when converting a request spec to an older version | |
| 19:32:10 | mriedem | oh you know what else, this bypasses our same-cell check | |
| 19:32:21 | mriedem | so you could force a host for live migration in another cell | |
| 19:33:39 | mriedem | this force live migration thing is all sorts of f'ed | |
| 19:35:02 | dansmith | yeah, so all the more reason to let it do the cell and memory check in one go of select_destinations right? | |
| 19:35:23 | mriedem | yeah, although i don't see where in the scheduler we check that the requested destination is in the same cell... | |
| 19:35:44 | mriedem | get_host_states_by_uuids ? | |
| 19:36:06 | mriedem | https://github.com/openstack/nova/blob/16.0.0.0rc1/nova/scheduler/host_manager.py#L635 | |
| 19:36:33 | dansmith | yeah | |
| 19:36:52 | mriedem | cool, ok, so i think this narrows things a bit | |
| 19:37:09 | mriedem | i think we are missing some stuff in the request spec backport to legacy format stuff, but i'm not messing with that | |
| 19:37:13 | mriedem | those are all bauzas questions | |
| 19:37:25 | mriedem | since it seems there are 4 ways to construct a request spec for the scheduler | |
| 19:39:03 | dansmith | yeah that whole mess is still...a mess | |
| 19:39:26 | mriedem | well it's 2:40pm on my first day back and i'm already knee deep in a mess that has to be fixed for rc2 | |
| 19:39:27 | mriedem | yay! | |
| 19:40:23 | dansmith | well, | |
| 19:40:42 | dansmith | presumably we _could_ just document and fix post release if needed | |
| 19:40:48 | dansmith | if it helps | |
| 19:40:53 | mriedem | yeah i thought about that | |
| 19:40:57 | mriedem | known issue and all | |
| 19:41:00 | mriedem | "known regression" | |
| 19:41:16 | mriedem | "at least we let you know about it beforehand, you're welcome" | |
| 19:41:21 | dansmith | we wouldn't want to wait long, of course, but.. | |
| 19:41:29 | dansmith | things could be worse | |
| 19:41:33 | mriedem | yeah | |
| 19:41:38 | mriedem | i.e. optional api | |
| 19:42:26 | mriedem | ok i'll wip something together and put it on top of https://review.openstack.org/#/c/495170/ and see what happens | |
| 19:43:04 | dansmith | okay | |
| 19:43:56 | openstackgerrit | Merged openstack/nova master: Correct statement in api-ref https://review.openstack.org/495724 | |
| 19:54:54 | mriedem | this was the other bug, I haven't triaged it yet https://bugs.launchpad.net/nova/+bug/1712045 | |
| 19:54:55 | openstack | Launchpad bug 1712045 in OpenStack Compute (nova) "nova doesn't clean up the resources after live migrate" [Undecided,New] | |
| 19:55:43 | mriedem | looks similar to what was needed with evacuate though https://review.openstack.org/#/c/494625/1/nova/compute/manager.py | |
| 19:55:51 | mriedem | remove the allocations for the instance and the source node | |
| 19:56:46 | dansmith | yeah | |
| 19:58:17 | dansmith | mriedem: that test for the bug you're working on has the post assertion for the destination's allocations commented out, | |
| 19:58:30 | dansmith | so fixing 1712045 will be required for that one too I think | |
| 20:06:02 | openstackgerrit | Merged openstack/nova master: doc: Address review comments for contributor index https://review.openstack.org/491517 | |
| 20:07:10 | mriedem | wtf, is local docs build blowing up a known issue? | |
| 20:07:25 | mriedem | https://gist.github.com/mriedem/4a6dcb52ed867af14f16989d7b83739a | |
| 20:18:40 | openstackgerrit | Matt Riedemann proposed openstack/nova master: doc: Address review comments for main index https://review.openstack.org/492645 | |
| 20:19:31 | mriedem | dansmith: i think we should probably get ^ to rc2 to fix the "OpenSack" thing in the first section you read in nova's docs | |
| 20:19:53 | dansmith | lol | |
| 20:26:45 | openstackgerrit | Ilya Popov proposed openstack/nova master: Tests: Add cleanup of 'instances' directory https://review.openstack.org/491589 | |
| 20:32:50 | mriedem | as for that live migration test, yeah it's a mix of both bugs, and the comments are wrong for some of the existing test | |
| 20:32:55 | mriedem | i'll update that test also | |
| 20:32:58 | dansmith | mriedem: well, I have the change made for the post-migration update I think, but I can't really use it until I have your patch to create the doubled allocation in the scheduler | |
| 20:33:11 | mriedem | cleaning up the test atm | |
| 20:33:12 | dansmith | otherwise I'm trying to push empty allocations to placement | |
| 20:33:16 | dansmith | no problem, just FYI | |
| 20:38:56 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add functional live migrate test https://review.openstack.org/495811 | |
| 20:38:57 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add functional force live migrate test https://review.openstack.org/495170 | |
| 20:45:25 | mriedem | ok +2 on both of the live migration functional tests | |
| 21:14:16 | mriedem | i was thinking that with setting RequestSpec.requested_destination we might end up bypassing the filtering for MEMORY_MB in placement, but looks like we do that first regardless of the requested/forced host | |
| 21:14:37 | mriedem | so we always call to placement to get the candidates, and then wittle that down based on forced hosts, and then further filter that using the filters | |
| 21:14:59 | mriedem | so we should be ok with removing that ram check in the live migration task in conductor | |
| 21:23:52 | cfriesen_ | mriedem: looking at your comments for https://bugs.launchpad.net/nova/+bug/1712008 I guess that actually calling the scheduler will end up filtering for cells as well? | |
| 21:23:53 | openstack | Launchpad bug 1712008 in OpenStack Compute (nova) pike "Force live migrate doesn't claim resources on the target host" [Critical,Triaged] | |
| 21:24:09 | mriedem | nope | |
| 21:24:20 | mriedem | because conductor doesn't set request_spec.requested_destination.cell | |
| 21:24:25 | mriedem | in the LiveMigrateTask | |
| 21:38:35 | cfriesen_ | mriedem: that bug is in the context of forcing a host...how is it even valid to claim resources when forcing a host? It could end up consuming resources that aren't available. | |
| 21:39:10 | cfriesen_ | mriedem: (when factoring in overcommit etc) | |
| 21:39:50 | mriedem | cfriesen_: pre-placement you'd end up claiming resources in the compute anyway | |
| 21:39:52 | mriedem | via the resource tracker | |
| 21:39:55 | mriedem | yo'ud just fail much later | |
| 21:40:38 | mriedem | but let me verify that first | |
| 21:42:57 | mriedem | hmm, yeah we don't call the resource tracker to make a claim during live migration... | |
| 21:43:33 | cfriesen_ | mriedem: that's part of the patch series that's been under review forever | |
| 21:44:00 | mriedem | so, we don't want to build more on claims in the computes, yes? | |
| 21:44:09 | mriedem | because the RT is a mess | |
| 21:44:41 | mriedem | and we are moving things to the scheduler so we can make more accurate decisions when building instances up front, rather than rely on reschedules | |
| 21:44:42 | mriedem | yes? | |