Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-08-12
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 cdent lock _after_ getting the list
15:03:37 mriedem i know what you're asking
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 sean-k-mooney getting the list shouldnt be the moest expensive part though right its caulating the useage?
15:06:52 mriedem so getting 1000+ instances for every instance operation on the same host is blocked by this big lock
15:07:11 sean-k-mooney e.g. https://github.com/openstack/nova/blob/52b9359d6c6f387b8b9728c723c9a2501136d605/nova/compute/resource_tracker.py#L751-L752
15:07:11 mriedem it's 1000+ instances + several joins to other tables
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 cdent and for profiling/testing is not an issue
15:15:32 mriedem cdent: efried: it's not just isolation, it's also to support rolling upgrades
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
15:20:29 cdent go RT
15:21:13 sean-k-mooney well anythin relying on the perodic task to fix up allocation could be off but it should get fixed on the next run
15:21:27 sean-k-mooney missing frees is not that concuerning but missing claims would be
15:21:33 cdent sean-k-mooney: yeah, which ought to be fine
15:21:54 mriedem sean-k-mooney: the periodic doesn't fix up allocations...
15:21:59 mriedem not since ocata/pike anyway
15:22:36 sean-k-mooney out side of the migration ones? i though it healed allcoation for instace once the migration was conrimed
15:23:09 mriedem the rt does not do that
15:23:12 cdent sure it does: it spends lots of time in here: https://github.com/openstack/nova/blob/52b9359d6c6f387b8b9728c723c9a2501136d605/nova/compute/resource_tracker.py#L1314
15:23:23 cdent orphans
15:23:23 mriedem the compute manager explicitly does stuff with migration-based allocations
15:23:54 mriedem cdent: that's about it since all of the other allocation "healing" code was removed
15:23:54 cdent ever single allocation for instance.uuid is compared to each instance on the host
15:24:13 cdent that chunk can get really loud becuase of https://github.com/openstack/nova/blob/52b9359d6c6f387b8b9728c723c9a2501136d605/nova/compute/resource_tracker.py#L1340
15:24:22 cdent commenting out that line can save a bunch of log noise
15:24:51 mriedem debug("things are working normally")
15:24:55 cdent and 1372 there is one last remove
15:25:53 sean-k-mooney mriedem: i dont know im suspios that we normally would print that :P
15:26:14 sean-k-mooney but yes that does not really add much value
15:26:30 cdent if you both agree, I can put up a patch to remove it?
15:26:45 cdent meanwhile, I will create 1000 fake instances again
15:27:17 mriedem there used to be an audit level which is lower than debug, i'm not sure i'm totally in favor of removing it,
15:27:21 mriedem as the comment at the top says,
15:27:22 mriedem # NOTE(jaypipes): All of this code sucks.
15:27:34 mriedem but when i'm debugging rt issues, the more logging i have the better

Earlier   Later