| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-26 | |||
| 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 | mriedem | yuck | |
| 14:21:50 | dansmith | and not delete any that don't have host set | |
| 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 | bauzas | dansmith: I see your point | |
| 14:23:40 | dansmith | so we lookup _just_ those instances | |
| 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 | dansmith | bauzas: I'm not sure this integrated cleaning makes sense in the report client anyway | |
| 14:24:18 | bauzas | dansmith: I prefer the latter | |
| 14:24:23 | dansmith | right | |
| 14:24:27 | dansmith | it's code, it can change :) | |
| 14:24:27 | bauzas | dansmith: yup, zactly | |
| 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 | mriedem | that totally threw me | |
| 14:26:36 | bauzas | from placement | |
| 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 | self.scheduler_client.reportclient.remove_deleted_instances( | |
| 14:27:01 | mriedem | cn, self.tracked_instances.values()) | |
| 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 | 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:28:36 | mriedem | yes? | |
| 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 | |
| 14:32:07 | openstackgerrit | John Garbutt proposed openstack/nova master: Add description to policies in floating_ip files https://review.openstack.org/459791 | |
| 14:32:45 | dansmith | mriedem: oh I see you mean query for instances with host=None specifically? We'll want to also skip instances that match our hostname in case we're racing with our own build | |
| 14:32:54 | openstackgerrit | John Garbutt proposed openstack/nova master: Add description to policies in migrations.py https://review.openstack.org/459727 | |
| 14:32:59 | dansmith | otherwise we'd delete things that we just owned ourselves | |
| 14:33:00 | mriedem | dansmith: yes, i'm saying move the python condition into the sql query | |
| 14:33:09 | openstackgerrit | John Garbutt proposed openstack/nova master: Add description to policies in migrate_server.py https://review.openstack.org/459711 | |
| 14:33:10 | mriedem | so we're pulling less stuff we don't care about over rpc | |
| 14:33:22 | mriedem | like with a bunch of joins and shit we don't care about | |
| 14:33:30 | dansmith | mriedem: but it's a specific list of instance uuids and we care about all of them because of ^ | |
| 14:33:32 | mriedem | this is really just an existence check | |
| 14:33:37 | dansmith | it's not | |
| 14:33:50 | dansmith | because we need to delete allocations for instances that have been evacuated in our absence to another host | |
| 14:34:04 | dansmith | or instances that were locally deleted while we were gone | |
| 14:34:18 | dansmith | but not instances that are in flight (=None) or assigned to us while we were doing this (=$host) | |
| 14:34:24 | mriedem | i'll have to wait for the code change then to think about it | |
| 14:34:41 | dansmith | we have a call right now anyway | |
| 14:35:17 | mriedem | sure, i just want to make sure this is as fast as possible | |
| 14:35:22 | mriedem | expected_attrs=[] ftw | |
| 14:35:32 | dansmith | it will almost always be a no-op, but sure | |
| 14:48:54 | bauzas | mriedem: mmm, unless I misunderstood, you wanted to ask to use get_by_filters() by saying "not this host", right? | |
| 14:49:29 | bauzas | mriedem: but AFAIK you can use a filter like 'host'='foo', not 'host'= NOT 'foo' | |
| 14:49:40 | mriedem | host=None | |
| 14:49:47 | mriedem | we do that elsewhere in the code | |
| 14:50:11 | bauzas | mriedem: a-ha, I see | |
| 14:50:58 | bauzas | mriedem: but I also need to verify host=me | |
| 14:51:18 | mriedem | then i guess you can't filter in sql | |
| 14:51:36 | bauzas | yup, my point | |
| 14:51:39 | bauzas | I was confused | |
| 14:51:41 | bauzas | anyway | |
| 14:52:54 | openstackgerrit | John Garbutt proposed openstack/nova master: Add description to policies in migrate_server.py https://review.openstack.org/459711 | |
| 14:54:03 | mriedem | bauzas: just make sure you pass expected_attrs=[] so we don't do a bunch of unnecessary joins | |
| 14:54:10 | bauzas | I know | |
| 14:57:11 | dansmith | you guys are optimizing the heck out of a list of <=1 :P | |
| 14:57:12 | jaypipes | dansmith, cdent: answered your questions on https://review.openstack.org/#/c/457782 | |