Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-10-11
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 "host that might need to be removed: %s.",
22:02:21 mriedem "There are allocations remaining against the source "
22:02:21 mriedem LOG.debug("Instance %s has been moved to another host %s(%s). "
22:02:21 mriedem and we'd log
22:03:09 mriedem and those allocatoins remaining on the source would get cleaned up if/when the source comes back up and deletes them
22:03:15 mriedem and if it doesn't come back up, we don't care
22:03:45 dansmith ack yeah
22:03:46 mriedem if this runs after instance.host=$us, then we hit if consumer_uuid in known_instances: and we don't remove allocations
22:04:45 mriedem so i think we're ok
22:05:37 mriedem and i think we should probably add 'done' to https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L4863 in some other unrelated change, probably just because someone forgot about it before, but 'done' is definitely not in-progres
22:05:40 dansmith mriedem: so, we _are_ passing the functional tests that cover evac.. the only thing that might be missing is source compute coming back or something
22:06:07 mriedem i know we have some functional tests for ^ because they are asserting the source removes the allocations from the source after it's moved
22:06:36 dansmith I'm not really sure why accepted should be in there, tbh
22:06:42 dansmith since it's very much in progress
22:06:54 mriedem https://github.com/openstack/nova/blob/master/nova/tests/functional/test_servers.py#L1658
22:07:15 mriedem yeah not sure about accepted being in there either
22:07:33 mriedem so ^ is the test that once you have evacuated to the dest and start up the source, the allocations are removed from the source
22:07:44 dansmith ah yeah okay
22:07:51 mriedem which is actually currently broken :)
22:08:05 mriedem https://review.openstack.org/#/c/510176/
22:08:13 mriedem mayhap you'd like to +W that
22:08:28 mriedem our code to remove the source allocations on startup after evac is not working
22:09:17 dansmith so explain to me again how you can evacuate from a source, go to error, and then start back up on the source successfully?
22:09:41 dansmith we should be caught by the vm_state==ERROR skip clause no?
22:10:26 SamYaple if its in an errored state and you reset it to state active then do a hard-reboot on it (assuming shared storage) it should come back
22:11:46 dansmith SamYaple: yeah, reset-state means you're cheating and other things are likely going to be out of sync
22:12:26 SamYaple dansmith: fair enough. im just saying what would typically get done in ops
22:12:32 SamYaple im not advocating for this
22:12:45 dansmith SamYaple: ack, yeah, I was thinking mriedem was saying there's a path out of the forest for this by a user
22:12:50 mriedem dansmith: do we set the instance to error state on failed evac?
22:13:00 dansmith mriedem: if we didn't you couldn't call evac on it again
22:13:09 dansmith and you said we could
22:13:28 mriedem i don't see that we change the vm_state on evac
22:13:36 dansmith so we leave it as rebuilding?
22:13:47 mriedem no
22:13:48 mriedem def evacuate(self, context, instance, host, on_shared_storage,
22:13:48 mriedem vm_states.ERROR])
22:13:48 mriedem @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED,
22:13:56 mriedem i think it would be active
22:14:05 mriedem well, on a successful evac it would be active
22:14:18 mriedem if it started as error and evac failed, it'd stay in error
22:14:26 dansmith oh task_state=rebuilding
22:14:29 mriedem yar
22:14:29 dansmith so um
22:14:40 dansmith that means I can re-evac while an evac is going?
22:14:45 dansmith that'd be fun
22:14:49 mriedem no
22:15:04 mriedem i think @check_instance_state defaults to not allow doing things if the task_state is set
22:15:18 mriedem and we set the task_state to rebuilding in the api once you've started an evac
22:15:21 dansmith does that decorator imply no task state if not specified?
22:15:22 dansmith okay
22:15:53 mriedem yeah it's weird and confusing to read

Earlier   Later