| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-12 | |||
| 14:39:30 | dansmith | okay | |
| 14:39:54 | tssurya | mriedem: gdi, thanks for letting me know | |
| 14:40:06 | dansmith | hah | |
| 14:40:08 | tssurya | that means I need to again change the version on the ironic side also | |
| 14:40:13 | dansmith | we've ruined tssurya :) | |
| 14:40:27 | tssurya | dansmith: tear emoji | |
| 14:40:28 | mriedem | used to be so innocent | |
| 14:40:45 | mriedem | https://review.opendev.org/#/q/file:%255Enova/api/openstack/compute/rest_api_version_history.rst+status:open | |
| 14:40:49 | dansmith | tssurya: I meant ruined because we've got you swearing when api versions change :P | |
| 14:41:05 | tssurya | dansmith: true that | |
| 14:41:19 | dansmith | tssurya: I say rebase that one on the cleanup to fix the version, and the ironic side, and let's push to get the acks in place so we can try to get it landed before it happens again | |
| 14:41:22 | tssurya | my office mate thinks I am swearing at him | |
| 14:41:28 | dansmith | heh | |
| 14:41:49 | tssurya | dansmith: ack, on it | |
| 14:45:52 | TheJulia | tssurya: will order of merging matter? Trying to figure out if I should try and get more eyes on the ironic side this week | |
| 14:46:32 | tssurya | nova one has to land first technically, but they just want to know if the ironic client side change looks good | |
| 14:47:06 | tssurya | TheJulia: more eyes this week would be great | |
| 14:47:31 | dansmith | what tssurya said | |
| 14:47:52 | TheJulia | tssurya: ack | |
| 14:50:39 | efried | tssurya: Do you have someone to take over for you if changes are needed while you're sipping mai tais on the beach? | |
| 14:58:05 | cdent | mriedem, dansmith : What do you imagine the consequences of moving the get_by_host_node in _update_available_resource out from under the COMPUTE_RESOURCE_SEMAPHORE would be? https://github.com/openstack/nova/blob/52b9359d6c6f387b8b9728c723c9a2501136d605/nova/compute/resource_tracker.py#L744 | |
| 14:58:25 | cdent | on a host with many insgtn | |
| 14:58:28 | cdent | oopes | |
| 14:58:46 | cdent | oh a host with many instances, that lock blocks new instance creation | |
| 15:00:09 | cdent | and a large portion of the time under the lock is spent in get_by_host_and_node assuming the is_bfv cache is hot (if it's cold it is _all_ there) | |
| 15:01:11 | sean-k-mooney | cdent: you might be able to optimise it without fully removing the lock. e.g. caulate the usage for each instance without the lock and keep track of the instace checked. aquire the lock and check that all instace check are still present and no others have been added then update the usage and release the lock. | |
| 15:01:39 | sean-k-mooney | if an instance has been added or removed then you can just make that update under the lock. | |
| 15:02:27 | mriedem | cdent: the consequence could be a race where we don't report usage or claim properly because the underlying resource usage changed since we got the lock | |
| 15:03:26 | cdent | mriedem: are you certain? I'm not saying remove the lock. I'm saying not use the lock while getting the list of instances used by _update_available_resources | |
| 15:03:37 | mriedem | i know what you're asking | |
| 15:03:37 | cdent | lock _after_ getting the list | |
| 15:03:57 | mriedem | you're saying move the get here https://github.com/openstack/nova/blob/52b9359d6c6f387b8b9728c723c9a2501136d605/nova/compute/resource_tracker.py#L710 | |
| 15:04:05 | mriedem | and pass the instances into _update_available_resource | |
| 15:04:20 | mriedem | sean-k-mooney: how would you know if an instance has been added or removed? | |
| 15:04:28 | mriedem | between the time you got the list and got the lock? | |
| 15:05:03 | mriedem | cdent: presumably you could have: | |
| 15:05:07 | sean-k-mooney | you would have to key two list of instances. one that you got outside the lock and then inside it you would get the list again | |
| 15:05:39 | sean-k-mooney | you can then caulated teh added and removed instances | |
| 15:05:46 | mriedem | t0: thread 1 is getting the list of instances on the host, t1: thread 2 gets the lock and is adding a new instance claim to the host which won't be in the list returned in thread 1, t2: thread 1 gets the lock with a list of instances that doesn't include the one from thread2 | |
| 15:06:05 | mriedem | sean-k-mooney: getting hte list again within the lock defeats the purpose of what cdent is trying to do | |
| 15:06:10 | sean-k-mooney | if the list are the same the pending updats shoudl be correct if not you either need to recaulate it entirely or comute the delta | |
| 15:06:14 | mriedem | which is reduce the amount of time spent *within* the lock | |
| 15:06:26 | cdent | yes, that is the goal | |
| 15:06:38 | mriedem | b/c vcenter has 1000+ instances on the same compute service host / RT | |
| 15:06:52 | mriedem | so getting 1000+ instances for every instance operation on the same host is blocked by this big lock | |
| 15:06:52 | sean-k-mooney | getting the list shouldnt be the moest expensive part though right its caulating the useage? | |
| 15:07:11 | mriedem | it's 1000+ instances + several joins to other tables | |
| 15:07:11 | sean-k-mooney | e.g. https://github.com/openstack/nova/blob/52b9359d6c6f387b8b9728c723c9a2501136d605/nova/compute/resource_tracker.py#L751-L752 | |
| 15:07:16 | mriedem | over rpc as well.... | |
| 15:07:28 | mriedem | cdent: i'm curious, what does vio set for rpc_response_timeout? | |
| 15:07:39 | mriedem | considering something like this has to pull 1K instances from the db over rpc | |
| 15:07:46 | cdent | getting the instances is by far the most expensive chunk of _update_avail when you have many instances | |
| 15:08:21 | mriedem | reminds me of this https://review.opendev.org/#/c/633042/ | |
| 15:08:26 | cdent | mriedem: something relatively high, but it's not _that_ slow as to cause a problem, rather that the lock can be around for a long time | |
| 15:08:27 | sean-k-mooney | mriedem: 1000+ on vmware because its clustering multiple server under one compute service | |
| 15:08:40 | mriedem | sean-k-mooney: yes | |
| 15:08:49 | mriedem | cdent: but over the default 60 seconds? | |
| 15:09:02 | sean-k-mooney | cdent: ok that is surprising to me but if that is the most expesnive part then yes my suggestion wont help | |
| 15:09:22 | cdent | mriedem: I don't have the details to hand, but yes | |
| 15:09:46 | mriedem | cdent: you could potentially detect if you lost a race if there is something in self.tracked_instances that isn't in the list of instances you just pulled from the db | |
| 15:09:48 | cdent | sean-k-mooney: it depends on whether you are doing the loop the first time or subsequence times. 1st time is_bfv is the expensive part | |
| 15:10:16 | sean-k-mooney | i had expected self._update_usage_from_instances ot be more costly | |
| 15:10:32 | cdent | sean-k-mooney: yes, when bfv cache is cold, not but not otherwise | |
| 15:10:40 | mriedem | cdent: i'm assuming is_bfv is expensive since it has to make the is_volume_backed call which needs to get the bdms | |
| 15:10:46 | cdent | mriedem: yes | |
| 15:10:51 | cdent | and 1000 of those adds up | |
| 15:11:24 | mriedem | yeah...there is a change floating around where i asked someone if they could put a sqla joined field on the instances model to detect in a join query if an instance is volume-backed so we could optimize that | |
| 15:11:26 | mriedem | sec | |
| 15:11:50 | mriedem | the logic is pretty simple i think - is there a boot_index=0 bdm with destination_type=volume for this instance | |
| 15:11:52 | mriedem | if so, it's bfv | |
| 15:12:14 | cdent | once the cache is hot, it disappears as a problem which suggests that a similar instance cache would have similar properties... | |
| 15:12:26 | cdent | but managing such a cache is...tricky | |
| 15:12:30 | mriedem | well, my point is, | |
| 15:12:50 | cdent | but is exactly what I would think a "resource tracker" would do... | |
| 15:12:54 | mriedem | rather than (1) big InstanceList.get and then (2) big is_bfv cache build, if we could get the is_bfv value per instance in (1) then we avoid (2) | |
| 15:13:11 | mriedem | https://review.opendev.org/#/c/612626/10/nova/db/sqlalchemy/api.py | |
| 15:13:45 | cdent | sure, but that only helps on the first trip, right? | |
| 15:14:30 | efried | tssurya: I left a handful of comments as well | |
| 15:14:32 | cdent | btw: have you seen my comments about how the eventlet profiler gets all chundered up when RPC is in play? | |
| 15:14:48 | mriedem | no | |
| 15:14:53 | efried | cdent: just commented on that, I don't think n-cpu is supposed to talk to the db. | |
| 15:14:54 | cdent | turning off the indirection_api fixes it | |
| 15:15:10 | cdent | efried: only for hypervisor isolation, which is not a problem for vsphere... | |
| 15:15:32 | mriedem | cdent: efried: it's not just isolation, it's also to support rolling upgrades | |
| 15:15:32 | cdent | and for profiling/testing is not an issue | |
| 15:15:44 | cdent | which vsphere doesn't do either :) | |
| 15:15:46 | mriedem | temporarily disabling for profilng is one thing | |
| 15:16:03 | cdent | yeah, I wouldn't suggest there being some kind of config setting for it | |
| 15:16:05 | mriedem | we need a mogan for nova+ironic and a vogan for nova+vmware | |
| 15:16:10 | mriedem | and nova can then just be kvm like it already is | |
| 15:16:27 | cdent | i actually think that's probalby a good idea, sadly | |
| 15:16:48 | cdent | trying to be all things to many things is terrible | |
| 15:17:01 | mriedem | cdent: anyway, i'd probably investigate trying to detect races by comparing the instances you get from the db to the set of tracked_instances in the rt after you get the lock | |
| 15:17:11 | mriedem | and determine if you need to adjust your 'instances' like which might just be re-getting it | |
| 15:17:31 | mriedem | and then, like run rally against that to see if you can break it? idk. | |
| 15:17:44 | cdent | I'm still unclear on where a lost race experiences a problem. what breaks | |
| 15:18:23 | mriedem | claims related stuff, which at this point is numa/pci i think, | |
| 15:18:26 | sean-k-mooney | i think it woudl only be an issue for resouce that are not tracked in placmenet | |
| 15:18:29 | mriedem | since we just removed the cpu/ram/disk claims | |
| 15:20:05 | mriedem | tracked_migrations could also be wonky but on a smaller scale / tighter race | |