| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-11 | |||
| 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 | |
| 21:50:28 | dansmith | are you concerned about inbound or outbound migrations being included? | |
| 21:50:56 | dansmith | you said "source and dest" but didn't emphasize either, so I'm not sure which you're concerned about | |
| 21:53:01 | mriedem | yeah i'm unclear on which we care about here too | |
| 21:53:17 | mriedem | i would think source since migration allocations should only be for source | |
| 21:53:38 | dansmith | source of an evacuation, coming back up early? | |
| 21:54:22 | dansmith | currently it'll still count those instances as its own and not delete their allocations until .host flips | |
| 21:57:56 | mriedem | ok so in https://review.openstack.org/#/c/506419/19/nova/compute/resource_tracker.py | |
| 21:58:10 | mriedem | if we assume we're on the dest node, because the source is dead during an evac | |
| 21:58:48 | mriedem | and the migration status is done | |
| 21:58:54 | dansmith | migration is in accepted early, | |
| 21:58:59 | dansmith | then done once it's on our node | |
| 21:59:01 | mriedem | migration_get_in_progress_by_host_and_node could return it and the migration.dest_compute would be our local RT | |
| 21:59:09 | dansmith | then we're not the dest node anymore because instance.host=$us | |
| 21:59:43 | dansmith | i.e. if this runs before evac completes, it's still in accepted state | |
| 21:59:56 | mriedem | correct, and the migration wouldn't be returned | |
| 21:59:59 | mriedem | from migration_get_in_progress_by_host_and_node | |
| 22:00:05 | mriedem | because: filter(~models.Migration.status.in_(['accepted', 'confirmed', | |
| 22:00:38 | mriedem | if the migration status is 'done', then we've already set instance.host=$us | |
| 22:00:57 | dansmith | yeah, so if it runs early, we might nuke the doubled allocation yeah? | |
| 22:01:15 | mriedem | well i'm looking in _remove_deleted_instances_allocations now | |
| 22:01:20 | dansmith | not sure how we're not doing that already today though | |
| 22:01:48 | mriedem | if it runs before instance.host is set to the dest node, the instance shouldn't be in tracked_instances | |
| 22:02:06 | dansmith | right | |
| 22:02:12 | mriedem | and we'd get to if instance.host != cn.host: | |
| 22:02:14 | dansmith | but we have an allocation already because of conductor | |
| 22:02:21 | mriedem | and we'd log | |
| 22:02:21 | mriedem | LOG.debug("Instance %s has been moved to another host %s(%s). " | |