| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-26 | |||
| 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 | |
| 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 | 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:21 | bauzas | cdent: cool then | |
| 15:02:37 | bauzas | cdent: FWIW the gabbi tests for allocations are a bit long to read :) | |
| 15:02:42 | cdent | mdbooth: we're trying to kill off nova-side use of placement-used ovos, especially when rcp not involved | |
| 15:02:47 | jaypipes | mdbooth: but we didn't want to depend on versioned objects defined in nova/objects/resource_provider.py within the virt driver layer. | |
| 15:03:13 | jaypipes | bauzas: compared to...? | |
| 15:03:30 | bauzas | jaypipes: for knowing which allocations to delete | |
| 15:03:40 | bauzas | anyway, patch is close to be up | |
| 15:03:58 | jaypipes | bauzas: no, I mean the gabbits are hard to read, compared to what? :) | |
| 15:04:19 | cdent | bauzas: see line 51 of https://etherpad.openstack.org/p/placement-newton-leftovers | |
| 15:04:20 | bauzas | compared to API docs :) | |
| 15:04:23 | mriedem | there is a note in there that the get allocations gabbits can't sub the left side of the json path b/c of a bug | |
| 15:04:40 | mriedem | they are working on api-ref | |
| 15:04:50 | cdent | s/they/we/ | |
| 15:04:57 | cdent | as in, all of us | |
| 15:05:13 | mriedem | us vs them | |
| 15:05:15 | mriedem | duh | |
| 15:05:31 | cdent | mriedem: are you the wolf or the dog today? | |
| 15:05:41 | mriedem | i meant, they as in 'people that aren't me' | |
| 15:05:42 | mriedem | :) | |
| 15:05:46 | mriedem | b/c i'm not being helpful | |
| 15:06:43 | cdent | the docs, although not yet published, are coming together nicely and look very nice and polished | |
| 15:08:21 | cdent | given sufficient time/energy/people it would be great to decompose the gabbi files into smaller pieces, been on the looking for volunteers todo list for months | |
| 15:10:41 | openstackgerrit | John Garbutt proposed openstack/nova master: Add description to policies in floating_ip files https://review.openstack.org/459791 | |