Posted	Nick	Remark
#openstack-nova - 2017-04-26
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
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'

Earlier   Later