| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-11 | |||
| 21:27:59 | dansmith | regardless, I don't think this affects the new-world path | |
| 21:28:05 | mriedem | we never remove the allocation from the source node for an evacuation | |
| 21:28:15 | mriedem | except when the source node comes back, if it does | |
| 21:28:18 | openstackgerrit | Hongbin Lu proposed openstack/nova master: placement: add API reference for create inventory https://review.openstack.org/511342 | |
| 21:28:29 | dansmith | you think we never do it now? | |
| 21:28:47 | dansmith | or you think we never will with the new-world way? | |
| 21:28:51 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L680 | |
| 21:28:55 | dansmith | ah, maybe your point is, | |
| 21:29:07 | mriedem | it's going to be a weird side thing | |
| 21:29:08 | dansmith | in the new world way we'll still end up calling the doubler? | |
| 21:29:15 | mriedem | it's a move operation where the source allocation isn't on the migration uuid | |
| 21:29:43 | mriedem | unlike live migrate, cold migrate/resize | |
| 21:29:43 | dansmith | can't we just stop doing the doubling across the board? | |
| 21:29:50 | dansmith | just delete it before we call claim_resources | |
| 21:29:57 | mriedem | for evac? | |
| 21:30:01 | dansmith | that should work for both old and new paths | |
| 21:30:02 | dansmith | yeah | |
| 21:30:08 | mriedem | well, unless evac fails on the dest | |
| 21:30:13 | mriedem | and the instance never moved | |
| 21:30:15 | dansmith | what does it matter? you're not going back | |
| 21:30:34 | dansmith | we've created the migration, the source node is going to nuke it when it wakes up, per the rules | |
| 21:30:42 | mriedem | not necessarily | |
| 21:30:43 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L643 | |
| 21:30:56 | mriedem | the source only cleans up if the move completed, or is in progress | |
| 21:32:03 | dansmith | um | |
| 21:32:24 | dansmith | the whole point of that robustification was to not do that, | |
| 21:32:32 | dansmith | else we'll race with the operation finishing | |
| 21:32:46 | dansmith | I don't see where we're setting =accepted anymore actually | |
| 21:32:53 | mriedem | conductor | |
| 21:32:56 | mriedem | er api | |
| 21:33:00 | dansmith | I don't see it | |
| 21:33:11 | dansmith | ah I see | |
| 21:33:20 | mriedem | yeah so the api creates the migration record in 'accepted' status, | |
| 21:33:22 | dansmith | right, so that happens synchronously with the api call | |
| 21:33:28 | dansmith | right? | |
| 21:33:29 | mriedem | if the evac is successful on the dest, the status goes to 'done' | |
| 21:33:46 | dansmith | so if the evac api call returns, you know it's never starting on the source again right? | |
| 21:34:00 | dansmith | we do that before we create the new allocation, | |
| 21:34:05 | dansmith | so if we have saved that in the db, | |
| 21:34:10 | dansmith | we know we can delete the old allocation without caring | |
| 21:34:19 | dansmith | because at that point, the source host will never take it back | |
| 21:34:27 | mriedem | not necessarily :) | |
| 21:34:56 | mriedem | if conductor/scheduler fails to find a host, the migration status for the evac is marked 'error' | |
| 21:35:02 | mriedem | and the instance is still on the source | |
| 21:35:05 | mriedem | with no allocation on the dest | |
| 21:35:16 | mriedem | https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L871 | |
| 21:35:26 | mriedem | this is all related to that thing gibi and i were talking about before the ptg | |
| 21:35:33 | mriedem | handling allocations for failed evacs | |
| 21:35:41 | dansmith | right and I was trying to say we shouldn't ever let it go back | |
| 21:35:54 | dansmith | so we should include error in the list of states we use in compute right? | |
| 21:36:20 | dansmith | because the recovery path should be re-rebuilding that instance | |
| 21:36:58 | mriedem | rebuilding the instance on the source won't recreate the allocations in placement | |
| 21:37:12 | mriedem | b/c rebuild skips the scheduler, which skips the claim | |
| 21:37:24 | dansmith | okay, I guess, yeah | |
| 21:37:26 | dansmith | regardless, | |
| 21:37:37 | mriedem | so if you didn't move the instance, but you delete the allocations on the source when starting up, rebuild won't help you with the allocations | |
| 21:37:49 | dansmith | I really think we should avoid the source coming back up and re-owning the instance because I think it's going to be a likely source of races | |
| 21:38:31 | dansmith | the people that use this for HA stuff hammer on this pretty hard from scripts and I think the behavior needs to be as predictable and linear as possible | |
| 21:39:51 | mriedem | ok, well, it seems that requires more thought to determine if we change how evac works if the source comes back up and the instance didn't move, or if we should move source node allocations to a migration record during evac, | |
| 21:40:03 | mriedem | all things i can't really sort out in my head right now with maya in my office reading her homework... | |
| 21:40:28 | dansmith | so, we don't clear the instance.host in evacuate for some reason | |
| 21:40:48 | mriedem | we just update it once evac is successful | |
| 21:41:03 | dansmith | if we did, it would make it easier to just let people retry with an evac again if it failed the first time | |
| 21:41:09 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2841 | |
| 21:41:14 | dansmith | yeah | |
| 21:41:34 | mriedem | you can already retry an evac if the first one failed | |
| 21:41:43 | dansmith | not if thecompute host came backup | |
| 21:41:53 | dansmith | because we still have instance.host set and require it be down | |
| 21:42:01 | dansmith | right? | |
| 21:42:12 | dansmith | the source I mean | |
| 21:42:23 | mriedem | well, if we nulled out instance.host, shit gets all sorts of wonky because you then have to deal with local delete in the api bullshit | |
| 21:42:37 | mriedem | wonkified | |
| 21:43:03 | mriedem | i think i've overshot the original problem we were talking about that started this :) | |
| 21:43:21 | mriedem | the assertion that RT doesn't have to care about migration allocations during evac | |
| 21:43:24 | dansmith | so, in your case, | |
| 21:43:39 | dansmith | let's say we evac, fail, leave a doubled allocation today, | |
| 21:43:46 | dansmith | then evac again, what do we do, triple it? | |
| 21:44:49 | mriedem | no, we cleanup the allocations for the failed dest host | |
| 21:44:59 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2831 | |
| 21:45:06 | mriedem | https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2817 | |
| 21:45:15 | mriedem | those are for if the actual spawn failed, or the claim on dest failed | |
| 21:45:27 | mriedem | remember me freaking out on you and jay about this a couple of weeks ago? :) | |
| 21:45:39 | mriedem | so i think we've got that covered | |
| 21:45:43 | mriedem | the cleanup i mean | |
| 21:45:58 | mriedem | and if we failed to pick a host or claim in conductor, we don't have anything to cleanup anyway | |
| 21:46:32 | dansmith | I've lost sight of the thing you think is wrong in the new case | |
| 21:46:41 | mriedem | i'm not saying it's wrong, | |
| 21:46:57 | mriedem | but this code in the RT can pull migrations that are 'done' | |
| 21:47:30 | mriedem | and that db api query is not making a distinction on if the current node is the migration's source or dest | |
| 21:47:39 | dansmith | which would cause it to skip allocations at most, yes? | |
| 21:47:42 | mriedem | maybe we should just fix the db api filter to include 'done'? | |
| 21:47:57 | dansmith | until the source comes up and marks it finished or whatever | |
| 21:48:09 | mriedem | i think so... | |
| 21:48:19 | mriedem | well, that's why i asked for you to double check my thinking here | |
| 21:48:24 | mriedem | because this is weird | |
| 21:48:26 | dansmith | but it's an instance on our host, so we would skip it for that reason anyway | |
| 21:48:33 | dansmith | mriedem: yeah, how'd that work out for you? :) | |
| 21:48:45 | mriedem | i feel better | |
| 21:49:49 | dansmith | we could just filter out migrations that don't have us as the source or the right state since we have to iterate them anyway, | |
| 21:49:58 | dansmith | but I'm not sure it's required | |