| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-26 | |||
| 14:14:23 | bauzas | ah ok | |
| 14:14:25 | bauzas | yup | |
| 14:14:37 | bauzas | no races yet, but possible race after my changes, gotcha | |
| 14:14:41 | dansmith | yes | |
| 14:16:12 | mriedem | althought, | |
| 14:16:16 | mriedem | *although, | |
| 14:16:30 | mriedem | now that you say this, i'm not sure where in the code #4 happens | |
| 14:16:45 | dansmith | hang on I'll show you | |
| 14:16:56 | bauzas | unless I'm blind, the only case where we get allocations, it's per instance | |
| 14:17:04 | bauzas | and we never delete them | |
| 14:17:19 | bauzas | only if when tracking, we removed the instance uuid | |
| 14:17:21 | dansmith | I had to find this for jaypipes a couple weeks ago | |
| 14:17:48 | bauzas | which is happening because we do get the list of instances per host | |
| 14:17:51 | dansmith | https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L873-L885 | |
| 14:17:57 | bauzas | and we compare to what we have in memory | |
| 14:18:04 | dansmith | we get allocations based on compute node, | |
| 14:18:14 | dansmith | then any instances we don't have in the instances_dict provided get their allocations deleted | |
| 14:18:25 | bauzas | oh fuck, missed that one | |
| 14:18:30 | mriedem | dansmith: i saw that, | |
| 14:18:48 | mriedem | but how is an instance we're about to build that doesn't have instance.host set showing up in that list? | |
| 14:18:51 | bauzas | dansmith: but we should get the instance uuid to be scheduled part of the argument | |
| 14:19:04 | mriedem | via the allocations tied to the RP? | |
| 14:19:13 | mriedem | oh i see | |
| 14:19:14 | mriedem | yeah | |
| 14:19:16 | dansmith | mriedem: it's not in instances_uuids yet, but is in get_allocations() | |
| 14:19:20 | mriedem | right | |
| 14:19:23 | dansmith | bauzas: no | |
| 14:19:24 | mriedem | instance_uuids could be empty | |
| 14:19:27 | dansmith | right | |
| 14:20:12 | bauzas | if there are no tracked instances, correct | |
| 14:20:21 | openstackgerrit | John Garbutt proposed openstack/nova master: Remove all discoverable policy rules https://review.openstack.org/459608 | |
| 14:21:06 | mriedem | self.tracked_instances[uuid] = obj_base.obj_to_primitive(instance) | |
| 14:21:09 | bauzas | mriedem: dansmith: oooh man, I see the problem | |
| 14:21:19 | mriedem | so rather than pass instance_uuids to remove_deleted_instances, we'll have to pass the actual instances | |
| 14:21:23 | mriedem | to see if host is set or not | |
| 14:21:25 | bauzas | since instance.host is none, then the get_instances_per_host doesn't get it | |
| 14:21:26 | dansmith | mriedem: no | |
| 14:21:34 | dansmith | mriedem: because instances with no host won't be in tracked_instances | |
| 14:21:39 | bauzas | yup | |
| 14:21:43 | dansmith | mriedem: we have to look up instances we think we want to delete allocations for | |
| 14:21:48 | mriedem | oh right | |
| 14:21:49 | bauzas | I now see the potential race | |
| 14:21:50 | dansmith | and not delete any that don't have host set | |
| 14:21:50 | mriedem | yuck | |
| 14:22:25 | dansmith | usually the list we think we need to delete will be empty, so it's not a big deal | |
| 14:22:41 | bauzas | mmm | |
| 14:22:44 | dansmith | only if we're really healing from some big failure, or if we lose this race, and then it'll likely only be one instance | |
| 14:23:09 | mriedem | can we filter on host when pulling that instance list to try and narrow the results? | |
| 14:23:16 | dansmith | no | |
| 14:23:20 | dansmith | we don't need to | |
| 14:23:25 | dansmith | but it doesn't have host set, so.. | |
| 14:23:33 | dansmith | we will have a list of instance uuids we need to consider, | |
| 14:23:40 | dansmith | so we lookup _just_ those instances | |
| 14:23:40 | bauzas | dansmith: I see your point | |
| 14:23:48 | dansmith | and they will only be instances that have allocations against our compute node | |
| 14:23:55 | bauzas | dansmith: but then we need to lookup those instances in the report client code | |
| 14:24:00 | dansmith | so it's not like it's all unscheduled instances or anything | |
| 14:24:08 | dansmith | bauzas: or refactor how this cleanup works | |
| 14:24:18 | bauzas | dansmith: I prefer the latter | |
| 14:24:18 | dansmith | bauzas: I'm not sure this integrated cleaning makes sense in the report client anyway | |
| 14:24:23 | dansmith | right | |
| 14:24:27 | bauzas | dansmith: yup, zactly | |
| 14:24:27 | dansmith | it's code, it can change :) | |
| 14:24:35 | bauzas | okay, hands on | |
| 14:25:06 | bauzas | there is only one caller for remove_deleted_instances, not a big deal indeed | |
| 14:25:18 | bauzas | dansmith: thanks for the explanation | |
| 14:25:20 | dansmith | it should be named something less obscure too | |
| 14:25:24 | dansmith | and more allocationy | |
| 14:25:29 | bauzas | I somehow captured that a while ago, but my brain fried in between | |
| 14:25:45 | bauzas | I should have been more explicit in the spec | |
| 14:25:51 | mriedem | heh this is blowing my mind https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L878 | |
| 14:25:59 | mriedem | apparently instance_uuids isn't a list of instance uuids | |
| 14:26:04 | mriedem | because instance['uuid'] | |
| 14:26:10 | bauzas | yup | |
| 14:26:33 | bauzas | it's not the instance object, it's rather the instance payload we pass back | |
| 14:26:36 | bauzas | from placement | |
| 14:26:36 | mriedem | that totally threw me | |
| 14:26:47 | mriedem | it's not an instance object, it's the primitive | |
| 14:26:54 | mriedem | self.tracked_instances[uuid] = obj_base.obj_to_primitive(instance) | |
| 14:27:01 | mriedem | cn, self.tracked_instances.values()) | |
| 14:27:01 | mriedem | self.scheduler_client.reportclient.remove_deleted_instances( | |
| 14:27:06 | bauzas | correct, sorry | |
| 14:27:10 | bauzas | anyway, on it | |
| 14:27:26 | dansmith | it doesn't matter for this anyway | |
| 14:27:35 | dansmith | we don't need the instance object because those are instances we _don't_ care about | |
| 14:28:36 | mriedem | yes? | |
| 14:28:36 | mriedem | right, because if instance_uuids is empty, but allocations isn't, removed_instances = set(allocations.keys()) - set(instance_dict.keys()) will make us remove all of those allocations | |
| 14:29:27 | mriedem | so we have to get the list of instances from the allocations we're about to remove, and filter out the ones where instance.host is None | |
| 14:29:43 | dansmith | yes | |
| 14:30:03 | mriedem | and that's where i was saying we could filter on host when pulling the instances from the db | |
| 14:30:07 | mriedem | as an optimization | |
| 14:30:30 | dansmith | it's not an optimization | |
| 14:30:40 | dansmith | we can't filter by host because these instances necessarily are host=None | |
| 14:30:41 | mriedem | select * from nova.instances where uuid in (for allocation.consumer_id in allocations) and instance.host is not None; | |
| 14:30:58 | dansmith | we're already doing that effectively, | |
| 14:31:11 | dansmith | because we're asking placement for instances with allocations against our host | |
| 14:31:14 | bauzas | lemme try something, POC should be in the next couple of minutes | |
| 14:31:19 | bauzas | easier to talk | |
| 14:31:55 | mriedem | dansmith: there is going to be a conditional in the code once we pull the instance list back from the db to see if instance.host is None or not, right? | |
| 14:31:56 | openstackgerrit | John Garbutt proposed openstack/nova master: Remove all discoverable policy rules https://review.openstack.org/459608 | |