| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-26 | |||
| 14:06:39 | mriedem | bauzas: that was a race that edleafe pointed out i think | |
| 14:06:51 | bauzas | mriedem: dansmith: jaypipes: the fact that we post allocations for instances that we discover, right? | |
| 14:06:58 | dansmith | the pre-scheduling race or something? | |
| 14:07:06 | bauzas | I think so | |
| 14:07:06 | sfinucan | mriedem: niiiiice | |
| 14:07:16 | bauzas | anyway, just writing the patch, comments welcome | |
| 14:07:34 | mriedem | oh right, | |
| 14:07:35 | dansmith | like, you create an instance, don't assign the host, do create allocations, and that host deletes the allocations before building because of self-heal | |
| 14:07:42 | mriedem | because instance.host isn't set until we're in the RT | |
| 14:07:45 | dansmith | yeah | |
| 14:09:39 | bauzas | dansmith: wait | |
| 14:09:56 | bauzas | so we create allocations in self-heal | |
| 14:10:06 | dansmith | and delete them | |
| 14:10:09 | bauzas | but if we can't track it, then we delete the allocations | |
| 14:10:15 | bauzas | correct | |
| 14:10:34 | bauzas | so, we should filter out to delete the allocation, or to post it ? | |
| 14:11:11 | dansmith | not sure what you mean | |
| 14:11:21 | dansmith | we should avoid deleting it if we think we should but no host is set on the instance | |
| 14:11:36 | dansmith | like instance.host==None | |
| 14:11:56 | bauzas | we delete the allocations for instances that aren't tracked | |
| 14:12:16 | dansmith | ...right | |
| 14:12:17 | bauzas | okay, so the problem is about deleting allocations | |
| 14:12:21 | dansmith | yes | |
| 14:12:23 | bauzas | lemme see the code | |
| 14:13:13 | bauzas | I mean, can't see the race | |
| 14:13:29 | dansmith | 1. conductor creates the instance in the db, instance.host=None | |
| 14:13:40 | dansmith | 2. Conductor/scheduler/whatever creates the allocations in placement | |
| 14:13:52 | dansmith | 3. Conductor gets ready to send the instance to compute to build | |
| 14:14:07 | dansmith | 4. Compute runs self heal before that happens, sees allocations it doesn't know about, deletes them | |
| 14:14:14 | dansmith | 5. Instance arrives at compute ready to build | |
| 14:14:18 | dansmith | make sense? | |
| 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 | |