Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-10-11
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
22:15:57 mriedem task_state=(None,)
22:16:02 mriedem instance.task_state not in task_state):
22:16:02 mriedem if (task_state is not None and
22:16:17 dansmith okay
22:16:57 mriedem so unrelated,
22:17:12 mriedem but now i realize that if your instance was stopped and it was successfully evacuated, it's now running
22:17:31 mriedem we don't stop it again like we do for resizing/cold migrating a stopped instance
22:17:57 mriedem which, as it turns out, was my first major bug fix in nova :)
22:18:00 mriedem remember?!
22:18:38 dansmith oh I remember
22:20:00 dansmith okay so nothing to change on this?
22:22:36 dansmith also I'd like to note that this migration unfuckification has been the least fun thing to work on since, oh I dunno, the great flavor migration of 2015
22:22:50 mriedem yeah i just +Wed
22:23:37 mriedem appears CI just exploded
22:25:48 dansmith we _can_ do a migration-holding source allocation if you want,
22:26:23 dansmith but it's work for no good reason, IMHO,
22:26:33 mriedem yeah, that's why i said if/when we need to
22:26:43 dansmith okay
22:26:55 mriedem because that would mean the cleanup if source comes back up thing would have to deal with whether or not the allocation is on the migration or not
22:27:03 dansmith right
22:27:20 mriedem and that would definitely be
22:27:23 mriedem say it with me now
22:27:30 dansmith I'm a little concerned that we have this reset-state-hard-reboot path to keep these on the source even after an evac has been triggered once
22:28:15 mriedem wonky

Earlier   Later