| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-11 | |||
| 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). " | |
| 22:02:21 | mriedem | "There are allocations remaining against the source " | |
| 22:02:21 | mriedem | "host that might need to be removed: %s.", | |
| 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 | @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED, | |
| 22:13:48 | mriedem | vm_states.ERROR]) | |
| 22:13:48 | mriedem | def evacuate(self, context, instance, host, on_shared_storage, | |
| 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 | dansmith | so um | |
| 22:14:29 | mriedem | yar | |
| 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 | if (task_state is not None and | |
| 22:16:02 | mriedem | instance.task_state not in task_state): | |
| 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 | |