| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-07-27 | |||
| 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 | openstackgerrit | Merged openstack/python-novaclient master: Allow tuple as for nics value https://review.openstack.org/475816 | |
| 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: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? | |
| 18:38:35 | jaypipes | dansmith: so we're calling get_allocations_for_resource_)provider() and passing in the source compute node UUID. | |
| 18:38:55 | jaypipes | dansmith: so we're guaranteed that the only allocations returned are instances that are "on the source host" according to placement. | |
| 18:39:09 | dansmith | mriedem: well the healing doesn't happen all the time, as he pointed out yesterday, only when instances are added/removed, right? | |
| 18:39:22 | dansmith | mriedem: so I'm not sure we'll actually heal over stuff that ocata computes don't do :/ | |
| 18:39:56 | dansmith | jaypipes: delete_allocation_for_instance() operates only on an instance uuid | |
| 18:40:16 | dansmith | jaypipes: if we call that after the flip happened for whatever reason we'll do the wrong thing | |
| 18:40:31 | dansmith | jaypipes: even if we started ten minutes ago and got blocked for a while or something | |
| 18:40:47 | dansmith | jaypipes: really we should use generation on delete to make sure we don't delete something we're not intending to :/ | |
| 18:40:49 | jaypipes | dansmith: right, but didn't you want me to check to see if the allocation had the source compute node UUID in it and if not, don't call delete allocation? | |
| 18:41:08 | dansmith | jaypipes: yes, but delete allocation is only fetching via instance uuid | |
| 18:41:21 | dansmith | https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L999-L1001 | |
| 18:41:36 | jaypipes | dansmith: I understand that, but get_allocations_for_resource_provider() will always only return allocations that "have the source compute node UUID in them" | |
| 18:41:53 | jaypipes | dansmith: so that logic isn't going to filter anything out... | |
| 18:41:59 | mriedem | hangout? | |
| 18:42:05 | dansmith | jaypipes: yeah dude I get that, but we query that way, then time passes, then we delete | |
| 18:42:50 | jaypipes | mriedem: sure | |
| 18:43:09 | dansmith | https://hangouts.google.com/call/5pmzfm5wpfckpptt4l5hxjw5cyu | |