Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-04-26
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
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
14:58:26 mriedem lyarwood: can you hit some of these when you get a chance? https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:stable/newton
14:58:49 cdent thanks jaypipes
14:59:26 bauzas cdent: correct me if I'm wrong but...
14:59:47 bauzas cdent: is https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L871 returning a list of instance UUIDs ?
15:00:15 openstackgerrit Huan Xie proposed openstack/nova master: WIP: XenAPI use os-xenapi v2 in nova https://review.openstack.org/453493
15:00:42 cdent bauzas it's more than that, but does include instance uuids
15:01:03 bauzas cdent: it's keyed by that ? https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L880
15:01:14 mdbooth cdent jaypipes: Any reason we can't return structured objects from get_inventory(): https://review.openstack.org/#/c/457782/ ?
15:01:39 mdbooth dicts with grab bags of fields returned by drivers make me nervous
15:01:55 cdent bauzas: https://github.com/openstack/nova/blob/master/nova/api/openstack/placement/handlers/allocation.py#L138
15:02:00 lyarwood mriedem: ack, sorry did mean to get around to that earlier in the week.
15:02:21 bauzas cdent: cool then
15:02:21 jaypipes mdbooth: eventually, there will be a bit more structure there, when nested resource providers lands this patch: https://review.openstack.org/#/c/415920/
15:02:37 bauzas cdent: FWIW the gabbi tests for allocations are a bit long to read :)

Earlier   Later