| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-11-26 | |||
| 22:31:47 | mriedem | because the claim is made on the dest during prep_resize | |
| 22:31:52 | efried | mriedem: Sorry, I'm back now. We're trying to figure out how to know whether allocation ratios were defaulted by placement or conf or nova? | |
| 22:32:57 | mriedem | efried: yeah - need to determine when initial_*_allocation_ratio config should be used (first time the compute was created) and report that to placement, or if we should set allocation ratios because CONF.*_allocation_ratio is some non-default value, or if we should leave it alone because the admin set some allocation_ratio via the placement API directly | |
| 22:33:24 | mriedem | i think we can know the initial case from upt if the provider tree doesn't have those inventory resource class keys in it | |
| 22:33:44 | mriedem | otherwise only ever overwrite if CONF.*_allocation_ratio is not None | |
| 22:34:05 | efried | "if the provider tree doesn't have those inventory resource class keys" <== I don't think this can happen within upt | |
| 22:34:30 | mriedem | on initial startup, wont upt just have a root provider with no inventory on it? | |
| 22:34:35 | mriedem | the driver reports the inventory initially | |
| 22:34:41 | efried | I don't think so, because of the (legacy) code that's bootstrapping the CPU/MEMORY_MB/DISK_GB inventories before we get to upt | |
| 22:35:03 | mriedem | i'm not sure what you're referring to | |
| 22:35:06 | efried | I could be wrong, it's possible we've removed that at this point. | |
| 22:35:35 | efried | but there was a time when code *outside* of virt driver space would do the initial populate of placement | |
| 22:35:52 | mriedem | by calling driver.get_inventory() right? | |
| 22:36:04 | mriedem | that no longer happens if the driver implements upt | |
| 22:36:05 | efried | get_available_resource | |
| 22:36:14 | mriedem | yeah, get_available_resource was before get_inventory | |
| 22:36:14 | efried | which *does* still get called I believe. | |
| 22:36:33 | mriedem | i think it's upt -> get_inventory -> get_available_resource | |
| 22:36:37 | efried | gar doesn't get called in the rt update flow, but it still gets called somewhere else, lemme find it. | |
| 22:37:03 | mriedem | https://github.com/openstack/nova/blob/c1de096098344c733565c244163fc3ebf8c35e68/nova/compute/resource_tracker.py#L711 | |
| 22:37:50 | mriedem | we'd create the provider root initially here https://github.com/openstack/nova/blob/c1de096098344c733565c244163fc3ebf8c35e68/nova/compute/resource_tracker.py#L921 | |
| 22:38:05 | mriedem | then flush inventory from upt here https://github.com/openstack/nova/blob/c1de096098344c733565c244163fc3ebf8c35e68/nova/compute/resource_tracker.py#L944 | |
| 22:38:13 | mriedem | else this https://github.com/openstack/nova/blob/c1de096098344c733565c244163fc3ebf8c35e68/nova/compute/resource_tracker.py#L951 | |
| 22:38:20 | mriedem | failing those, this https://github.com/openstack/nova/blob/c1de096098344c733565c244163fc3ebf8c35e68/nova/compute/resource_tracker.py#L961 | |
| 22:38:23 | mriedem | which would use the values from gar | |
| 22:38:44 | mriedem | https://github.com/openstack/nova/blob/c1de096098344c733565c244163fc3ebf8c35e68/nova/scheduler/client/report.py#L1442 | |
| 22:39:26 | mriedem | might be interesting to drop that final path, | |
| 22:39:39 | mriedem | looks like maybe only the hyperv driver hasn't implemented an alternative | |
| 22:40:11 | mriedem | oh and zvm | |
| 22:40:15 | mriedem | forgot that was in tree... | |
| 22:40:52 | efried | yeah, I remember looking at this when I was working on https://review.openstack.org/#/c/615705/ to see if I could just real quick implement upt for all the drivers. | |
| 22:41:11 | efried | and realizing that was going to be more work than I was ready to undertake at the time. | |
| 22:41:50 | efried | but unwinding the gar stuff, that's going to require some synapses I haven't yet explored. | |
| 22:42:01 | mriedem | heh, good thing you noted that because i was just thinking about removing that old code path to see what would break | |
| 22:42:33 | mriedem | i haven't seen hyper-v run on the latest version of that, but zvm failed | |
| 22:42:35 | efried | any case, I think the point is that we can't rely on upt being in the code path for initial population of the root provider inventories. | |
| 22:42:59 | efried | yet | |
| 22:43:05 | mriedem | no it doesn't need to be though | |
| 22:43:21 | efried | if we want upt to be able to decide whether to use initial_*_allocation_ratio it does. | |
| 22:43:22 | mriedem | as of https://review.openstack.org/#/c/613126/, if a driver implements upt, it sets the allocation_ratio on the inventory it reports, | |
| 22:43:28 | mriedem | otherwise that normalize method in the RT does | |
| 22:43:31 | mriedem | _normalize_inventory_from_cn_obj | |
| 22:43:52 | efried | right, it sets the allocation ratio based on the non-initial_* conf values. | |
| 22:43:53 | mriedem | i think upt can determine if initi allocation ratios can be used though | |
| 22:43:57 | efried | how? | |
| 22:44:11 | mriedem | if the inventory for a given class is not in the tree, it's initial | |
| 22:45:13 | efried | Wait, did you just prove (to yourself, at least) that if upt is implemented, it *does* get first crack at the root provider inventory? | |
| 22:45:51 | mriedem | yes | |
| 22:45:59 | mriedem | i believe so anyway | |
| 22:46:22 | efried | should be relatively easy to prove with a func test? | |
| 22:46:54 | mriedem | i think the one i wrote here will do it https://review.openstack.org/#/c/613126/4/nova/tests/functional/compute/test_resource_tracker.py | |
| 22:47:08 | mriedem | along with the fake driver todo being resolved https://review.openstack.org/#/c/613126/4/nova/virt/fake.py | |
| 22:47:28 | mriedem | but yeah https://review.openstack.org/#/c/602804/ really needs to run through the expected / support scenarios in a functional test, | |
| 22:47:30 | mriedem | 1. initial create | |
| 22:47:35 | mriedem | 2. overwrite in placement API | |
| 22:47:39 | mriedem | 3. overwrite via config | |
| 22:48:01 | mriedem | and make sure #2 doesn't get f'ed up when the periodic runs | |
| 22:49:59 | efried | so we're talking about changing https://review.openstack.org/#/c/613126/4/nova/virt/libvirt/driver.py and its brethren to have logic like: | |
| 22:49:59 | efried | inv = ptree.data(root) | |
| 22:49:59 | efried | if CPU not in inv: ratio = CONF.initial_cpu_allocation_ratio or 16.0 | |
| 22:49:59 | efried | else: ratio = CONF.cpu_allocation_ratio or 16.0 | |
| 22:49:59 | efried | and similar for mem/disk? | |
| 22:50:18 | efried | oh, except f'ed up when periodic runs | |
| 22:50:23 | mriedem | the "or 16.0" gets removed | |
| 22:50:33 | mriedem | CONF.initial_cpu_allocation_ratio defaults to 16.0 | |
| 22:50:33 | efried | the second one? | |
| 22:50:37 | mriedem | both | |
| 22:51:04 | mriedem | the non-initial else becomes only set the ratio if CONF.cpu_allocation_ratio is not None | |
| 22:51:11 | efried | right | |
| 22:51:12 | efried | so | |
| 22:51:13 | mriedem | in other words, don't change the allocation ratio if it was set externally | |
| 22:51:17 | mriedem | and config hasn't changed | |
| 22:51:55 | efried | if CPU not in inv: ratio = CONF.initial_cpu_allocation_ratio # which defaults to 16.0 | |
| 22:51:55 | efried | elif CONF.cpu_allocation_ratio: ratio = CONF.cpu_allocation_ratio | |
| 22:51:55 | efried | # else no-op, leave it tf alone | |
| 22:52:13 | mriedem | exactly | |
| 22:52:35 | efried | I guess technically that last bit would have to be | |
| 22:52:35 | efried | else: ratio = data[CPU][ratio] | |
| 22:52:47 | efried | so it doesn't get omitted and wind up with the placement default :( | |
| 22:52:50 | mriedem | yup | |
| 22:53:21 | efried | okay, I can dig it. Assuming we can prove the upt-gets-initial-look thing. Let me take a look at that test you highlighted... | |
| 22:55:06 | efried | still looks slightly holey. | |
| 22:55:27 | efried | Let me go list all the permutations... | |
| 22:55:43 | mriedem | in yikun's patch? | |
| 22:55:47 | mriedem | i'm dumping notes in there | |
| 22:55:58 | efried | I'll just pastebin 'em for now | |
| 22:58:00 | efried | mm, if both values are set we want to end up with the conf one, so my above algo won't quite work | |
| 22:58:40 | mriedem | why not? | |
| 22:58:57 | efried | you would end up with the CONF.initial | |
| 22:59:05 | efried | until the next time update runs. | |
| 22:59:27 | efried | works if you reverse the conditions I think. | |
| 23:00:11 | efried | if CONF.cpu_allocation_ratio: ratio = CONF.cpu_allocation_ratio | |
| 23:00:11 | efried | elif CPU not in inv: ratio = CONF.initial_cpu_allocation_ratio # which defaults to 16.0 | |
| 23:00:11 | efried | else: ratio = data[CPU][allocation_ratio] | |
| 23:00:15 | mriedem | you should only get initial config if CPU not in inv though | |
| 23:00:46 | mriedem | ok i think either would be ok | |
| 23:00:53 | openstackgerrit | Michael Still proposed openstack/nova master: Move bridge creation to privsep. https://review.openstack.org/620180 | |
| 23:01:00 | efried | but in all cases if CONF.cpu_allocation_ratio is set you want *that* value - i.e. ignore initial_* | |
| 23:01:05 | mriedem | well well well, look who it is | |
| 23:01:12 | efried | brb | |