| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-27 | |||
| 18:16:03 | mriedem | wiping out all of the allocations for an instance because it's no longer on the source node | |
| 18:16:33 | dansmith | hmm, I thought not because of the claim at the end on the destination, but let me look | |
| 18:17:16 | dansmith | mriedem: yeah, we should check the allocations before we delete to see if we own any of them I think, or delete the ones that pertain to us | |
| 18:17:19 | dansmith | instead of just nuking them all | |
| 18:17:23 | dansmith | good call | |
| 18:20:16 | mriedem | it also seems that https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L1068 could simply updated to be: | |
| 18:20:25 | mriedem | if not instance.host or instance.host != CONF.host: | |
| 18:20:28 | mriedem | continue | |
| 18:21:24 | jaypipes | err, not sure about that... | |
| 18:21:39 | mriedem | otherwise yeah, pass the cn uuid to https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L999 and we could make sure to only remove allocations for the source node + instance | |
| 18:22:07 | mriedem | note that if we have to change anything here in the compute, it wouldn't be there for ocata computes | |
| 18:22:15 | jaypipes | mriedem: an allocation is an all-or-none thing, though. | |
| 18:22:31 | mriedem | jaypipes: i don't know what that means | |
| 18:22:47 | mriedem | we're doubling up allocations here https://review.openstack.org/#/c/487589/6/nova/scheduler/client/report.py | |
| 18:22:54 | mriedem | to maintain the source node allocations | |
| 18:23:07 | mriedem | but https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L1068 will clearly blast those away | |
| 18:23:08 | jaypipes | mriedem: You can't delete "part of an allocation". | |
| 18:23:19 | mriedem | why not? we amended part of an allocation | |
| 18:23:27 | mriedem | here https://review.openstack.org/#/c/487589/6/nova/scheduler/client/report.py | |
| 18:23:28 | jaypipes | no, we replaced it. | |
| 18:23:38 | mriedem | so we patched something in, we can't patch something out? | |
| 18:23:43 | jaypipes | PUT /allocations overwrites. | |
| 18:23:50 | mriedem | yes i know | |
| 18:23:51 | jaypipes | mriedem: hold up. | |
| 18:23:53 | mriedem | i'm saying, | |
| 18:24:01 | mriedem | we have to do the same thing for https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L1068 | |
| 18:24:07 | mriedem | to remove the allocations for the instance on the source node | |
| 18:24:14 | mriedem | but leave the allocations for the instance on the dest node | |
| 18:24:25 | mriedem | is it hangout time? | |
| 18:25:00 | dansmith | we either have to not delete, | |
| 18:25:05 | jaypipes | mriedem: the dest host will end up writing the allocation entirely (only including the allocated resources on the dest host) when the move operation completes successfully. | |
| 18:25:08 | dansmith | or put the allocation with our part removed | |
| 18:25:19 | dansmith | jaypipes: right but the source will then delete it without checking it | |
| 18:25:32 | dansmith | and I think those two things probably race with each other | |
| 18:25:44 | jaypipes | mriedem: so I think what we need to do is just ensure _update_usage_from_instances() does not call that _remove_deleted_instances_allocations() for instances currently in a move operation | |
| 18:26:10 | jaypipes | dansmith: understood. we just need to ensure we don't call that delete for any instances in a move operation | |
| 18:26:11 | dansmith | jaypipes: or instances that are just finishing a move operation that it thinks have been deleted | |
| 18:26:26 | dansmith | jaypipes: I don't think you can know that it's in a move operation if you're late to the party | |
| 18:26:43 | mriedem | dansmith: the instance would have a migration_context? | |
| 18:26:53 | bauzas | mriedem: back there | |
| 18:27:01 | jaypipes | guys, won't https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L1060 skip instances that are currently being mnoved? | |
| 18:27:01 | openstackgerrit | Merged openstack/python-novaclient master: Allow tuple as for nics value https://review.openstack.org/475816 | |
| 18:27:02 | dansmith | mriedem: not if you'reon the late end of the race | |
| 18:27:15 | dansmith | currently being moved is not the problem | |
| 18:27:23 | dansmith | "just moved a half second ago" is the problem right? | |
| 18:27:35 | jaypipes | but instance.host will be not None. | |
| 18:27:48 | jaypipes | the only time instance.host is None is when the instance is deleted. | |
| 18:28:38 | dansmith | jaypipes: actually not | |
| 18:28:50 | dansmith | jaypipes: that's reversed.. None means "not yet booted".. it's still $host after you delete | |
| 18:28:55 | dansmith | it's skipping not yet booted instances | |
| 18:29:09 | dansmith | if the hostname doesn't match you'll continue on to delete it because it's not on our host right? | |
| 18:29:17 | jaypipes | ah, shit. | |
| 18:29:21 | dansmith | since this logic was all based on just accounting for the local host, I don't think it quite works | |
| 18:29:22 | dansmith | plus, | |
| 18:29:32 | dansmith | there is a race between fetchign the instance and its host flipping to the other side | |
| 18:30:09 | mriedem | right the not instance.host is for scheduling instances the first time | |
| 18:30:11 | mriedem | per the comment | |
| 18:30:17 | mriedem | "# Allocations related to instances being scheduled should not be # deleted if we already wrote the allocation previously." | |
| 18:30:22 | dansmith | yeah | |
| 18:30:27 | dansmith | this was the "leafe race" | |
| 18:30:28 | mriedem | "already wrote the allocation previously" == in the scheduler | |
| 18:30:32 | mriedem | right | |
| 18:30:58 | jaypipes | but allocations_to_delete won't contain migrating instances. Because migrating instances are in RT.tracked_instances, no? | |
| 18:31:16 | dansmith | jaypipes: not at the instance after we complete the migration right? | |
| 18:31:47 | mriedem | that i don't know | |
| 18:31:49 | mriedem | b/c it's f'ed | |
| 18:31:50 | dansmith | jaypipes: either way, this is not a big deal: | |
| 18:32:10 | dansmith | just pull the allocation like we do, and if we're not in it, we don't touch it | |
| 18:32:28 | dansmith | and maybe something else about if there are two in there, then don't delete either, although not sure if that's important or not | |
| 18:32:43 | jaypipes | dansmith: sure, that's a fairly safe thing. | |
| 18:33:30 | jaypipes | ok, guys, lemme update the move patch. | |
| 18:33:31 | mriedem | just looking at where this starts from, the periodic is in here https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L667 | |
| 18:33:40 | mriedem | jaypipes: just do it on top of that one since it's in the gate | |
| 18:33:49 | mriedem | we get the instances for the host and node (source) | |
| 18:34:09 | mriedem | clear out all tracked instances https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L1030 | |
| 18:34:42 | dansmith | jaypipes: mriedem: ++ for not interrupting the one in the gate | |
| 18:34:48 | mriedem | then we go here https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L1044 | |
| 18:34:54 | mriedem | which is where i get lost | |
| 18:35:51 | bauzas | mriedem: what's the concern? | |
| 18:35:52 | mriedem | i think it just gets added here https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L995 | |
| 18:36:01 | mriedem | because we clear self.tracked_instances before calling that method | |
| 18:36:05 | jaypipes | yeah | |
| 18:36:08 | mriedem | so is_new_instance = uuid not in self.tracked_instances will be True | |
| 18:36:16 | mriedem | and then self.tracked_instances[uuid] = obj_base.obj_to_primitive(instance) | |
| 18:36:24 | bauzas | mriedem: you want to know whether migrating instances are in tracked_instances ? | |
| 18:36:36 | dansmith | mriedem: even still, we pull the allocation, we should never just blindly delete it without looking to make sure it's what we expect, which is kinda the point of the generation stuff, but in reverse here | |
| 18:36:48 | jaypipes | bauzas: they aren't. they are. depends on when in the update_available_resource() method you're looking at ;) | |
| 18:36:51 | bauzas | mriedem: if that's the question, I don't think so | |
| 18:37:02 | bauzas | mriedem: because we lookup at all the existing instances | |
| 18:37:03 | jaypipes | dansmith: ack | |
| 18:37:07 | mriedem | dansmith: yeah, that seems simplest too | |
| 18:37:16 | dansmith | anybody else looking forward to most of this code going away? :) | |
| 18:37:21 | mriedem | o/ | |
| 18:37:28 | bauzas | so since we update the instance.host once the migration is done, the target RT doesn't see it | |
| 18:37:31 | mriedem | so here is another question, which no one is going to like | |
| 18:37:44 | mriedem | this is going to be a change in how the compute is behaving | |
| 18:37:48 | mriedem | and ocata computes won't have this | |
| 18:37:50 | mriedem | so, | |
| 18:38:13 | jaypipes | dansmith: hmm... | |
| 18:38:20 | mriedem | do we (1) make claims in the scheduler dependent on pike computes, or (2) throw to the wind and rely on the dest periodic self-heal fixing the allocations? | |